[PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
l.stelmach at samsung.com
Wed Dec 16 07:21:52 EST 2020
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.
>>>> + 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.
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?
Samsung R&D Institute Poland
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 487 bytes
Desc: not available
More information about the linux-arm-kernel