Andy Walls wrote: > On Mon, 2010-02-22 at 18:45 -0300, Mauro Carvalho Chehab wrote: >> Andy Walls wrote: >>> Mauro, >>> >>> Please pull from http://linuxtv.org/hg/~awalls/ivtv-changes >>> >>> for the following 4 changesets: >>> >>> 01/04: ivtv: Fix ivtv_api_get_data() to avoid unneeded IO during IRQ handling >>> http://linuxtv.org/hg/~awalls/ivtv-changes?cmd=changeset;node=4090ec224ef2 >> Hmm: >> >> WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt >> #89: FILE: drivers/media/video/ivtv/ivtv-mailbox.c:375: >> + volatile u32 __iomem *p = mbdata->mbox[mb].data; > > :) > > Take out the "volatile" and you get a gcc compiler warning, because the > type of the mbdata->mbox variable is "volatile". > > struct ivtv_mailbox_data { > volatile struct ivtv_mailbox __iomem *mbox; > [...] > }; > > void ivtv_api_get_data(struct ivtv_mailbox_data *mbdata, int mb, > int argc, u32 data[]) > { > volatile u32 __iomem *p = mbdata->mbox[mb].data; > int i; > for (i = 0; i < argc; i++, p++) > data[i] = readl(p); > } > > > Using > > data[i] = readl(p); > > in the code above is a little more efficient than > > data[i] = readl(mbdata->mbox[mb].data[i]); > > So if you *really* don't like the checkpatch.pl warning, I could change > the code to do that. > > However doing that would be somewhat backwards. The checkpatch.pl > warning is for people who do not understand that "volatile" only > supresses optimizations and that it doesn't provide locking. I don't > need locking here, and readl() is going to supress optimization anyway > due to its use of "volatile" on its input argument: > > static inline u32 __raw_readl(const volatile void __iomem *addr) > { > return *(const volatile u32 __force *) addr; > } > > #define readl(addr) __le32_to_cpu(__raw_readl(addr)) > > > So the "volatile" warning from checkpatch.pl is a false positive in this > case. > >> Is the driver missing some locks? > > Nope. And "volatile" would not help with a locking problem anyway. > I'll apply the patch, but I think you should later remove the volatile from struct ivtv_mailbox_data & friends. You might need some memory barriers around the places this var is used (f. example, by using smp_wmb). -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
