Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Matthew Wilcox <matthew@wil.cx>]

On Sat, Nov 08, 2008 at 01:28:22AM -0700, Grant Grundler wrote:
> On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote:
> > msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> > and that function already does follow writel() by readl() in the MSI-X case.
> 
> Yes - this part is correct.
> If you care to split this into seperate parts, please add:
>    Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> for this chunk.

It's certainly true (I'd noticed it too).  What I hadn't decided was
whether to take out the readl() or take out the msix_flush_writes().
I'm conflicted.

> > Also, isn't the single use of multi_msi_capable() broken (in the event that
> > the Multiple Message Capable field was 5, the shift would be undefined,
> > on x86 in particular would yield 1 as the result, where 0 would be needed),
> > and the subsequent twiddling of temp needlessly complicated (subtracting
> > one should be sufficient here).
> 
> Comment on this chunk below.
> 
> > And isn't multi_msi_enable(), though unused (since msi_{en,dis}able() are
> > unused), broken altogether (shifting num right by 1 instead of taking the
> > binary log)?

Yes, it is, but I delete it in my patch series that enables support for
multiple MSI.

> > @@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
> >  		pci_read_config_dword(dev,
> >  			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
> >  			&maskbits);
> > -		temp = (1 << multi_msi_capable(control));
> > -		temp = ((temp - 1) & ~temp);
> > -		maskbits |= temp;
> > +		temp = 1U << (multi_msi_capable(control) - 1);
> > +		maskbits |= (temp << 1) - 1;
> 
> Isn't this the new code the same as:
>     maskbits |= (1U << multi_msi_capable(control)) - 1;

You're missing the << 32 possibility.  This all gets a bit confusing
since the number of bits has to be a power of two.  So what we're
actually looking at doing is expanding a number between 0 and 5
(inclusive) to a bitmask.  I gave up and used a lookup table ... can't
find that patch right now, but it was something like:

unsigned int msi_mask[] = {
	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
};

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."