Since commit 6f043e757445 ("asm-generic/io.h: Remove I/O port accessors
for HAS_IOPORT=n") the I/O port accessors are compile-time optional. As
m68k may or may not select HAS_IOPORT the COLDFIRE dependency is not
enough to guarantee I/O port access. Add an explicit HAS_IOPORT
dependency for mcf8390 to prevent a build failure as seen by the kernel
test robot.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202412080511.ORVinTDs-lkp@intel.com/
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/net/ethernet/8390/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
index 345f250781c6d9c3c6cbe5445250dc5987803b1a..f2ee99532187d133fdb02bc4b82c7fc4861f90af 100644
--- a/drivers/net/ethernet/8390/Kconfig
+++ b/drivers/net/ethernet/8390/Kconfig
@@ -87,7 +87,7 @@ config MAC8390
 
 config MCF8390
 	tristate "ColdFire NS8390 based Ethernet support"
-	depends on COLDFIRE
+	depends on COLDFIRE && HAS_IOPORT
 	select CRC32
 	help
 	  This driver is for Ethernet devices using an NS8390-compatible

---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241209-mcf8390_has_ioport-7dcb85a5170c

Best regards,
-- 
Niklas Schnelle

On Mon, Dec 9, 2024, at 11:28, Niklas Schnelle wrote:
> Since commit 6f043e757445 ("asm-generic/io.h: Remove I/O port accessors
> for HAS_IOPORT=n") the I/O port accessors are compile-time optional. As
> m68k may or may not select HAS_IOPORT the COLDFIRE dependency is not
> enough to guarantee I/O port access. Add an explicit HAS_IOPORT
> dependency for mcf8390 to prevent a build failure as seen by the kernel
> test robot.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202412080511.ORVinTDs-lkp@intel.com/
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---

Hi Niklas,

I think your patch is correct in the sense that the I/O port
handling on m68k coldfire is only defined for the PCI bus
the port operations is this driver have nowhere to go when
PCI is disabled.

However, I suspect what you actually found is a different
preexisting bug, likely introduced in the addition of PCI
support in commits 927c28c252dc ("m68k: setup PCI support
code in io_no.h") and d97cf70af097 ("m68k: use asm-generic/io.h
for non-MMU io access functions").

As far as I can tell, the driver predates this patch and
presumably relied on inb/outb getting redirected to readb/writeb,
using the port number as a pointer (without the 
((void __iomem *) PCI_IO_PA) offset).

Note that the dev->base_addr that gets passed into inb()/outb()
is a physical address from a IORESOURCE_MEM resource,
which is normally different from both 16-bit I/O port numbers
and from virtual __iomem pointers, though on coldfire nommu
the three traditionally could be used interchangeably.

Adding Greg Ungerer to Cc, as he maintains the coldfire
platform and wrote the driver.

>  drivers/net/ethernet/8390/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/8390/Kconfig 
> b/drivers/net/ethernet/8390/Kconfig
> index 
> 345f250781c6d9c3c6cbe5445250dc5987803b1a..f2ee99532187d133fdb02bc4b82c7fc4861f90af 
> 100644
> --- a/drivers/net/ethernet/8390/Kconfig
> +++ b/drivers/net/ethernet/8390/Kconfig
> @@ -87,7 +87,7 @@ config MAC8390
> 
>  config MCF8390
>  	tristate "ColdFire NS8390 based Ethernet support"
> -	depends on COLDFIRE
> +	depends on COLDFIRE && HAS_IOPORT
>  	select CRC32
>  	help
>  	  This driver is for Ethernet devices using an NS8390-compatible
>
> ---

If I read this right, we would need something like the
patch below, to actually pass the address through ioremap()
and use readb/writeb on __iomem pointers.

      Arnd

diff --git a/drivers/net/ethernet/8390/mcf8390.c b/drivers/net/ethernet/8390/mcf8390.c
index 94ff8364cdf0..0e2a93e9ef59 100644
--- a/drivers/net/ethernet/8390/mcf8390.c
+++ b/drivers/net/ethernet/8390/mcf8390.c
@@ -44,19 +44,19 @@ static const char version[] =
  * Note that the data port accesses are treated a little differently, and
  * always accessed via the insX/outsX functions.
  */
-static inline u32 NE_PTR(u32 addr)
+static inline void __iomem *NE_PTR(void __iomem *addr)
 {
-	if (addr & 1)
+	if ((uintptr_t)addr & 1)
 		return addr - 1 + NE2000_ODDOFFSET;
 	return addr;
 }
 
-static inline u32 NE_DATA_PTR(u32 addr)
+static inline void __iomem *NE_DATA_PTR(void __iomem *addr)
 {
 	return addr;
 }
 
-void ei_outb(u32 val, u32 addr)
+void ei_outb(u32 val, void __iomem *addr)
 {
 	NE2000_BYTE *rp;
 
@@ -65,7 +65,7 @@ void ei_outb(u32 val, u32 addr)
 }
 
 #define	ei_inb	ei_inb
-u8 ei_inb(u32 addr)
+u8 ei_inb(void __iomem *addr)
 {
 	NE2000_BYTE *rp, val;
 
@@ -74,7 +74,7 @@ u8 ei_inb(u32 addr)
 	return (u8) (RSWAP(val) & 0xff);
 }
 
-void ei_insb(u32 addr, void *vbuf, int len)
+void ei_insb(void __iomem *addr, void *vbuf, int len)
 {
 	NE2000_BYTE *rp, val;
 	u8 *buf;
@@ -87,7 +87,7 @@ void ei_insb(u32 addr, void *vbuf, int len)
 	}
 }
 
-void ei_insw(u32 addr, void *vbuf, int len)
+void ei_insw(void __iomem *addr, void *vbuf, int len)
 {
 	volatile u16 *rp;
 	u16 w, *buf;
@@ -100,7 +100,7 @@ void ei_insw(u32 addr, void *vbuf, int len)
 	}
 }
 
-void ei_outsb(u32 addr, const void *vbuf, int len)
+void ei_outsb(void __iomem *addr, const void *vbuf, int len)
 {
 	NE2000_BYTE *rp, val;
 	u8 *buf;
@@ -113,7 +113,7 @@ void ei_outsb(u32 addr, const void *vbuf, int len)
 	}
 }
 
-void ei_outsw(u32 addr, const void *vbuf, int len)
+void ei_outsw(void __iomem *addr, const void *vbuf, int len)
 {
 	volatile u16 *rp;
 	u16 w, *buf;
@@ -128,12 +128,12 @@ void ei_outsw(u32 addr, const void *vbuf, int len)
 
 #else /* !NE2000_ODDOFFSET */
 
-#define	ei_inb		inb
-#define	ei_outb		outb
-#define	ei_insb		insb
-#define	ei_insw		insw
-#define	ei_outsb	outsb
-#define	ei_outsw	outsw
+#define	ei_inb		readb
+#define	ei_outb		writeb
+#define	ei_insb		readsb
+#define	ei_insw		readsw
+#define	ei_outsb	writesb
+#define	ei_outsw	writesw
 
 #endif /* !NE2000_ODDOFFSET */
 
@@ -149,7 +149,7 @@ void ei_outsw(u32 addr, const void *vbuf, int len)
 static void mcf8390_reset_8390(struct net_device *dev)
 {
 	unsigned long reset_start_time = jiffies;
-	u32 addr = dev->base_addr;
+	void __iomem *addr = ei_local->mem;
 	struct ei_device *ei_local = netdev_priv(dev);
 
 	netif_dbg(ei_local, hw, dev, "resetting the 8390 t=%ld...\n", jiffies);
@@ -190,7 +190,7 @@ static void mcf8390_get_8390_hdr(struct net_device *dev,
 				 struct e8390_pkt_hdr *hdr, int ring_page)
 {
 	struct ei_device *ei_local = netdev_priv(dev);
-	u32 addr = dev->base_addr;
+	void __iomem *addr = ei_local->mem;
 
 	if (ei_local->dmaing) {
 		mcf8390_dmaing_err(__func__, dev, ei_local);
@@ -225,7 +225,7 @@ static void mcf8390_block_input(struct net_device *dev, int count,
 				struct sk_buff *skb, int ring_offset)
 {
 	struct ei_device *ei_local = netdev_priv(dev);
-	u32 addr = dev->base_addr;
+	void __iomem *addr = ei_local->mem;
 	char *buf = skb->data;
 
 	if (ei_local->dmaing) {
@@ -255,7 +255,7 @@ static void mcf8390_block_output(struct net_device *dev, int count,
 				 const int start_page)
 {
 	struct ei_device *ei_local = netdev_priv(dev);
-	u32 addr = dev->base_addr;
+	void __iomem *addr = ei_local->mem;
 	unsigned long dma_start;
 
 	/* Make sure we transfer all bytes if 16bit IO writes */
@@ -318,7 +318,7 @@ static int mcf8390_init(struct net_device *dev)
 	};
 	struct ei_device *ei_local = netdev_priv(dev);
 	unsigned char SA_prom[32];
-	u32 addr = dev->base_addr;
+	void __iomem *addr = ei_local->mem;
 	int start_page, stop_page;
 	int i, ret;
 
@@ -403,7 +403,8 @@ static int mcf8390_init(struct net_device *dev)
 static int mcf8390_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
-	struct resource *mem;
+	struct ei_device *ei_local;
+	void __iomem *mem;
 	resource_size_t msize;
 	int ret, irq;
 
@@ -411,15 +412,11 @@ static int mcf8390_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENXIO;
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (mem == NULL) {
-		dev_err(&pdev->dev, "no memory address specified?\n");
+	mem = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mem)) {
+		dev_err(&pdev->dev, "failed to map resource\n");
 		return -ENXIO;
 	}
-	msize = resource_size(mem);
-	if (!request_mem_region(mem->start, msize, pdev->name))
-		return -EBUSY;
-
 	dev = ____alloc_ei_netdev(0);
 	if (dev == NULL) {
 		release_mem_region(mem->start, msize);
@@ -430,25 +427,20 @@ static int mcf8390_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, dev);
 
 	dev->irq = irq;
-	dev->base_addr = mem->start;
+	ei_local->mem = mem;
 
 	ret = mcf8390_init(dev);
-	if (ret) {
-		release_mem_region(mem->start, msize);
+	if (ret)
 		free_netdev(dev);
-		return ret;
-	}
-	return 0;
+
+	return ret;
 }
 
 static void mcf8390_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
-	struct resource *mem;
 
 	unregister_netdev(dev);
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(mem->start, resource_size(mem));
 	free_netdev(dev);
 }

On Mon, 2024-12-09 at 12:49 +0100, Arnd Bergmann wrote:
> On Mon, Dec 9, 2024, at 11:28, Niklas Schnelle wrote:
> > Since commit 6f043e757445 ("asm-generic/io.h: Remove I/O port accessors
> > for HAS_IOPORT=n") the I/O port accessors are compile-time optional. As
> > m68k may or may not select HAS_IOPORT the COLDFIRE dependency is not
> > enough to guarantee I/O port access. Add an explicit HAS_IOPORT
> > dependency for mcf8390 to prevent a build failure as seen by the kernel
> > test robot.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: 
> > https://lore.kernel.org/oe-kbuild-all/202412080511.ORVinTDs-lkp@intel.com/
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> 
> Hi Niklas,
> 
> I think your patch is correct in the sense that the I/O port
> handling on m68k coldfire is only defined for the PCI bus
> the port operations is this driver have nowhere to go when
> PCI is disabled.
> 
> However, I suspect what you actually found is a different
> preexisting bug, likely introduced in the addition of PCI
> support in commits 927c28c252dc ("m68k: setup PCI support
> code in io_no.h") and d97cf70af097 ("m68k: use asm-generic/io.h
> for non-MMU io access functions").
> 
> As far as I can tell, the driver predates this patch and
> presumably relied on inb/outb getting redirected to readb/writeb,
> using the port number as a pointer (without the 
> ((void __iomem *) PCI_IO_PA) offset).
> 
> Note that the dev->base_addr that gets passed into inb()/outb()
> is a physical address from a IORESOURCE_MEM resource,
> which is normally different from both 16-bit I/O port numbers
> and from virtual __iomem pointers, though on coldfire nommu
> the three traditionally could be used interchangeably.
> 
> Adding Greg Ungerer to Cc, as he maintains the coldfire
> platform and wrote the driver.

Thanks for taking a closer look. Note that I was a bit hasty in sending
this and forgot the "PATCH net" suffix so resent[0] with that just
before your reply and discussing it here might not be seen by all the
netdev folks. I did get a checkpatch warning that this driver is
depreated too. But hey, seems like our HAS_IOPORT patches have a talent
for uncovering old bugs.

[0]
https://lore.kernel.org/netdev/3ca55478-a5a9-442b-ae4f-a0a822f786d9@app.fastmail.com/T/#t

> 
> >  drivers/net/ethernet/8390/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/8390/Kconfig 
> > b/drivers/net/ethernet/8390/Kconfig
> > index 
> > 345f250781c6d9c3c6cbe5445250dc5987803b1a..f2ee99532187d133fdb02bc4b82c7fc4861f90af 
> > 100644
> > --- a/drivers/net/ethernet/8390/Kconfig
> > +++ b/drivers/net/ethernet/8390/Kconfig
> > @@ -87,7 +87,7 @@ config MAC8390
> > 
> >  config MCF8390
> >  	tristate "ColdFire NS8390 based Ethernet support"
> > -	depends on COLDFIRE
> > +	depends on COLDFIRE && HAS_IOPORT
> >  	select CRC32
> >  	help
> >  	  This driver is for Ethernet devices using an NS8390-compatible
> > 
> > ---
---8<---