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. Regards, Andy > 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
