Re: [PATCH] net: ethernet: 8390: Add HAS_IOPORT dependency for mcf8390

[Niklas Schnelle <schnelle@linux.ibm.com>]

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<---