Messages in this thread | | | Date | Wed, 21 Dec 2005 18:56:37 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH 2/4] msi vector targeting abstractions |
| |
On Wed, Dec 21, 2005 at 12:42:41PM -0600, Mark Maule wrote: > Abstract portions of the MSI core for platforms that do not use standard > APIC interrupt controllers. This is implemented through a set of callouts > which default to current behavior, but which can be overridden by calling > msi_register_callouts() in the platform msi init code.
we tend to calls these _ops or _operations instead of _callouts. Also I'd suggest to not keep the generic ones where they are but in a separate file and let the existing plattforms calls msi_register() with the ops table for those. This keeps the interface symmetric instead of favouring the first implementation.
> @@ -89,10 +91,25 @@ > } > > #ifdef CONFIG_SMP > +static void msi_target_generic(unsigned int vector, > + unsigned int dest_cpu, > + uint32_t *address_hi, /* in/out */ > + uint32_t *address_lo) /* in/out */
Please try to use u32 instead of uint32_t everywhere. Dito for other sizes and signed types.
> +{ > + struct msg_address address; > + > + address.lo_address.value = *address_lo; > + address.lo_address.value &= MSI_ADDRESS_DEST_ID_MASK; > + address.lo_address.value |= > + (cpu_physical_id(dest_cpu) << MSI_TARGET_CPU_SHIFT); > + > + *address_lo = address.lo_address.value; > +}
Why do we need the full struct msg_address here? What about just:
static void msi_target_apic(unsigned int vector, unsigned int dest_cpu, u32 *address_hi, u32 *address_lo) { u32 addr = *address_lo;
addr &= MSI_ADDRESS_DEST_ID_MASK; addr |= (cpu_physical_id(dest_cpu) << MSI_TARGET_CPU_SHIFT);
*address_lo = addr; }
> + (*msi_callouts.msi_teardown)(vector); > +
just msi_ops.teardown(vector);
> +union msg_data { > + struct { > #if defined(__LITTLE_ENDIAN_BITFIELD) > - __u32 vector : 8; > - __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */ > - __u32 reserved_1 : 3; > - __u32 level : 1; /* 0: deassert | 1: assert */ > - __u32 trigger : 1; /* 0: edge | 1: level */ > - __u32 reserved_2 : 16; > + __u32 vector : 8; > + __u32 delivery_mode : 3; /* 000b: FIXED */ > + /* 001b: lowest prior */ > + __u32 reserved_1 : 3; > + __u32 level : 1; /* 0: deassert | 1: assert */ > + __u32 trigger : 1; /* 0: edge | 1: level */ > + __u32 reserved_2 : 16; > #elif defined(__BIG_ENDIAN_BITFIELD) > - __u32 reserved_2 : 16; > - __u32 trigger : 1; /* 0: edge | 1: level */ > - __u32 level : 1; /* 0: deassert | 1: assert */ > - __u32 reserved_1 : 3; > - __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */ > - __u32 vector : 8; > + __u32 reserved_2 : 16; > + __u32 trigger : 1; /* 0: edge | 1: level */ > + __u32 level : 1; /* 0: deassert | 1: assert */ > + __u32 reserved_1 : 3; > + __u32 delivery_mode : 3; /* 000b: FIXED */ > + /* 001b: lowest prior */ > + __u32 vector : 8; > #else > #error "Bitfield endianness not defined! Check your byteorder.h" > #endif > + }u; > + __u32 value; > } __attribute__ ((packed));
While you're cleaning things up, could you please kill the horrible abuse of bitfields for H/W structures and do proper masking instead.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |