[PATCH net-next v2 0/7] smsc W=1 warning fixes

[PATCH net-next v2 0/7] smsc W=1 warning fixes

[Andrew Lunn]

Fixup various W=1 warnings, and then add COMPILE_TEST support, which
explains why these where missed on the previous pass.

One of these patches added as a new checkpatch warning, by
copy/pasting an existing macro and slightly modifying it. The code
already has lots of checkpatch warnings, so one more is not going to
make that much difference.

v2:
Use while (0)
Rework buffer alignment to make it clearer

Andrew Lunn (7):
  drivers: net: smc91x: Fix set but unused W=1 warning
  drivers: net: smc91x: Fix missing kerneldoc reported by W=1
  drivers: net: smc911x: Work around set but unused status
  drivers: net: smc911x: Fix set but unused status because of DBG macro
  drivers: net: smc911x: Fix passing wrong number of parameters to DBG()
    macro
  drivers: net: smc911x: Fix cast from pointer to integer of different
    size
  drivers: net: smsc: Add COMPILE_TEST support

 drivers/net/ethernet/smsc/Kconfig   |  6 +++---
 drivers/net/ethernet/smsc/smc911x.c | 19 +++++++++++--------
 drivers/net/ethernet/smsc/smc91x.c  | 10 ++++++++--
 drivers/net/ethernet/smsc/smc91x.h  | 10 ++++++++++
 4 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.28.0

[PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but unused W=1 warning

[Andrew Lunn]

drivers/net/ethernet/smsc/smc91x.c:706:51: warning: variable ‘pkt_len’ set but not used [-Wunused-but-set-variable]
  706 |  unsigned int saved_packet, packet_no, tx_status, pkt_len;

Add a new macro for getting fields out of the header, which only gets
the status, not the length which in this case is not needed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc91x.c |  4 ++--
 drivers/net/ethernet/smsc/smc91x.h | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
index f6b73afd1879..61333939a73e 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -703,7 +703,7 @@ static void smc_tx(struct net_device *dev)
 {
 	struct smc_local *lp = netdev_priv(dev);
 	void __iomem *ioaddr = lp->base;
-	unsigned int saved_packet, packet_no, tx_status, pkt_len;
+	unsigned int saved_packet, packet_no, tx_status;
 
 	DBG(3, dev, "%s\n", __func__);
 
@@ -720,7 +720,7 @@ static void smc_tx(struct net_device *dev)
 
 	/* read the first word (status word) from this packet */
 	SMC_SET_PTR(lp, PTR_AUTOINC | PTR_READ);
-	SMC_GET_PKT_HDR(lp, tx_status, pkt_len);
+	SMC_GET_PKT_HDR_STATUS(lp, tx_status);
 	DBG(2, dev, "TX STATUS 0x%04x PNR 0x%02x\n",
 	    tx_status, packet_no);
 
diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h
index 387539a8094b..705d9d6ebaa5 100644
--- a/drivers/net/ethernet/smsc/smc91x.h
+++ b/drivers/net/ethernet/smsc/smc91x.h
@@ -1056,6 +1056,16 @@ static const char * chip_ids[ 16 ] =  {
 		}							\
 	} while (0)
 
+#define SMC_GET_PKT_HDR_STATUS(lp, status)				\
+	do {								\
+		if (SMC_32BIT(lp)) {					\
+			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
+			(status) = __val & 0xffff;			\
+		} else {						\
+			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
+		}							\
+	} while (0)
+
 #define SMC_PUSH_DATA(lp, p, l)					\
 	do {								\
 		if (SMC_32BIT(lp)) {				\
-- 
2.28.0

[PATCH net-next v2 2/7] drivers: net: smc91x: Fix missing kerneldoc reported by W=1

[Andrew Lunn]

drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'dev' not described in 'try_toggle_control_gpio'
drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'desc' not described in 'try_toggle_control_gpio'
drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'name' not described in 'try_toggle_control_gpio'
drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'index' not described in 'try_toggle_control_gpio'
drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'value' not described in 'try_toggle_control_gpio'
drivers/net/ethernet/smsc/smc91x.c:2199: warning: Function parameter or member 'nsdelay' not described in 'try_toggle_control_gpio'

Document these parameters.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc91x.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
index 61333939a73e..83dde7db0851 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2191,6 +2191,12 @@ MODULE_DEVICE_TABLE(of, smc91x_match);
 
 /**
  * of_try_set_control_gpio - configure a gpio if it exists
+ * @dev: net device
+ * @desc: where to store the GPIO descriptor, if it exists
+ * @name: name of the GPIO in DT
+ * @index: index of the GPIO in DT
+ * @value: set the GPIO to this value
+ * @nsdelay: delay before setting the GPIO
  */
 static int try_toggle_control_gpio(struct device *dev,
 				   struct gpio_desc **desc,
-- 
2.28.0

[PATCH net-next v2 3/7] drivers: net: smc911x: Work around set but unused status

[Andrew Lunn]

drivers/net/ethernet/smsc/smc911x.c: In function ‘smc911x_phy_interrupt’:
drivers/net/ethernet/smsc/smc911x.c:976:6: warning: variable ‘status’ set but not used [-Wunused-but-set-variable]
  976 |  int status;

A comment indicates the status needs to be read from the PHY,
otherwise bad things happen. But due to the macro magic, it is hard to
perform the read without assigning it to a variable. So add
_always_unused attribute to status to tell the compiler we don't
expect to use the value.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc911x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index 01069dfaf75c..8f748a0c057e 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -879,7 +879,7 @@ static void smc911x_phy_configure(struct work_struct *work)
 	int phyaddr = lp->mii.phy_id;
 	int my_phy_caps; /* My PHY capabilities */
 	int my_ad_caps; /* My Advertised capabilities */
-	int status;
+	int status __always_unused;
 	unsigned long flags;
 
 	DBG(SMC_DEBUG_FUNC, dev, "--> %s()\n", __func__);
@@ -973,7 +973,7 @@ static void smc911x_phy_interrupt(struct net_device *dev)
 {
 	struct smc911x_local *lp = netdev_priv(dev);
 	int phyaddr = lp->mii.phy_id;
-	int status;
+	int status __always_unused;
 
 	DBG(SMC_DEBUG_FUNC, dev, "--> %s\n", __func__);
 
-- 
2.28.0

[PATCH net-next v2 4/7] drivers: net: smc911x: Fix set but unused status because of DBG macro

[Andrew Lunn]

drivers/net/ethernet/smsc/smc911x.c: In function ‘smc911x_timeout’:
drivers/net/ethernet/smsc/smc911x.c:1251:6: warning: variable ‘status’ set but not used [-Wunused-but-set-variable]
 1251 |  int status, mask;

The status is read in order to print it via the DBG macro. However,
due to the way DBG is disabled, the compiler never sees it being used.

Change the DBG macro to actually make use of the passed parameters,
and the leave the optimiser to remove the unwanted code inside the
while (0).

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc911x.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index 8f748a0c057e..6978050496ac 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -102,7 +102,10 @@ MODULE_ALIAS("platform:smc911x");
 
 #define PRINTK(dev, args...)   netdev_info(dev, args)
 #else
-#define DBG(n, dev, args...)   do { } while (0)
+#define DBG(n, dev, args...)			 \
+	while (0) {				 \
+		netdev_dbg(dev, args);		 \
+	}
 #define PRINTK(dev, args...)   netdev_dbg(dev, args)
 #endif
 
-- 
2.28.0

[PATCH net-next v2 5/7] drivers: net: smc911x: Fix passing wrong number of parameters to DBG() macro

[Andrew Lunn]

Now that the compiler always sees the parameters passed to the DBG()
macro, it gives an error message about wrong parameters. The comment
says it all:

	/* ndev is not valid yet, so avoid passing it in. */
	DBG(SMC_DEBUG_FUNC, "--> %s\n",  __func__);

You cannot not just pass a parameter!

The DBG does not seem to have any real value, to just remove it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc911x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index 6978050496ac..ac1a764364fb 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -2047,8 +2047,6 @@ static int smc911x_drv_probe(struct platform_device *pdev)
 	void __iomem *addr;
 	int ret;
 
-	/* ndev is not valid yet, so avoid passing it in. */
-	DBG(SMC_DEBUG_FUNC, "--> %s\n",  __func__);
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		ret = -ENODEV;
-- 
2.28.0

[PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[Andrew Lunn]

drivers/net/ethernet/smsc/smc911x.c: In function ‘smc911x_hardware_send_pkt’:
drivers/net/ethernet/smsc/smc911x.c:471:11: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  471 |  cmdA = (((u32)skb->data & 0x3) << 16) |

When built on 64bit targets, the skb->data pointer cannot be cast to a
u32 in a meaningful way. Use an temporary variable and cast to
unsigned long.

Suggested-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/smc911x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index ac1a764364fb..5b0041996f1f 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -448,6 +448,7 @@ static void smc911x_hardware_send_pkt(struct net_device *dev)
 	struct sk_buff *skb;
 	unsigned int cmdA, cmdB, len;
 	unsigned char *buf;
+	int offset;
 
 	DBG(SMC_DEBUG_FUNC | SMC_DEBUG_TX, dev, "--> %s\n", __func__);
 	BUG_ON(lp->pending_tx_skb == NULL);
@@ -465,9 +466,10 @@ static void smc911x_hardware_send_pkt(struct net_device *dev)
 			TX_CMD_A_INT_FIRST_SEG_ | TX_CMD_A_INT_LAST_SEG_ |
 			skb->len;
 #else
-	buf = (char*)((u32)skb->data & ~0x3);
-	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
-	cmdA = (((u32)skb->data & 0x3) << 16) |
+	offset = (unsigned long)skb->data & 3;
+	buf = skb->data - offset;
+	len = skb->len + offset;
+	cmdA = (offset << 16) |
 			TX_CMD_A_INT_FIRST_SEG_ | TX_CMD_A_INT_LAST_SEG_ |
 			skb->len;
 #endif
-- 
2.28.0

[PATCH net-next v2 7/7] drivers: net: smsc: Add COMPILE_TEST support

[Andrew Lunn]

Improve the build testing of these SMSC drivers by enabling them when
COMPILE_TEST is selected.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/smsc/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/smsc/Kconfig b/drivers/net/ethernet/smsc/Kconfig
index b77e427e6729..c52a38df0e0d 100644
--- a/drivers/net/ethernet/smsc/Kconfig
+++ b/drivers/net/ethernet/smsc/Kconfig
@@ -8,7 +8,7 @@ config NET_VENDOR_SMSC
 	default y
 	depends on ARM || ARM64 || ATARI_ETHERNAT || COLDFIRE || \
 		   ISA || MAC || MIPS || NIOS2 || PCI || \
-		   PCMCIA || SUPERH || XTENSA || H8300
+		   PCMCIA || SUPERH || XTENSA || H8300 || COMPILE_TEST
 	help
 	  If you have a network (Ethernet) card belonging to this class, say Y.
 
@@ -39,7 +39,7 @@ config SMC91X
 	select MII
 	depends on !OF || GPIOLIB
 	depends on ARM || ARM64 || ATARI_ETHERNAT || COLDFIRE || \
-		   MIPS || NIOS2 || SUPERH || XTENSA || H8300
+		   MIPS || NIOS2 || SUPERH || XTENSA || H8300 || COMPILE_TEST
 	help
 	  This is a driver for SMC's 91x series of Ethernet chipsets,
 	  including the SMC91C94 and the SMC91C111. Say Y if you want it
@@ -78,7 +78,7 @@ config SMC911X
 	tristate "SMSC LAN911[5678] support"
 	select CRC32
 	select MII
-	depends on (ARM || SUPERH)
+	depends on (ARM || SUPERH || COMPILE_TEST)
 	help
 	  This is a driver for SMSC's LAN911x series of Ethernet chipsets
 	  including the new LAN9115, LAN9116, LAN9117, and LAN9118.
-- 
2.28.0

Re: [PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but unused W=1 warning

[Jakub Kicinski]

On Wed,  4 Nov 2020 16:48:52 +0100 Andrew Lunn wrote:
> drivers/net/ethernet/smsc/smc91x.c:706:51: warning: variable ‘pkt_len’ set but not used [-Wunused-but-set-variable]
>   706 |  unsigned int saved_packet, packet_no, tx_status, pkt_len;
> 
> Add a new macro for getting fields out of the header, which only gets
> the status, not the length which in this case is not needed.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Sorry I missed something on v1

> +#define SMC_GET_PKT_HDR_STATUS(lp, status)				\
> +	do {								\
> +		if (SMC_32BIT(lp)) {					\
> +			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> +			(status) = __val & 0xffff;			\
> +		} else {						\
> +			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
> +		}							\
> +	} while (0)

This is the original/full macro:

#define SMC_GET_PKT_HDR(lp, status, length)				\
	do {								\
		if (SMC_32BIT(lp)) {				\
			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
			(status) = __val & 0xffff;			\
			(length) = __val >> 16;				\
		} else {						\
			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
			(length) = SMC_inw(ioaddr, DATA_REG(lp));	\
		}							\
	} while (0)

Note that it reads the same address twice in the else branch.

I'm 90% sure we can't remove the read here either so best treat it
like the ones in patch 3, right?

Re: [PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[Jakub Kicinski]

On Wed,  4 Nov 2020 16:48:57 +0100 Andrew Lunn wrote:
> -	buf = (char*)((u32)skb->data & ~0x3);
> -	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
> -	cmdA = (((u32)skb->data & 0x3) << 16) |
> +	offset = (unsigned long)skb->data & 3;
> +	buf = skb->data - offset;
> +	len = skb->len + offset;
> +	cmdA = (offset << 16) |

The len calculation is wrong, you trusted people on the mailing list 
too much ;)

This is better:

-	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
+	len = round_up(skb->len, 4);

You can also do PTR_ALIGN_DOWN() for the skb->data if you want.
And give the same treatment to the other side of the #if statement.

RE: [PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[David Laight]

From: Jakub Kicinski
> Sent: 05 November 2020 22:47
> 
> On Wed,  4 Nov 2020 16:48:57 +0100 Andrew Lunn wrote:
> > -	buf = (char*)((u32)skb->data & ~0x3);
> > -	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
> > -	cmdA = (((u32)skb->data & 0x3) << 16) |
> > +	offset = (unsigned long)skb->data & 3;
> > +	buf = skb->data - offset;
> > +	len = skb->len + offset;
> > +	cmdA = (offset << 16) |
> 
> The len calculation is wrong, you trusted people on the mailing list
> too much ;)

I misread what the comment-free convoluted code was doing....

Clearly it is rounding the base address down to a multiple of 4
and passing the offset in cmdA.
This is fine - but quite why the (I assume) hardware doesn't do
this itself and just document that it does a 32bit read is
another matter - the logic will be much the same and I doubt
anything modern is that pushed for space.

However rounding the length up to a multiple of 4 is buggy.
If this is an ethernet chipset it must (honest) be able to
send frames that don't end on a 4 byte boundary.
So rounding up the length is very dubious.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but unused W=1 warning

[David Laight]

From: Jakub Kicinski
> Sent: 05 November 2020 22:38
> On Wed,  4 Nov 2020 16:48:52 +0100 Andrew Lunn wrote:
> > drivers/net/ethernet/smsc/smc91x.c:706:51: warning: variable ‘pkt_len’ set but not used [-Wunused-
> but-set-variable]
> >   706 |  unsigned int saved_packet, packet_no, tx_status, pkt_len;
> >
> > Add a new macro for getting fields out of the header, which only gets
> > the status, not the length which in this case is not needed.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Sorry I missed something on v1
> 
> > +#define SMC_GET_PKT_HDR_STATUS(lp, status)				\
> > +	do {								\
> > +		if (SMC_32BIT(lp)) {					\
> > +			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> > +			(status) = __val & 0xffff;			\
> > +		} else {						\
> > +			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
> > +		}							\
> > +	} while (0)
> 
> This is the original/full macro:
> 
> #define SMC_GET_PKT_HDR(lp, status, length)				\
> 	do {								\
> 		if (SMC_32BIT(lp)) {				\
> 			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> 			(status) = __val & 0xffff;			\
> 			(length) = __val >> 16;				\
> 		} else {						\
> 			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
> 			(length) = SMC_inw(ioaddr, DATA_REG(lp));	\
> 		}							\
> 	} while (0)
> 
> Note that it reads the same address twice in the else branch.
> 
> I'm 90% sure we can't remove the read here either so best treat it
> like the ones in patch 3, right?

One of the two SMC_inw() needs to use 'ioaddr + 2'.
Probably the one for (length).

The code may also be buggy on BE systems.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[Nicolas Pitre]

On Fri, 6 Nov 2020, David Laight wrote:

> From: Jakub Kicinski
> > Sent: 05 November 2020 22:47
> > 
> > On Wed,  4 Nov 2020 16:48:57 +0100 Andrew Lunn wrote:
> > > -	buf = (char*)((u32)skb->data & ~0x3);
> > > -	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
> > > -	cmdA = (((u32)skb->data & 0x3) << 16) |
> > > +	offset = (unsigned long)skb->data & 3;
> > > +	buf = skb->data - offset;
> > > +	len = skb->len + offset;
> > > +	cmdA = (offset << 16) |
> > 
> > The len calculation is wrong, you trusted people on the mailing list
> > too much ;)
> 
> I misread what the comment-free convoluted code was doing....
> 
> Clearly it is rounding the base address down to a multiple of 4
> and passing the offset in cmdA.
> This is fine - but quite why the (I assume) hardware doesn't do
> this itself and just document that it does a 32bit read is
> another matter - the logic will be much the same and I doubt
> anything modern is that pushed for space.
> 
> However rounding the length up to a multiple of 4 is buggy.
> If this is an ethernet chipset it must (honest) be able to
> send frames that don't end on a 4 byte boundary.
> So rounding up the length is very dubious.

I probably wrote that code. Probably something like 20 years ago at this 
point. I no longer have access to the actual hardware either.

But my recollection is that this ethernet chip had the ability to do 1, 
2 or 4 byte wide data transfers.

To be able to efficiently use I/O helpers like readsl()/writesl() on 
ARM, the host memory pointer had to be aligned to a 32-bit boundary 
because misaligned accesses were not supported by the hardware and 
therefore were very costly to perform in software with a bytewise 
approach. Remember that back then, the CPU clock was very close to the 
actual ethernet throughput and PIO was the only option.

This was made even worse by the fact that, on some boards, the hw 
designers didn't consider connecting the byte select signals as a 
worthwhile thing to do. That means only 32-bit wide access to the chip 
were possible.

So to work around this, the skb buffer address was rounded down, the 
length was rounded up, and 
the on-chip pointer was adjusted to refer to the actual data 
payload accordingly with the original length. Therefore the proposed 
patch is indeed wrong.

Just to say that, although the code might look suspicious, there was a 
reason for that and it did work correctly for a long long time at this 
point. Obviously those were only 32- bit systems (I really doubt those 
ethernet chips were ever used on 64-bit systems).


Nicolas

RE: [PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[David Laight]

From: Nicolas Pitre
> Sent: 06 November 2020 15:06
> 
> On Fri, 6 Nov 2020, David Laight wrote:
> 
> > From: Jakub Kicinski
> > > Sent: 05 November 2020 22:47
> > >
> > > On Wed,  4 Nov 2020 16:48:57 +0100 Andrew Lunn wrote:
> > > > -	buf = (char*)((u32)skb->data & ~0x3);
> > > > -	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
> > > > -	cmdA = (((u32)skb->data & 0x3) << 16) |
> > > > +	offset = (unsigned long)skb->data & 3;
> > > > +	buf = skb->data - offset;
> > > > +	len = skb->len + offset;
> > > > +	cmdA = (offset << 16) |
> > >
> > > The len calculation is wrong, you trusted people on the mailing list
> > > too much ;)
> >
> > I misread what the comment-free convoluted code was doing....
> >
> > Clearly it is rounding the base address down to a multiple of 4
> > and passing the offset in cmdA.
> > This is fine - but quite why the (I assume) hardware doesn't do
> > this itself and just document that it does a 32bit read is
> > another matter - the logic will be much the same and I doubt
> > anything modern is that pushed for space.
> >
> > However rounding the length up to a multiple of 4 is buggy.
> > If this is an ethernet chipset it must (honest) be able to
> > send frames that don't end on a 4 byte boundary.
> > So rounding up the length is very dubious.
> 
> I probably wrote that code. Probably something like 20 years ago at this
> point. I no longer have access to the actual hardware either.
> 
> But my recollection is that this ethernet chip had the ability to do 1,
> 2 or 4 byte wide data transfers.
> 
> To be able to efficiently use I/O helpers like readsl()/writesl() on
> ARM, the host memory pointer had to be aligned to a 32-bit boundary
> because misaligned accesses were not supported by the hardware and
> therefore were very costly to perform in software with a bytewise
> approach. Remember that back then, the CPU clock was very close to the
> actual ethernet throughput and PIO was the only option.
> 
> This was made even worse by the fact that, on some boards, the hw
> designers didn't consider connecting the byte select signals as a
> worthwhile thing to do. That means only 32-bit wide access to the chip
> were possible.
> 
> So to work around this, the skb buffer address was rounded down, the
> length was rounded up, and
> the on-chip pointer was adjusted to refer to the actual data
> payload accordingly with the original length. Therefore the proposed
> patch is indeed wrong.

Which one, the one that rounds the length up.
Or the one that just adds 'initial padding'.

> Just to say that, although the code might look suspicious, there was a
> reason for that and it did work correctly for a long long time at this
> point. Obviously those were only 32- bit systems (I really doubt those
> ethernet chips were ever used on 64-bit systems).

Oh, OK, this is one of the ethernet chips that had an on-chip fifo
that the software had to use PIO to fill.
(I remember them well! Mostly 16bit ISA ones and the odd EISA one.)

So you can 'cheat' at the start of the frame to do aligned 32-bit writes.
(Unless the skb has odd fragmentation - unlikely for IP.)

The end of frame is more problematic if the byte enables are missing.
Depending exactly on how the end of frame is signalled.
If the frame length is passed (which probably needs to include the
initial pad) then it may not matter about extra bytes in the tx fifo.
(Provided they don't end up in the following frame.)

I wonder when this was last used?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH net-next v2 6/7] drivers: net: smc911x: Fix cast from pointer to integer of different size

[Nicolas Pitre]

On Fri, 6 Nov 2020, David Laight wrote:

> From: Nicolas Pitre
> > Sent: 06 November 2020 15:06
> > 
> > On Fri, 6 Nov 2020, David Laight wrote:
> > 
> > > From: Jakub Kicinski
> > > > Sent: 05 November 2020 22:47
> > > >
> > > > On Wed,  4 Nov 2020 16:48:57 +0100 Andrew Lunn wrote:
> > > > > -	buf = (char*)((u32)skb->data & ~0x3);
> > > > > -	len = (skb->len + 3 + ((u32)skb->data & 3)) & ~0x3;
> > > > > -	cmdA = (((u32)skb->data & 0x3) << 16) |
> > > > > +	offset = (unsigned long)skb->data & 3;
> > > > > +	buf = skb->data - offset;
> > > > > +	len = skb->len + offset;
> > > > > +	cmdA = (offset << 16) |
> > > >
> > > > The len calculation is wrong, you trusted people on the mailing list
> > > > too much ;)
> > >
> > > I misread what the comment-free convoluted code was doing....
> > >
> > > Clearly it is rounding the base address down to a multiple of 4
> > > and passing the offset in cmdA.
> > > This is fine - but quite why the (I assume) hardware doesn't do
> > > this itself and just document that it does a 32bit read is
> > > another matter - the logic will be much the same and I doubt
> > > anything modern is that pushed for space.
> > >
> > > However rounding the length up to a multiple of 4 is buggy.
> > > If this is an ethernet chipset it must (honest) be able to
> > > send frames that don't end on a 4 byte boundary.
> > > So rounding up the length is very dubious.
> > 
> > I probably wrote that code. Probably something like 20 years ago at this
> > point. I no longer have access to the actual hardware either.
> > 
> > But my recollection is that this ethernet chip had the ability to do 1,
> > 2 or 4 byte wide data transfers.
> > 
> > To be able to efficiently use I/O helpers like readsl()/writesl() on
> > ARM, the host memory pointer had to be aligned to a 32-bit boundary
> > because misaligned accesses were not supported by the hardware and
> > therefore were very costly to perform in software with a bytewise
> > approach. Remember that back then, the CPU clock was very close to the
> > actual ethernet throughput and PIO was the only option.
> > 
> > This was made even worse by the fact that, on some boards, the hw
> > designers didn't consider connecting the byte select signals as a
> > worthwhile thing to do. That means only 32-bit wide access to the chip
> > were possible.
> > 
> > So to work around this, the skb buffer address was rounded down, the
> > length was rounded up, and
> > the on-chip pointer was adjusted to refer to the actual data
> > payload accordingly with the original length. Therefore the proposed
> > patch is indeed wrong.
> 
> Which one, the one that rounds the length up.
> Or the one that just adds 'initial padding'.

Let's say the hunk quoted above. That's what caught my attention.

My suggestion would be to simply change the u32 cast to uintptr_t to 
quiet warnings and leave it at that.

> > Just to say that, although the code might look suspicious, there was a
> > reason for that and it did work correctly for a long long time at this
> > point. Obviously those were only 32- bit systems (I really doubt those
> > ethernet chips were ever used on 64-bit systems).
> 
> Oh, OK, this is one of the ethernet chips that had an on-chip fifo
> that the software had to use PIO to fill.
> (I remember them well! Mostly 16bit ISA ones and the odd EISA one.)
> 
> So you can 'cheat' at the start of the frame to do aligned 32-bit writes.
> (Unless the skb has odd fragmentation - unlikely for IP.)
> 
> The end of frame is more problematic if the byte enables are missing.
> Depending exactly on how the end of frame is signalled.
> If the frame length is passed (which probably needs to include the
> initial pad) then it may not matter about extra bytes in the tx fifo.

It was more like on-chip SRAM than an actual FIFO. The position within 
that SRAM for the frame to send could be modified which is why having 
2 extra bytes of unrelated data at the beginning didn't matter. And the 
command to send the frame has a byte length, so that takes care of the 
extra trailing bytes too.

> (Provided they don't end up in the following frame.)

No because the data start is adjusted again for the next one.

Trust me, it all worked fine back then, confirmed by multiple tests and 
ethernet dumps.

> I wonder when this was last used?

It was still common in 2005 or so.

This very chip is still being used by a few individuals interested in 
running latest kernels on vintage hardware.


Nicolas

Re: [PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but unused W=1 warning

[Jakub Kicinski]

On Fri, 6 Nov 2020 08:48:47 +0000 David Laight wrote:
> > > +#define SMC_GET_PKT_HDR_STATUS(lp, status)				\
> > > +	do {								\
> > > +		if (SMC_32BIT(lp)) {					\
> > > +			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> > > +			(status) = __val & 0xffff;			\
> > > +		} else {						\
> > > +			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
> > > +		}							\
> > > +	} while (0)  
> > 
> > This is the original/full macro:
> > 
> > #define SMC_GET_PKT_HDR(lp, status, length)				\
> > 	do {								\
> > 		if (SMC_32BIT(lp)) {				\
> > 			unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> > 			(status) = __val & 0xffff;			\
> > 			(length) = __val >> 16;				\
> > 		} else {						\
> > 			(status) = SMC_inw(ioaddr, DATA_REG(lp));	\
> > 			(length) = SMC_inw(ioaddr, DATA_REG(lp));	\
> > 		}							\
> > 	} while (0)
> > 
> > Note that it reads the same address twice in the else branch.
> > 
> > I'm 90% sure we can't remove the read here either so best treat it
> > like the ones in patch 3, right?  
> 
> One of the two SMC_inw() needs to use 'ioaddr + 2'.
> Probably the one for (length).
> 
> The code may also be buggy on BE systems.

More proof that this code is fragile.

Changing IO accesses is not acceptable in a "warning cleanup" patch,
unless it can be tested on real HW.

We can follow up on the issues you see separately, please.