On Monday 20 June 2011 20:48:49 Russell King - ARM Linux wrote: > On Mon, Jun 20, 2011 at 01:35:35PM -0400, Alan Stern wrote: > > According to Arnd, any remaining possible issues will be addressed by > > changing the implementation of readl/writel on ARM. It doesn't look as > > though the ehci files need anything else done. > > I'm not about to change their implementation because they've proven > themselves over the last 10 years to be perfectly fine, and changing > them has a habbit of causing GCC to play less optimally than it should > do. Well, we do know that gcc now makes different tradeoffs, and that it's entirely within the C99 specification when it's generating byte accesses from __raw_readl(). The case where the pointer is __packed is just the obvious case where it would do that, and I fully agree that the __packed in that case is a bug, but I'm much in favor of writing code so that we instruct the compiler to create correct code rather than giving it the choice between correct and incorrect. > I've seen drivers where GCC reloads the base address from the driver > private data structure each time a register access is performed, rather > than caching the base address in a register. I've seen it issuing > separate add instructions and using a zero pre-index load/store. The > existing way is the only way I've found to get GCC to come anywhere > close to producing "optimal" code for the IO accessors. Two points here: * What's the olders compiler that we really need to be able to build efficient kernels? Would you consider it if we can show that gcc-4.2 and higher produce code that is as good as the existing macros? How about making the code gcc version dependent? * We already need a compiler barrier in the non-_relaxed() versions of the I/O accessors, which will force a reload of the base address in a lot of cases, so the code is already suboptimal. Yes, we don't have the barrier today without CONFIG_ARM_DMA_MEM_BUFFERABLE, but that is a bug, because it lets the compiler move accesses to DMA buffers around readl/writel. > If it is the case that these structures do not require packing to get > their desired layout, then they don't require packing, and the packed > attribute should be dropped. Yes. But are you going to audit every other use of __packed in the kernel to check if it is used on __iomem pointers? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
