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
