On Sat, Nov 08, 2008 at 07:10:22AM -0700, Matthew Wilcox wrote:
> 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.
Good, I'm not. :)
I'd much rather remove msix_flush_writes(). It's clearer to me to have the
readl() next to the writel() than to bury the readl() a function call later.
...
> > > + 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.
Thanks - You are right. I expected (1U<<32) to be zero (arithmetic shift left)
instead of 1 ("Rotate Left" aka "logical shift left"). Code below produced:
5 32 0x1 0x0
when using "1U << k". Here's the test code:
grundler@mb500:~$ cat test_shift.c
#include <stdio.h>
main()
{
unsigned int i;
for (i=0;i<6; i++) {
unsigned int k = 1U << i ;
unsigned int j = 1ULL << k ;
printf("%d %d 0x%x 0x%x\n", i, k, j, j - 1);
}
return 0;
}
grundler@mb500:~$ ./test_shift
0 1 0x2 0x1
1 2 0x4 0x3
2 4 0x10 0xf
3 8 0x100 0xff
4 16 0x10000 0xffff
5 32 0x0 0xffffffff
Using "1ULL" (64-bit constant) is enough to get the result I was
expecting.... note I didn't declare j as "unsigned long long j"
because gcc didn't warn about assigning "1ULL < k" (64-bit) to an
unsigned int (32-bit). gcc bug?
I think the kernel should declare maskbits as 64-bit and
use "1ULL" to just keep things obvious.
> 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
> };
The code, when written correctly, seems compact and simple enough.
thanks,
grant
--
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
