Date:   Wed, 16 Dec 2020 01:42:03 +0100
From:   Lukasz Stelmach <l.stelmach@...sung.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>, jim.cromie@...il.com,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: 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&q=1&e=0b28bac6-bda4-4cf1-8d38-c5c264b64aca&u=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&q=1&e=0b28bac6-bda4-4cf1-8d38-c5c264b64aca&u=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 <l.stelmach@...sung.com>
>> Reviewed-by: Andrew Lunn <andrew@...n.ch>
>
>> +	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),
>> +		       &info->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),
>> +		       &info->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);
>> +
>> +	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(&rxhdr->flags_len);
>> +	be16_to_cpus(&rxhdr->seq_lenbar);
>> +	be16_to_cpus(&rxhdr->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?
>

It is not. Done.

>> +		netif_err(ax_local, rx_err, ndev, "Header error\n");
>> +
>> +		ax_local->stats.rx_frame_errors++;
>> +		kfree_skb(rx_skb);
>> +		return;
>> +	}
>> +
>> +	if ((rxhdr->flags_len & RX_HDR1_MII_ERR) ||
>> +	    (rxhdr->flags_len & RX_HDR1_CRC_ERR)) {
>> +		netif_err(ax_local, rx_err, ndev, "CRC or MII error\n");
>> +
>> +		ax_local->stats.rx_crc_errors++;
>> +		kfree_skb(rx_skb);
>> +		return;
>> +	}
>> +
>> +	len = rxhdr->flags_len & RX_HDR1_PKT_LEN;
>> +	if (netif_msg_pktdata(ax_local)) {
>> +		char pfx[IFNAMSIZ + 7];
>> +
>> +		snprintf(pfx, sizeof(pfx), "%s:     ", ndev->name);
>> +		netdev_info(ndev, "RX data, total len %d, packet len %d\n",
>> +			    rx_skb->len, len);
>> +
>> +		netdev_info(ndev, "  Dump RX packet header:");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       rx_skb->data, sizeof(*rxhdr), 0);
>> +
>> +		netdev_info(ndev, "  Dump RX packet:");
>> +		print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> +			       rx_skb->data + sizeof(*rxhdr), len, 0);
>> +	}
>> +
>> +	skb_pull(rx_skb, sizeof(*rxhdr));
>> +	__pskb_trim(rx_skb, len);
>
> Why __pskb_trim? skb_trim() should do.
>

Done.

>> +	return ax88796c_skb_return(ax_local, rx_skb, rxhdr);
>
> please drop the "return", both caller and callee are void.
>

Done.

