[ofa-general] Re: [RFC/PATCH] mthca: ensure alignment of doorbell writes

[Michael S. Tsirkin]

> Quoting akepner at sgi.com <akepner at sgi.com>:
> Subject: Re: [RFC/PATCH] mthca: ensure alignment of doorbell writes
> 
> On Thu, Jul 26, 2007 at 06:39:46AM +0300, Michael S. Tsirkin wrote:
> 
> > ....
> > These should be getting 'union mthca_doorbell *db' I think.
> > 
> 
> Hi Michael;
> 
> Want to make sure I understand your point. Are you saying, e.g., 
> that the function:
> 
> static inline void mthca_ring_db(union mthca_doorbell db, 
> 				 void __iomem *dest,
>                                  spinlock_t *doorbell_lock)
> 
> should instead have the prototype:
> 
> static inline void mthca_ring_db(union mthca_doorbell* db,
>                                  void __iomem *dest,
>                                  spinlock_t *doorbell_lock)
> 
> ?

Yes.

> If so, I'm not sure I agree. The union mthca_doorbell is 
> 64 bits so can be passed in a register, but passing a pointer 
> requires a few extra operations to calculate the address, 
> and dereference the pointer. But maybe I misunderstand you...

This is really coding style thing.

It's usually not a good idea to pass unions/structures by value.
If union size is later changed to be large, gcc might pass it in
a global data section, which fails to be reentrant.

Try compiling both variants and looking at the code - I expect
there won't be difference.

> Now that I look at this again, the __attribute__ ((aligned...))
> thing on union mthca_doorbell is pretty silly - of course the  
> alignment is going to be sizeof(__be64)....
> 
> +union mthca_doorbell {
> +       __be64 val64;
> +       __be32 val32[2];
> +} __attribute__ ((aligned (sizeof(__be64))));
> +

Right.

-- 
MST