On Sat, Nov 08, 2008 at 11:37:45AM -0700, Grant Grundler wrote:
> 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.
That's certainly a respectable opinion. It's one I hold myself. Have
you considered another opinion that if you're trying to disable a
thousand or more interrupts, doing writel/readl() a thousand times is
going to be less efficient than doing writel() a thousand times and
readl() once? We don't really have a need to do that today, but it's a
possible direction we might want to move in the future.
> 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:
Er, rotate left is not logical shift left. Also, shifting by >= the number of
bits in the element is undefined by the C spec. Which is why Jan broke
it into two shifts.
> 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;
> }
This will give different results on different processors, and possibly
even with different compilers. It is entitled to emit nasal daemons.
> 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?
Not a bug. That kind of truncation isn't generally warned about.
Consider:
void foo(char *x, int bar)
{
x[0] = bar >> 24;
x[1] = bar >> 16;
x[2] = bar >> 8;
x[3] = bar;
}
> I think the kernel should declare maskbits as 64-bit and
> use "1ULL" to just keep things obvious.
I suspec that's rather inefficient.
> > unsigned int msi_mask[] = {
> > 1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
> > };
>
> The code, when written correctly, seems compact and simple enough.
I'm not sure I agree.
This is all rather moot since multiple MSI isn't supported, and my
patchset deletes the function in question anyway.
--
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