>> +}
>> +
>> +static int ax88796c_receive(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	struct skb_data *entry;
>> +	u16 w_count, pkt_len;
>> +	struct sk_buff *skb;
>> +	u8 pkt_cnt;
>> +
>> +	WARN_ON(!mutex_is_locked(&ax_local->spi_lock));
>> +
>> +	/* check rx packet and total word count */
>> +	AX_WRITE(&ax_local->ax_spi, AX_READ(&ax_local->ax_spi, P0_RTWCR)
>> +		  | RTWCR_RX_LATCH, P0_RTWCR);
>> +
>> +	pkt_cnt = AX_READ(&ax_local->ax_spi, P0_RXBCR2) & RXBCR2_PKT_MASK;
>> +	if (!pkt_cnt)
>> +		return 0;
>> +
>> +	pkt_len = AX_READ(&ax_local->ax_spi, P0_RCPHR) & 0x7FF;
>> +
>> +	w_count = ((pkt_len + 6 + 3) & 0xFFFC) >> 1;
>> +
>> +	skb = alloc_skb((w_count * 2), GFP_ATOMIC);
>
> netdev_alloc_skb()
>

Done.

>> +	if (!skb) {
>> +		AX_WRITE(&ax_local->ax_spi, RXBCR1_RXB_DISCARD, P0_RXBCR1);
>> +		return 0;
>> +	}
>> +	entry = (struct skb_data *)skb->cb;
>> +
>> +	AX_WRITE(&ax_local->ax_spi, RXBCR1_RXB_START | w_count, P0_RXBCR1);
>> +
>> +	axspi_read_rxq(&ax_local->ax_spi,
>> +		       skb_put(skb, w_count * 2), skb->len);
>> +
>> +	/* Check if rx bridge is idle */
>> +	if ((AX_READ(&ax_local->ax_spi, P0_RXBCR2) & RXBCR2_RXB_IDLE) == 0) {
>> +		netif_err(ax_local, rx_err, ndev,
>> +			  "Rx Bridge is not idle\n");
>> +		AX_WRITE(&ax_local->ax_spi, RXBCR2_RXB_REINIT, P0_RXBCR2);
>> +
>> +		entry->state = rx_err;
>> +	} else {
>> +		entry->state = rx_done;
>> +	}
>> +
>> +	AX_WRITE(&ax_local->ax_spi, ISR_RXPKT, P0_ISR);
>> +
>> +	ax88796c_rx_fixup(ax_local, skb);
>> +
>> +	return 1;
>> +}
>
>> +static irqreturn_t ax88796c_interrupt(int irq, void *dev_instance)
>> +{
>> +	struct ax88796c_device *ax_local;
>> +	struct net_device *ndev;
>> +
>> +	ndev = dev_instance;
>> +	if (!ndev) {
>> +		pr_err("irq %d for unknown device.\n", irq);
>> +		return IRQ_RETVAL(0);
>> +	}
>> +	ax_local = to_ax88796c_device(ndev);
>> +
>> +	disable_irq_nosync(irq);
>> +
>> +	netif_dbg(ax_local, intr, ndev, "Interrupt occurred\n");
>> +
>> +	set_bit(EVENT_INTR, &ax_local->flags);
>> +	schedule_work(&ax_local->ax_work);
>> +
>> +	return IRQ_HANDLED;
>> +}
>
> 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

> Also you're not checking if this is actually your devices IRQ that
> triggered. You can't set IRQF_SHARED.

Fixed.

