Right, that makes sense. However, most drivers use the field to store the
physical address, not the iomap token. Maybe there should be a new field
in struct sja1000_priv for the virtual address, but that would be a change
to the base driver, not just to the OF portion.
Is that common practice? If yes, I will add a member to store the
virtual address to struct sja1000_priv.
I grepped through the network driver for usage of ->base_addr, and
it's somewhat inconsistent. The majority of the users use it for
a physical address, but there are also a few that use it for the
__iomem token.
Casts between unsigned long and qualified (__iomem, __user, const, ...)
pointers do not cause a warning, but can easily lead to bugs when
another user casts to an unqualified pointer.
It's such a baroque thing, there is no reason to set it at all if you
ask me. It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.
It's such a baroque thing, there is no reason to set it at all if you
ask me. It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.
My original comment was about the fact that sja1000 was doing
dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't
even think about SIOCGIFMAP and command line overrides, but that
surely makes it worse and the driver should be changed to
store the virtual register address in its private data structure.
drivers/net/fec.c seems to have the same problem, which manifests
in a number of ugly casts and direct pointer dereferences in places
where it should do writel() or out_be32().
Arnd <><
On Tue, May 26, 2009 at 10:42:05AM +0100, Arnd Bergmann wrote:
On Tuesday 26 May 2009, David Miller wrote:
quoted
It's such a baroque thing, there is no reason to set it at all if you
ask me. It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.
My original comment was about the fact that sja1000 was doing
dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't
even think about SIOCGIFMAP and command line overrides, but that
surely makes it worse and the driver should be changed to
store the virtual register address in its private data structure.
drivers/net/fec.c seems to have the same problem, which manifests
in a number of ugly casts and direct pointer dereferences in places
where it should do writel() or out_be32().
Ack. I'll prepare a patch for fec.c. Internally the driver already uses
a void __iomem * and writel/readl in -next. There is only one usage
left.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Right, that makes sense. However, most drivers use the field to store the
physical address, not the iomap token. Maybe there should be a new field
in struct sja1000_priv for the virtual address, but that would be a change
to the base driver, not just to the OF portion.
Is that common practice? If yes, I will add a member to store the
virtual address to struct sja1000_priv.
I grepped through the network driver for usage of ->base_addr, and
it's somewhat inconsistent. The majority of the users use it for
a physical address, but there are also a few that use it for the
__iomem token.
Casts between unsigned long and qualified (__iomem, __user, const, ...)
pointers do not cause a warning, but can easily lead to bugs when
another user casts to an unqualified pointer.
It's such a baroque thing, there is no reason to set it at all if you
ask me. It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.
OK, I see, there are good reasons not to (mis-)use dev->base_addr. I
will prepare a patch for the SJA1000 CAN drivers.
Wolfgang.
Right, that makes sense. However, most drivers use the field to store the
physical address, not the iomap token. Maybe there should be a new field
in struct sja1000_priv for the virtual address, but that would be a change
to the base driver, not just to the OF portion.
Is that common practice? If yes, I will add a member to store the
virtual address to struct sja1000_priv.
I grepped through the network driver for usage of ->base_addr, and
it's somewhat inconsistent. The majority of the users use it for
a physical address, but there are also a few that use it for the
__iomem token.
Casts between unsigned long and qualified (__iomem, __user, const, ...)
pointers do not cause a warning, but can easily lead to bugs when
another user casts to an unqualified pointer.
It's such a baroque thing, there is no reason to set it at all if you
ask me. It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.
OK, I see, there are good reasons not to (mis-)use dev->base_addr. I
will prepare a patch for the SJA1000 CAN drivers.
I have just sent out a patch series fixing this issue and providing
a revised patch for the SJA1000 OF platform driver:
[net-next-2.6 PATCH 0/3] can: sja1000: misused netdev->base_addr and OF platform driver
Wolfgang.