Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Jakub Kicinski]

On Wed, 16 Dec 2020 01:42:03 +0100 Lukasz Stelmach wrote:
> >> +	ax_local->stats.rx_packets++;
> >> +	ax_local->stats.rx_bytes += skb->len;
> >> +	skb->dev = ndev;
> >> +
> >> +	skb->truesize = skb->len + sizeof(struct sk_buff);  
> >
> > Why do you modify truesize?
> >  
> 
> I don't know. Although uncommon, this appears in a few usb drivers, so I
> didn't think much about it when I ported this code.

I'd guess they do aggregation. I wouldn't touch it in your driver.

>> Since you always punt to a workqueue did you consider just using
>> threaded interrupts instead?   
> 
> Yes, and I have decided to stay with the workqueue. Interrupt
> processing is not the only task performed in the workqueue. There is
> also trasmission to the hardware, which may be quite slow (remember, it
> is SPI), so it's better decoupled from syscalls

I see, and since the device can't do RX and TX simultaneously (IIRC),
that makes sense.

> >> +	u8			plat_endian;
> >> +		#define PLAT_LITTLE_ENDIAN	0
> >> +		#define PLAT_BIG_ENDIAN		1  
> >
> > Why do you store this little nugget of information?
> >  
> 
> I don't know*. The hardware enables endianness detection by providing a
> constant value (0x1234) in one of its registers. Unfortunately I don't
> have a big-endian board with this chip to check if it is necessary to
> alter AX_READ/AX_WRITE in any way.

Yeah, may be hard to tell what magic the device is doing.
I was mostly saying that you don't seem to use this information,
so the member of the struct can be removed IIRC.

> > These all look like multiple of 2 bytes. Why do they need to be packed?
> >  
> 
> These are structures sent to and returned from the hardware. They are
> prepended and appended to the network packets. I think it is good to
> keep them packed, so compilers won't try any tricks.

Compilers can't play tricks on memory layout of structures, the
standard is pretty strict about that. Otherwise ABIs would never work.
We prefer not to unnecessarily pack structures in the neworking code,
because it generates byte by byte loads on architectures which can't 
do unaligned accesses.

> > No need to return some specific pattern on failure? Like 0xffff?
> >  
> 
> All registers are 16 bit wide. I am afraid it isn't safe to assume that
> there is a 16 bit value we could use. Chances that SPI goes south are
> pretty slim. And if it does, there isn't much more than reporting an
> error we can do about it anyway.
> 
> One thing I can think of is to change axspi_* to (s32), return -1,
> somehow (how?) shutdown the device in AX_*.

I'm mostly concerned about potentially random data left over in the
buffer. Seems like it could lead to hard to repro bugs. Hence the
suggestion to return a constant of your choosing on error, doesn't
really matter what as long as it's a known constant.