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

[Nicolas Pitre <nico@fluxnic.net>]

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