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@xxxxxxxxxxxxxxxx>
>
> 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."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
