Re: [PATCH 2/4] msi vector targeting abstractions

[Mark Maule <maule@sgi.com>]

On Wed, Dec 21, 2005 at 06:56:37PM +0000, Christoph Hellwig wrote:
> 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.

ok.

> 
> > @@ -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.

ok.

> 
> > +{
> > +	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;
> }

Right.


> 
> > +	(*msi_callouts.msi_teardown)(vector);
> > +
> 
> just
> 	msi_ops.teardown(vector);
> 

ok.