On Wednesday 16 February 2011, Guan Xuetao wrote:
> +{
> + PCICFG_ADDR = CONFIG_CMD(bus, devfn, where);
> + switch (size) {
> + case 1:
> + *value = (PCICFG_DATA >> ((where & 3) * 8)) & 0xFF;
> + break;
> + case 2:
It took me a while to figure out what this actually does. PCICFG_ADDR
and PCICFG_DATA are pointers to MMIO registers, which you should not
simply dereference. A lot of things can go wrong there, especially
if future machines move to weakly ordered I/O subsystem or have
multiple CPUs, but even for the simple case, the compiler has
a lot of ways to mess this up.
As explained in my reply to the "hardware registers" patch, all these
pointers should be marked as __iomem, so that sparse warns about
dangerous accesses such as the one here.
The code above should be written as
{
writel(CONFIG_CMD(bus, devfn, where), PCICFG_ADDR);
switch (size) {
case 1:
*value = readl(PCICFG_DATA >> ((where & 3) * 8)) & 0xFF;
break;
case 2:
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
