| Message ID | 20201104154858.1247725-7-andrew@lunn.ch |
|---|---|
| State | New |
| Headers | show |
| Series | smsc W=1 warning fixes | expand |
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.
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)
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
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)
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
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
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(-)