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

[Lukasz Stelmach]

It was <2020-12-15 wto 17:46>, when Jakub Kicinski wrote:
> 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.
>

Removed.

>>>> +	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.
>

Removed.

>>> 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.

Indeed, a struct of three u16 is 6 bytes long. I was worried it may be
rounded up to 8. Removed.

>>> 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.

I see, that makes sense, indeed.

So, the only thing that's left is pskb_expand_head(). I need to wrap my
head around it. Can you tell me how a cloned skb is different and why
there may be separate branch for it?

Thank you.
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachment: signature.asc
Description: PGP signature