Fixes a bunch of these warnings on arm allmodconfig:
In file included from /build/drivers/net/phy/mdio-cavium.c:11:
/build/drivers/net/phy/mdio-cavium.c: In function 'cavium_mdiobus_set_mode':
/build/drivers/net/phy/mdio-cavium.h:114:37: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
114 | #define oct_mdio_readq(addr) readq((void *)addr)
| ^
/build/drivers/net/phy/mdio-cavium.c:21:16: note: in expansion of macro 'oct_mdio_readq'
21 | smi_clk.u64 = oct_mdio_readq(p->register_base + SMI_CLK);
| ^~~~~~~~~~~~~~
Fixes: 171a9bae68c7 ("staging/octeon: Allow test build on !MIPS")
Signed-off-by: Olof Johansson <[email protected]>
---
drivers/net/phy/mdio-cavium.h | 14 +++++++-------
drivers/net/phy/mdio-octeon.c | 5 ++---
drivers/net/phy/mdio-thunder.c | 2 +-
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/mdio-cavium.h b/drivers/net/phy/mdio-cavium.h
index b7f89ad27465f..1cf81f0bc585f 100644
--- a/drivers/net/phy/mdio-cavium.h
+++ b/drivers/net/phy/mdio-cavium.h
@@ -90,7 +90,7 @@ union cvmx_smix_wr_dat {
struct cavium_mdiobus {
struct mii_bus *mii_bus;
- u64 register_base;
+ void __iomem *register_base;
enum cavium_mdiobus_mode mode;
};
@@ -98,20 +98,20 @@ struct cavium_mdiobus {
#include <asm/octeon/octeon.h>
-static inline void oct_mdio_writeq(u64 val, u64 addr)
+static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
{
- cvmx_write_csr(addr, val);
+ cvmx_write_csr((u64)addr, val);
}
-static inline u64 oct_mdio_readq(u64 addr)
+static inline u64 oct_mdio_readq(void __iomem *addr)
{
- return cvmx_read_csr(addr);
+ return cvmx_read_csr((u64)addr);
}
#else
#include <linux/io-64-nonatomic-lo-hi.h>
-#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
-#define oct_mdio_readq(addr) readq((void *)addr)
+#define oct_mdio_writeq(val, addr) writeq(val, addr)
+#define oct_mdio_readq(addr) readq(addr)
#endif
int cavium_mdiobus_read(struct mii_bus *bus, int phy_id, int regnum);
diff --git a/drivers/net/phy/mdio-octeon.c b/drivers/net/phy/mdio-octeon.c
index 8327382aa5689..c58ab8acd485a 100644
--- a/drivers/net/phy/mdio-octeon.c
+++ b/drivers/net/phy/mdio-octeon.c
@@ -44,8 +44,7 @@ static int octeon_mdiobus_probe(struct platform_device *pdev)
return -ENXIO;
}
- bus->register_base =
- (u64)devm_ioremap(&pdev->dev, mdio_phys, regsize);
+ bus->register_base = devm_ioremap(&pdev->dev, mdio_phys, regsize);
if (!bus->register_base) {
dev_err(&pdev->dev, "dev_ioremap failed\n");
return -ENOMEM;
@@ -56,7 +55,7 @@ static int octeon_mdiobus_probe(struct platform_device *pdev)
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
bus->mii_bus->name = KBUILD_MODNAME;
- snprintf(bus->mii_bus->id, MII_BUS_ID_SIZE, "%llx", bus->register_base);
+ snprintf(bus->mii_bus->id, MII_BUS_ID_SIZE, "%p", bus->register_base);
bus->mii_bus->parent = &pdev->dev;
bus->mii_bus->read = cavium_mdiobus_read;
diff --git a/drivers/net/phy/mdio-thunder.c b/drivers/net/phy/mdio-thunder.c
index b6128ae7f14f3..280cf84d4116e 100644
--- a/drivers/net/phy/mdio-thunder.c
+++ b/drivers/net/phy/mdio-thunder.c
@@ -84,7 +84,7 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
nexus->buses[i] = bus;
i++;
- bus->register_base = (u64)nexus->bar0 +
+ bus->register_base = nexus->bar0 +
r.start - pci_resource_start(pdev, 0);
smi_en.u64 = 0;
--
2.11.0
On Sun, Nov 10, 2019 at 04:42:11PM -0800, Olof Johansson wrote:
> Fixes a bunch of these warnings on arm allmodconfig:
>
> In file included from /build/drivers/net/phy/mdio-cavium.c:11:
> /build/drivers/net/phy/mdio-cavium.c: In function 'cavium_mdiobus_set_mode':
> /build/drivers/net/phy/mdio-cavium.h:114:37: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 114 | #define oct_mdio_readq(addr) readq((void *)addr)
> | ^
> /build/drivers/net/phy/mdio-cavium.c:21:16: note: in expansion of macro 'oct_mdio_readq'
> 21 | smi_clk.u64 = oct_mdio_readq(p->register_base + SMI_CLK);
> | ^~~~~~~~~~~~~~
>
> Fixes: 171a9bae68c7 ("staging/octeon: Allow test build on !MIPS")
> Signed-off-by: Olof Johansson <[email protected]>
> ---
> drivers/net/phy/mdio-cavium.h | 14 +++++++-------
> drivers/net/phy/mdio-octeon.c | 5 ++---
> drivers/net/phy/mdio-thunder.c | 2 +-
> 3 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/mdio-cavium.h b/drivers/net/phy/mdio-cavium.h
> index b7f89ad27465f..1cf81f0bc585f 100644
> --- a/drivers/net/phy/mdio-cavium.h
> +++ b/drivers/net/phy/mdio-cavium.h
> @@ -90,7 +90,7 @@ union cvmx_smix_wr_dat {
>
> struct cavium_mdiobus {
> struct mii_bus *mii_bus;
> - u64 register_base;
> + void __iomem *register_base;
> enum cavium_mdiobus_mode mode;
> };
>
> @@ -98,20 +98,20 @@ struct cavium_mdiobus {
>
> #include <asm/octeon/octeon.h>
>
> -static inline void oct_mdio_writeq(u64 val, u64 addr)
> +static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
> {
> - cvmx_write_csr(addr, val);
> + cvmx_write_csr((u64)addr, val);
> }
Hi Olof
Humm. The warning goes away, but is it really any better?
Did you try also changing the stub function in
drivers/staging/octeon/octeon-stubs.h so it takes void __iomem? Or
did that cause a lot more warnings from other places?
Thanks
Andrew
On Sun, Nov 10, 2019 at 6:32 PM Andrew Lunn <[email protected]> wrote:
>
> On Sun, Nov 10, 2019 at 04:42:11PM -0800, Olof Johansson wrote:
> > Fixes a bunch of these warnings on arm allmodconfig:
> >
> > In file included from /build/drivers/net/phy/mdio-cavium.c:11:
> > /build/drivers/net/phy/mdio-cavium.c: In function 'cavium_mdiobus_set_mode':
> > /build/drivers/net/phy/mdio-cavium.h:114:37: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> > 114 | #define oct_mdio_readq(addr) readq((void *)addr)
> > | ^
> > /build/drivers/net/phy/mdio-cavium.c:21:16: note: in expansion of macro 'oct_mdio_readq'
> > 21 | smi_clk.u64 = oct_mdio_readq(p->register_base + SMI_CLK);
> > | ^~~~~~~~~~~~~~
> >
> > Fixes: 171a9bae68c7 ("staging/octeon: Allow test build on !MIPS")
> > Signed-off-by: Olof Johansson <[email protected]>
> > ---
> > drivers/net/phy/mdio-cavium.h | 14 +++++++-------
> > drivers/net/phy/mdio-octeon.c | 5 ++---
> > drivers/net/phy/mdio-thunder.c | 2 +-
> > 3 files changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/phy/mdio-cavium.h b/drivers/net/phy/mdio-cavium.h
> > index b7f89ad27465f..1cf81f0bc585f 100644
> > --- a/drivers/net/phy/mdio-cavium.h
> > +++ b/drivers/net/phy/mdio-cavium.h
> > @@ -90,7 +90,7 @@ union cvmx_smix_wr_dat {
> >
> > struct cavium_mdiobus {
> > struct mii_bus *mii_bus;
> > - u64 register_base;
> > + void __iomem *register_base;
> > enum cavium_mdiobus_mode mode;
> > };
> >
> > @@ -98,20 +98,20 @@ struct cavium_mdiobus {
> >
> > #include <asm/octeon/octeon.h>
> >
> > -static inline void oct_mdio_writeq(u64 val, u64 addr)
> > +static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
> > {
> > - cvmx_write_csr(addr, val);
> > + cvmx_write_csr((u64)addr, val);
> > }
>
> Hi Olof
>
> Humm. The warning goes away, but is it really any better?
>
> Did you try also changing the stub function in
> drivers/staging/octeon/octeon-stubs.h so it takes void __iomem? Or
> did that cause a lot more warnings from other places?
That percolates through a bunch of MIPS code that I didn't feel like
getting into. So indeed, I stopped at that point.
-Olof
From: Olof Johansson <[email protected]>
Date: Sun, 10 Nov 2019 16:42:11 -0800
> -static inline void oct_mdio_writeq(u64 val, u64 addr)
> +static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
> {
> - cvmx_write_csr(addr, val);
> + cvmx_write_csr((u64)addr, val);
> }
I hate stuff like this, I think you really need to fix this from the bottom
up or similar. MMIO and such addresses are __iomem pointers, period.
On Mon, Nov 11, 2019 at 09:46:58PM -0800, David Miller wrote:
> From: Olof Johansson <[email protected]>
> Date: Sun, 10 Nov 2019 16:42:11 -0800
>
> > -static inline void oct_mdio_writeq(u64 val, u64 addr)
> > +static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
> > {
> > - cvmx_write_csr(addr, val);
> > + cvmx_write_csr((u64)addr, val);
> > }
>
> I hate stuff like this, I think you really need to fix this from the bottom
> up or similar. MMIO and such addresses are __iomem pointers, period.
Yes, i agree, but did not want to push the work to Olof. The point of
COMPILE_TEST is to find issues like this, code which should be
architecture independent, but is not. The cast just papers over the
cracks.
At a minimum, could we fix the stub cvmx_write_csr() used for
everything !MIPS. That should hopefully fix everything !MIPS, but
cause MIPS to start issuing warning. The MIPS folks can then cleanup
their code, which is really what is broken here.
Andrew
On Tue, Nov 12, 2019 at 5:23 AM Andrew Lunn <[email protected]> wrote:
>
> On Mon, Nov 11, 2019 at 09:46:58PM -0800, David Miller wrote:
> > From: Olof Johansson <[email protected]>
> > Date: Sun, 10 Nov 2019 16:42:11 -0800
> >
> > > -static inline void oct_mdio_writeq(u64 val, u64 addr)
> > > +static inline void oct_mdio_writeq(u64 val, void __iomem *addr)
> > > {
> > > - cvmx_write_csr(addr, val);
> > > + cvmx_write_csr((u64)addr, val);
> > > }
> >
> > I hate stuff like this, I think you really need to fix this from the bottom
> > up or similar. MMIO and such addresses are __iomem pointers, period.
>
> Yes, i agree, but did not want to push the work to Olof. The point of
> COMPILE_TEST is to find issues like this, code which should be
> architecture independent, but is not. The cast just papers over the
> cracks.
>
> At a minimum, could we fix the stub cvmx_write_csr() used for
> everything !MIPS. That should hopefully fix everything !MIPS, but
> cause MIPS to start issuing warning. The MIPS folks can then cleanup
> their code, which is really what is broken here.
I'm not disagreeing with Dave, going all the way down the rabbit hole
is preferred. In this case I mostly pushed the lack of __iomem usage
down one layer but not the whole way.
I'm unlikely to find time to do it in the near future myself (this was
a bit of a weekend drive-by from my side), but I don't mind doing it.
If someone else beats me to it, feel free to take it over.
-Olof