>> +static int
>> +ax88796c_open(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	unsigned long irq_flag = IRQF_SHARED;
>> +	int fc = AX_FC_NONE;
>> +	int ret;
>> +
>> +	ret = request_irq(ndev->irq, ax88796c_interrupt,
>> +			  irq_flag, ndev->name, ndev);
>> +	if (ret) {
>> +		netdev_err(ndev, "unable to get IRQ %d (errno=%d).\n",
>> +			   ndev->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	mutex_lock(&ax_local->spi_lock);
>> +
>> +	ret = ax88796c_soft_reset(ax_local);
>> +	if (ret < 0) {
>> +		mutex_unlock(&ax_local->spi_lock);
>> +		return ret;
>
> What frees the IRQ?
>

Fixed.

>> +	}
>> +	ax_local->seq_num = 0x1f;
>> +
>> +	ax88796c_set_mac_addr(ndev);
>> +	ax88796c_set_csums(ax_local);
>> +
>> +	/* Disable stuffing packet */
>> +	AX_WRITE(&ax_local->ax_spi,
>> +		 AX_READ(&ax_local->ax_spi, P1_RXBSPCR)
>> +		 & ~RXBSPCR_STUF_ENABLE, P1_RXBSPCR);
>
> Please use a temporary variable or create a RMW helper.
>
>> +	/* Enable RX packet process */
>> +	AX_WRITE(&ax_local->ax_spi, RPPER_RXEN, P1_RPPER);
>> +
>> +	AX_WRITE(&ax_local->ax_spi, AX_READ(&ax_local->ax_spi, P0_FER)
>> +		 | FER_RXEN | FER_TXEN | FER_BSWAP | FER_IRQ_PULL, P0_FER);
>
> Ditto. etc.
>

Done x2.

>> +	ndev = devm_alloc_etherdev(&spi->dev, sizeof(*ax_local));
>> +	if (!ndev)
>> +		return -ENOMEM;
>> +
>> +	SET_NETDEV_DEV(ndev, &spi->dev);
>> +
>> +	ax_local = to_ax88796c_device(ndev);
>> +	memset(ax_local, 0, sizeof(*ax_local));
>
> No need to zero out netdev priv, it's zalloced.
>

Fixed.

>> +	mutex_lock(&ax_local->spi_lock);
>> +
>> +	/* ax88796c gpio reset */
>> +	ax88796c_hard_reset(ax_local);
>> +
>> +	/* Reset AX88796C */
>> +	ret = ax88796c_soft_reset(ax_local);
>> +	if (ret < 0) {
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +	/* Check board revision */
>> +	temp = AX_READ(&ax_local->ax_spi, P2_CRIR);
>> +	if ((temp & 0xF) != 0x0) {
>> +		dev_err(&spi->dev, "spi read failed: %d\n", temp);
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>
> These jump out without releasing the lock.
>

Fixed.

>> +	/* Disable power saving */
>> +	AX_WRITE(&ax_local->ax_spi, (AX_READ(&ax_local->ax_spi, P0_PSCR)
>> +				     & PSCR_PS_MASK) | PSCR_PS_D0, P0_PSCR);
>
> This is asking for a temporary variable or a RMW helper.
>
>> +	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.

>> +/* Tx headers structure */
>> +struct tx_sop_header {
>> +	/* bit 15-11: flags, bit 10-0: packet length */
>> +	u16 flags_len;
>> +	/* bit 15-11: sequence number, bit 11-0: packet length bar */
>> +	u16 seq_lenbar;
>> +} __packed;
>> +
>> +struct tx_segment_header {
>> +	/* bit 15-14: flags, bit 13-11: segment number */
>> +	/* bit 10-0: segment length */
>> +	u16 flags_seqnum_seglen;
>> +	/* bit 15-14: end offset, bit 13-11: start offset */
>> +	/* bit 10-0: segment length bar */
>> +	u16 eo_so_seglenbar;
>> +} __packed;
>> +
>> +struct tx_eop_header {
>> +	/* bit 15-11: sequence number, bit 10-0: packet length */
>> +	u16 seq_len;
>> +	/* bit 15-11: sequence number bar, bit 10-0: packet length bar */
>> +	u16 seqbar_lenbar;
>> +} __packed;
>> +
>> +struct tx_pkt_info {
>> +	struct tx_sop_header sop;
>> +	struct tx_segment_header seg;
>> +	struct tx_eop_header eop;
>> +	u16 pkt_len;
>> +	u16 seq_num;
>> +} __packed;
>> +
>> +/* Rx headers structure */
>> +struct rx_header {
>> +	u16 flags_len;
>> +	u16 seq_lenbar;
>> +	u16 flags;
>> +} __packed;
>
> 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.

>> +u16 axspi_read_reg(struct axspi_data *ax_spi, u8 reg)
>> +{
>> +	int ret;
>> +	int len = ax_spi->comp ? 3 : 4;
>> +
>> +	ax_spi->cmd_buf[0] = 0x03;	/* OP code read register */
>> +	ax_spi->cmd_buf[1] = reg;	/* register address */
>> +	ax_spi->cmd_buf[2] = 0xFF;	/* dumy cycle */
>> +	ax_spi->cmd_buf[3] = 0xFF;	/* dumy cycle */
>> +	ret = spi_write_then_read(ax_spi->spi,
>> +				  ax_spi->cmd_buf, len,
>> +				  ax_spi->rx_buf, 2);
>> +	if (ret)
>> +		dev_err(&ax_spi->spi->dev, "%s() failed: ret = %d\n", __func__, ret);
>> +	else
>> +		le16_to_cpus(ax_spi->rx_buf);
>> +
>> +	return *(u16 *)ax_spi->rx_buf;
>
> 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_*.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (488 bytes)

Powered by blists - more mailing lists