* Re: [PATCH] net: ethernet: 8390: Add HAS_IOPORT dependency for mcf8390
2024-12-09 10:28 [PATCH] net: ethernet: 8390: Add HAS_IOPORT dependency for mcf8390 Niklas Schnelle
@ 2024-12-09 11:49 ` Arnd Bergmann
2024-12-09 11:59 ` Niklas Schnelle
0 siblings, 1 reply; 3+ messages in thread
From: Arnd Bergmann @ 2024-12-09 11:49 UTC (permalink / raw)
To: Niklas Schnelle, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Greg Ungerer
Cc: Netdev, linux-kernel, kernel test robot
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);
}
^ permalink raw reply related [flat|nested] 3+ messages in thread