Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Thu, 17 Dec 2020 12:46:57 +0100 Lukasz Stelmach wrote:
> > to the correct values so the stack pre-allocates the needed spaces,
> > when it can.
>
> Yes, I fonud these. However, I am not sure setting needed_tailroom has
> any effect. In many places where alloc_skb() is called needed_headrom
> and hard_header_len are refered to via the LL_RESERVED_SPACE macro. But
> the macro does not refer to needed_tailroom. Once (f5184d267c1a ("net:
> Allow netdevices to specify needed head/tailroom") there was
> LL_ALLOCATED_SPACE macro, but but it was removed in 56c978f1da1f ("net:
> Remove LL_ALLOCATED_SPACE"). And now only some protocols refer to
> needet_tailroom.
Yeah, tailroom is used a lot less often. Only really crappy HW requires
it.
> BTW. What is hard_header_len for? Is it the length of the link layer
> header? Considering "my" hardware requires some headers with each
> packet, I find hard_headr_len name a bit confusing.
Yup, L2 headers, not hardware. Not sure why "hard" was chosen, that must
have happened way back.
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-12-16 śro 08:13>, when Jakub Kicinski wrote:
> On Wed, 16 Dec 2020 13:21:52 +0100 Lukasz Stelmach wrote:
>> 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?
>
> I think this driver needs to prepend and append some info to the packet
> data, right? Cloned skb is sharing the data with another skb, the
> metadata is separate but the packet data is shared, so you can't modify
> the data, you need to do a copy of the data. pskb_expand_head() should
> take care of cloned skbs so you can just call it upfront and it will
> make sure the skb is the right geometry for you.
>
> BTW you should set netdev->needed_headroom and netdev->needed_tailroom
Done.
> to the correct values so the stack pre-allocates the needed spaces,
> when it can.
Yes, I fonud these. However, I am not sure setting needed_tailroom has
any effect. In many places where alloc_skb() is called needed_headrom
and hard_header_len are refered to via the LL_RESERVED_SPACE macro. But
the macro does not refer to needed_tailroom. Once (f5184d267c1a ("net:
Allow netdevices to specify needed head/tailroom") there was
LL_ALLOCATED_SPACE macro, but but it was removed in 56c978f1da1f ("net:
Remove LL_ALLOCATED_SPACE"). And now only some protocols refer to
needet_tailroom.
BTW. What is hard_header_len for? Is it the length of the link layer
header? Considering "my" hardware requires some headers with each
packet, I find hard_headr_len name a bit confusing.
I will be sending v9 in a minute or two.
Kind regards,
--
Łukasz Stelmach
Samsung R Institute Poland
Samsung Electronics
signature.asc
Description: PGP signature
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Wed, 16 Dec 2020 13:21:52 +0100 Lukasz Stelmach wrote: > 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? I think this driver needs to prepend and append some info to the packet data, right? Cloned skb is sharing the data with another skb, the metadata is separate but the packet data is shared, so you can't modify the data, you need to do a copy of the data. pskb_expand_head() should take care of cloned skbs so you can just call it upfront and it will make sure the skb is the right geometry for you. BTW you should set netdev->needed_headroom and netdev->needed_tailroom to the correct values so the stack pre-allocates the needed spaces, when it can.
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
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 0x? >> >> 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 Institute Poland Samsung Electronics signature.asc Description: PGP signature
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
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 0x? > > > > 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.
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-12-04 pią 19:37>, when Jakub Kicinski wrote:
> On Wed, 2 Dec 2020 22:47:09 +0100 Łukasz Stelmach wrote:
>> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be
>> connected to a CPU with a 8/16-bit bus or with an SPI. This driver
>> supports SPI connection.
>>
Before we begin let me emphisize the following sentence
>> The driver has been ported from the vendor kernel for ARTIK5[2]
>> boards.
This means, that there may be parts I am not very confident about. I am
not saying I don't take responsibility for them, far from that. But I am
more then happy, when someone points at them saying they make no
sense. When you ask - Why? And I say - I don't know. That means -
Please, be so kind and help me improve it. OK?
>> Several changes were made to adapt it to the current kernel which
>> include:
>>
>> + updated DT configuration,
>> + clock configuration moved to DT,
>> + new timer, ethtool and gpio APIs,
>> + dev_* instead of pr_* and custom printk() wrappers,
>> + removed awkward vendor power managemtn.
>> + introduced ethtool tunable to control SPI compression
>>
>> [1]
>> https://protect2.fireeye.com/v1/url?k=676ddfd4-38f6e6cb-676c549b-000babdfecba-4b1c0ca737b1deaa=1=0b28bac6-bda4-4cf1-8d38-c5c264b64aca=https%3A%2F%2Fwww.asix.com.tw%2Fproducts.php%3Fop%3DpItemdetail%26PItemID%3D104%3B65%3B86%26PLine%3D65
>> [2]
>> https://protect2.fireeye.com/v1/url?k=4148effe-1ed3d6e1-414964b1-000babdfecba-a257ebdf6f0e18f5=1=0b28bac6-bda4-4cf1-8d38-c5c264b64aca=https%3A%2F%2Fgit.tizen.org%2Fcgit%2Fprofile%2Fcommon%2Fplatform%2Fkernel%2Flinux-3.10-artik%2F
>>
>> The other ax88796 driver is for NE2000 compatible AX88796L chip. These
>> chips are not compatible. Hence, two separate drivers are required.
>>
>> Signed-off-by: Łukasz Stelmach
>> Reviewed-by: Andrew Lunn
>
>> +case ETHTOOL_SPI_COMPRESSION:
>> +if (netif_running(ndev))
>> +return -EBUSY;
>> +if ((*(u32 *)data) != 1)
>> +return -EINVAL;
>> +ax_local->capabilities &= ~AX_CAP_COMP;
>> +ax_local->capabilities |= (*(u32 *)data) == 1 ? AX_CAP_COMP : 0;
>
> Since you're just using a single bit of information please use
> a private driver flag.
>
Done.
>> +headroom = skb_headroom(skb);
>> +tailroom = skb_tailroom(skb);
>> +padlen = ((pkt_len + 3) & 0x7FC) - pkt_len;
>
> round_up(pkt_len, 4) - pkt_len;
>
>> +tol_len = ((pkt_len + 3) & 0x7FC) +
>> +TX_OVERHEAD + TX_EOP_SIZE + spi_len;
>
> Ditto
>
Done.
>> +seq_num = ++ax_local->seq_num & 0x1F;
>> +
>> +info = (struct tx_pkt_info *)skb->cb;
>> +info->pkt_len = pkt_len;
>> +
>> +if ((!skb_cloned(skb)) &&
>> +(headroom >= (TX_OVERHEAD + spi_len)) &&
>> +(tailroom >= (padlen + TX_EOP_SIZE))) {
>
>> +} else {
>
> I think you can just use pskb_expand_head() instead of all this
>
Under construction.
>> +tx_skb = alloc_skb(tol_len, GFP_KERNEL);
>> +if (!tx_skb)
>> +return NULL;
>> +
>> +/* Write SPI TXQ header */
>> +memcpy(skb_put(tx_skb, spi_len), ax88796c_tx_cmd_buf, spi_len);
>> +
>> +info->seq_num = seq_num;
>> +ax88796c_proc_tx_hdr(info, skb->ip_summed);
>> +
>> +/* SOP and SEG header */
>> +memcpy(skb_put(tx_skb, TX_OVERHEAD),
>> + >sop, TX_OVERHEAD);
>> +
>> +/* Packet */
>> +memcpy(skb_put(tx_skb, ((pkt_len + 3) & 0xFFFC)),
>> + skb->data, pkt_len);
>> +
>> +/* EOP header */
>> +memcpy(skb_put(tx_skb, TX_EOP_SIZE),
>> + >eop, TX_EOP_SIZE);
>> +
>> +skb_unlink(skb, q);
>> +dev_kfree_skb(skb);
>> +}
>
>> +static void
>> +ax88796c_skb_return(struct ax88796c_device *ax_local, struct sk_buff *skb,
>> +struct rx_header *rxhdr)
>> +{
>> +struct net_device *ndev = ax_local->ndev;
>> +int status;
>> +
>> +do {
>> +if (!(ndev->features & NETIF_F_RXCSUM))
>> +break;
>> +
>> +/* checksum error bit is set */
>> +if ((rxhdr->flags & RX_HDR3_L3_ERR) ||
>> +(rxhdr->flags & RX_HDR3_L4_ERR))
>> +break;
>> +
>> +/* Other types may be indicated by more than one bit. */
>> +if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) ||
>> +(rxhdr->flags & RX_HDR3_L4_TYPE_UDP))
>> +skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +} while (0);
>> +
>> +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.
>> +skb->protocol = eth_type_trans(skb, ax_local->ndev);
>> +
Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Wed, 2 Dec 2020 22:47:09 +0100 Łukasz Stelmach wrote:
> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be
> connected to a CPU with a 8/16-bit bus or with an SPI. This driver
> supports SPI connection.
>
> The driver has been ported from the vendor kernel for ARTIK5[2]
> boards. Several changes were made to adapt it to the current kernel
> which include:
>
> + updated DT configuration,
> + clock configuration moved to DT,
> + new timer, ethtool and gpio APIs,
> + dev_* instead of pr_* and custom printk() wrappers,
> + removed awkward vendor power managemtn.
> + introduced ethtool tunable to control SPI compression
>
> [1]
> https://www.asix.com.tw/products.php?op=pItemdetail=104;65;86=65
> [2]
> https://git.tizen.org/cgit/profile/common/platform/kernel/linux-3.10-artik/
>
> The other ax88796 driver is for NE2000 compatible AX88796L chip. These
> chips are not compatible. Hence, two separate drivers are required.
>
> Signed-off-by: Łukasz Stelmach
> Reviewed-by: Andrew Lunn
> + case ETHTOOL_SPI_COMPRESSION:
> + if (netif_running(ndev))
> + return -EBUSY;
> + if ((*(u32 *)data) != 1)
> + return -EINVAL;
> + ax_local->capabilities &= ~AX_CAP_COMP;
> + ax_local->capabilities |= (*(u32 *)data) == 1 ? AX_CAP_COMP : 0;
Since you're just using a single bit of information please use
a private driver flag.
> + headroom = skb_headroom(skb);
> + tailroom = skb_tailroom(skb);
> + padlen = ((pkt_len + 3) & 0x7FC) - pkt_len;
round_up(pkt_len, 4) - pkt_len;
> + tol_len = ((pkt_len + 3) & 0x7FC) +
> + TX_OVERHEAD + TX_EOP_SIZE + spi_len;
Ditto
> + seq_num = ++ax_local->seq_num & 0x1F;
> +
> + info = (struct tx_pkt_info *)skb->cb;
> + info->pkt_len = pkt_len;
> +
> + if ((!skb_cloned(skb)) &&
> + (headroom >= (TX_OVERHEAD + spi_len)) &&
> + (tailroom >= (padlen + TX_EOP_SIZE))) {
> + } else {
I think you can just use pskb_expand_head() instead of all this
> + tx_skb = alloc_skb(tol_len, GFP_KERNEL);
> + if (!tx_skb)
> + return NULL;
> +
> + /* Write SPI TXQ header */
> + memcpy(skb_put(tx_skb, spi_len), ax88796c_tx_cmd_buf, spi_len);
> +
> + info->seq_num = seq_num;
> + ax88796c_proc_tx_hdr(info, skb->ip_summed);
> +
> + /* SOP and SEG header */
> + memcpy(skb_put(tx_skb, TX_OVERHEAD),
> +>sop, TX_OVERHEAD);
> +
> + /* Packet */
> + memcpy(skb_put(tx_skb, ((pkt_len + 3) & 0xFFFC)),
> +skb->data, pkt_len);
> +
> + /* EOP header */
> + memcpy(skb_put(tx_skb, TX_EOP_SIZE),
> +>eop, TX_EOP_SIZE);
> +
> + skb_unlink(skb, q);
> + dev_kfree_skb(skb);
> + }
> +static void
> +ax88796c_skb_return(struct ax88796c_device *ax_local, struct sk_buff *skb,
> + struct rx_header *rxhdr)
> +{
> + struct net_device *ndev = ax_local->ndev;
> + int status;
> +
> + do {
> + if (!(ndev->features & NETIF_F_RXCSUM))
> + break;
> +
> + /* checksum error bit is set */
> + if ((rxhdr->flags & RX_HDR3_L3_ERR) ||
> + (rxhdr->flags & RX_HDR3_L4_ERR))
> + break;
> +
> + /* Other types may be indicated by more than one bit. */
> + if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) ||
> + (rxhdr->flags & RX_HDR3_L4_TYPE_UDP))
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + } while (0);
> +
> + 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?
> + skb->protocol = eth_type_trans(skb, ax_local->ndev);
> +
> + netif_info(ax_local, rx_status, ndev, "< rx, len %zu, type 0x%x\n",
> +skb->len + sizeof(struct ethhdr), skb->protocol);
> +
> + status = netif_rx_ni(skb);
> + if (status != NET_RX_SUCCESS)
> + netif_info(ax_local, rx_err, ndev,
> +"netif_rx status %d\n", status);
> +}
> +
> +static void
> +ax88796c_rx_fixup(struct ax88796c_device *ax_local, struct sk_buff *rx_skb)
> +{
> + struct rx_header *rxhdr = (struct rx_header *)rx_skb->data;
> + struct net_device *ndev = ax_local->ndev;
> + u16 len;
> +
> + be16_to_cpus(>flags_len);
> + be16_to_cpus(>seq_lenbar);
> + be16_to_cpus(>flags);
> +
> + if short)rxhdr->flags_len) & RX_HDR1_PKT_LEN) !=
> + (~((short)rxhdr->seq_lenbar) & 0x7FF)) {
Why do you cast these values to signed types?
Is the casting necessary at all?
> + netif_err(ax_local, rx_err, ndev, "Header error\n");
> +
> +
