diff mbox series

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

Message ID 20201104154858.1247725-7-andrew@lunn.ch
State Changes Requested
Delegated to: David Miller
Headers show
Series smsc W=1 warning fixes | expand

Checks

Context Check Description
jkicinski/stable success Stable not CCed
jkicinski/header_inline success Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
jkicinski/verify_fixes success Link
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/module_param success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/subject_prefix success Link
jkicinski/tree_selection success Clearly marked for net-next
jkicinski/patch_count success Link
jkicinski/fixes_present success Link
jkicinski/cover_letter success Link

Commit Message

Andrew Lunn Nov. 4, 2020, 3:48 p.m. UTC
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(-)

Comments

Jakub Kicinski Nov. 5, 2020, 10:47 p.m. UTC | #1
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.
David Laight Nov. 6, 2020, 8:44 a.m. UTC | #2
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)
Nicolas Pitre Nov. 6, 2020, 3:05 p.m. UTC | #3
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
David Laight Nov. 6, 2020, 3:29 p.m. UTC | #4
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)
Nicolas Pitre Nov. 6, 2020, 3:47 p.m. UTC | #5
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 mbox series

Patch

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