| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dleftj8s8nwgmx.fsf%l.stelmach@samsung.com>
Date: Wed, 20 Jan 2021 20:30:14 +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 v10 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet
Adapter Driver
It was <2021-01-15 pią 17:27>, when Jakub Kicinski wrote:
> On Wed, 13 Jan 2021 19:40:28 +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
>>
[...]
>>
>> 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>
>
>> +static u32 ax88796c_get_priv_flags(struct net_device *ndev)
>> +{
>> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +
>> + return ax_local->priv_flags;
>
> stray indent
>
>> +}
Done.
>> +#define MAX(x,y) ((x) > (y) ? (x) : (y))
>
> Please use the standard linux max / max_t macros.
Done.
>> +static struct sk_buff *
>> +ax88796c_tx_fixup(struct net_device *ndev, struct sk_buff_head *q)
>> +{
>> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> + u8 spi_len = ax_local->ax_spi.comp ? 1 : 4;
>> + struct sk_buff *skb;
>> + struct tx_pkt_info *info;
>> + struct skb_data *entry;
>> + u16 pkt_len;
>> + u8 padlen, seq_num;
>> + u8 need_pages;
>> + int headroom;
>> + int tailroom;
>> +
>> + if (skb_queue_empty(q))
>> + return NULL;
>> +
>> + skb = skb_peek(q);
>> + pkt_len = skb->len;
>> + need_pages = (pkt_len + TX_OVERHEAD + 127) >> 7;
>> + if (ax88796c_check_free_pages(ax_local, need_pages) != 0)
>> + return NULL;
>> +
>> + headroom = skb_headroom(skb);
>> + tailroom = skb_tailroom(skb);
>> + padlen = round_up(pkt_len, 4) - pkt_len;
>> + 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))) {
>> + size_t h = MAX((TX_OVERHEAD + spi_len) - headroom,0);
>> + size_t t = MAX((padlen + TX_EOP_SIZE) - tailroom,0);
>> +
>> + if (pskb_expand_head(skb, h, t, GFP_KERNEL))
>> + return NULL;
>> + }
>> +
>> + info->seq_num = seq_num;
>> + ax88796c_proc_tx_hdr(info, skb->ip_summed);
>> +
>> + /* SOP and SEG header */
>> + memcpy(skb_push(skb, TX_OVERHEAD), &info->sop, TX_OVERHEAD);
>
> why use skb->cb to store info? why not declare it on the stack?
>
Done.
>> + /* Write SPI TXQ header */
>> + memcpy(skb_push(skb, spi_len), ax88796c_tx_cmd_buf, spi_len);
>> +
>> + /* Make 32-bit alignment */
>> + skb_put(skb, padlen);
>> +
>> + /* EOP header */
>> + memcpy(skb_put(skb, TX_EOP_SIZE), &info->eop, TX_EOP_SIZE);
>> +
>> + skb_unlink(skb, q);
>> +
>> + entry = (struct skb_data *)skb->cb;
>> + memset(entry, 0, sizeof(*entry));
>> + entry->len = pkt_len;
>> +
>> + if (netif_msg_pktdata(ax_local)) {
>> + char pfx[IFNAMSIZ + 7];
>> +
>> + snprintf(pfx, sizeof(pfx), "%s: ", ndev->name);
>> +
>> + netdev_info(ndev, "TX packet len %d, total len %d, seq %d\n",
>> + pkt_len, skb->len, seq_num);
>> +
>> + netdev_info(ndev, " SPI Header:\n");
>> + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> + skb->data, 4, 0);
>> +
>> + netdev_info(ndev, " TX SOP:\n");
>> + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> + skb->data + 4, TX_OVERHEAD, 0);
>> +
>> + netdev_info(ndev, " TX packet:\n");
>> + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> + skb->data + 4 + TX_OVERHEAD,
>> + skb->len - TX_EOP_SIZE - 4 - TX_OVERHEAD, 0);
>> +
>> + netdev_info(ndev, " TX EOP:\n");
>> + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1,
>> + skb->data + skb->len - 4, 4, 0);
>> + }
>> +
>> + return skb;
>> +}
>> +
>> +static int ax88796c_hard_xmit(struct ax88796c_device *ax_local)
>> +{
>> + struct sk_buff *tx_skb;
>> + struct skb_data *entry;
>> +
>> + WARN_ON(!mutex_is_locked(&ax_local->spi_lock));
>> +
>> + tx_skb = ax88796c_tx_fixup(ax_local->ndev, &ax_local->tx_wait_q);
>> +
>> + if (!tx_skb)
>> + return 0;
>
> tx_dropped++ ?
>
Done.
>> + entry = (struct skb_data *)tx_skb->cb;
>> +
>> + AX_WRITE(&ax_local->ax_spi,
>> + (TSNR_TXB_START | TSNR_PKT_CNT(1)), P0_TSNR);
>> +
>> + axspi_write_txq(&ax_local->ax_spi, tx_skb->data, tx_skb->len);
>> +
>> + if (((AX_READ(&ax_local->ax_spi, P0_TSNR) & TXNR_TXB_IDLE) == 0) ||
>> + ((ISR_TXERR & AX_READ(&ax_local->ax_spi, P0_ISR)) != 0)) {
>> + /* Ack tx error int */
>> + AX_WRITE(&ax_local->ax_spi, ISR_TXERR, P0_ISR);
>> +
>> + ax_local->stats.tx_dropped++;
>> +
>> + netif_err(ax_local, tx_err, ax_local->ndev,
>> + "TX FIFO error, re-initialize the TX bridge\n");
>
> rate limit
>
Done.
>> + /* Reinitial tx bridge */
>> + AX_WRITE(&ax_local->ax_spi, TXNR_TXB_REINIT |
>> + AX_READ(&ax_local->ax_spi, P0_TSNR), P0_TSNR);
>> + ax_local->seq_num = 0;
>> + } else {
>> + ax_local->stats.tx_packets++;
>> + ax_local->stats.tx_bytes += entry->len;
>> + }
>> +
>> + entry->state = tx_done;
>> + dev_kfree_skb(tx_skb);
>
> dev_consume_skb() is better in cases the xmission was correct.
> kfree_skb() shows up in packet drop monitor.
>
This one is OK as it is because
1. dev_kfree_skb() is consume_skb()
include/linux/skbuff.h:#define dev_kfree_skb(a) consume_skb(a)
2. dev_consume_skb() does not exist. There are dev_consume_skb_irq() and
dev_consume_skb_any(). The former can be used in IRQs, the latter
anywher and de facto calls the former if in IRQ. If not, it calls
dev_kfree_skb() (see above)
3. Last but not least. kfree_skb() and consume_skb() become the same
without CONFIG_TRACEPOINTS (commit be769db2f958).
>> +
>> + return 1;
>> +}
>
>> +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->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);
>
> rate limit
>
Done.
>> +}
>> +
>> +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 (((rxhdr->flags_len) & RX_HDR1_PKT_LEN) !=
>> + (~(rxhdr->seq_lenbar) & 0x7FF)) {
>
> Lots of unnecessary parenthesis.
>
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);
>> +
>> + ax88796c_skb_return(ax_local, rx_skb, rxhdr);
>> +}
>> +
>> +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;
>
> w_count = round_up(pkt_len + 6, 4) >> 1;
>
Done.
>> + skb = netdev_alloc_skb(ndev, (w_count * 2));
>
> parenthesis unnecessary
>
Done.
>> + if (!skb) {
>> + AX_WRITE(&ax_local->ax_spi, RXBCR1_RXB_DISCARD, P0_RXBCR1);
>
> Increment rx_dropped counter here?
>
Done.
>> + 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");
>
> rate limit?
>
Ok.
>> + 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 int ax88796c_process_isr(struct ax88796c_device *ax_local)
>> +{
>> + struct net_device *ndev = ax_local->ndev;
>> + u8 done = 0;
>
> The logic associated with this variable is "is there more to do" rather
> than "done", no?
>
ax88796c_receive() returns 1, if there may be something more to do. So
yes, let's rename it to todo.
>> + u16 isr;
>> +
>> + WARN_ON(!mutex_is_locked(&ax_local->spi_lock));
>> +
>> + isr = AX_READ(&ax_local->ax_spi, P0_ISR);
>> + AX_WRITE(&ax_local->ax_spi, isr, P0_ISR);
>> +
>> + netif_dbg(ax_local, intr, ndev, " ISR 0x%04x\n", isr);
>> +
>> + if (isr & ISR_TXERR) {
>> + netif_dbg(ax_local, intr, ndev, " TXERR interrupt\n");
>> + AX_WRITE(&ax_local->ax_spi, TXNR_TXB_REINIT, P0_TSNR);
>> + ax_local->seq_num = 0x1f;
>> + }
>> +
>> + if (isr & ISR_TXPAGES) {
>> + netif_dbg(ax_local, intr, ndev, " TXPAGES interrupt\n");
>> + set_bit(EVENT_TX, &ax_local->flags);
>> + }
>> +
>> + if (isr & ISR_LINK) {
>> + netif_dbg(ax_local, intr, ndev, " Link change interrupt\n");
>> + phy_mac_interrupt(ax_local->ndev->phydev);
>> + }
>> +
>> + if (isr & ISR_RXPKT) {
>> + netif_dbg(ax_local, intr, ndev, " RX interrupt\n");
>> + done = ax88796c_receive(ax_local->ndev);
>> + }
>> +
>> + return done;
>> +}
>
>> +static void ax88796c_work(struct work_struct *work)
>> +{
>> + struct ax88796c_device *ax_local =
>> + container_of(work, struct ax88796c_device, ax_work);
>> +
>> + mutex_lock(&ax_local->spi_lock);
>> +
>> + if (test_bit(EVENT_SET_MULTI, &ax_local->flags)) {
>> + ax88796c_set_hw_multicast(ax_local->ndev);
>> + clear_bit(EVENT_SET_MULTI, &ax_local->flags);
>> + }
>> +
>> + if (test_bit(EVENT_INTR, &ax_local->flags)) {
>> + AX_WRITE(&ax_local->ax_spi, IMR_MASKALL, P0_IMR);
>> +
>> + while (1) {
>> + if (!ax88796c_process_isr(ax_local))
>> + break;
>
> while (ax88796c_process_isr(ax_local))
> /* nothing */;
> ?
>
Ok.
>> + }
>> +
>> + clear_bit(EVENT_INTR, &ax_local->flags);
>> +
>> + AX_WRITE(&ax_local->ax_spi, IMR_DEFAULT, P0_IMR);
>> +
>> + enable_irq(ax_local->ndev->irq);
>> + }
>> +
>> + if (test_bit(EVENT_TX, &ax_local->flags)) {
>> + while (skb_queue_len(&ax_local->tx_wait_q)) {
>> + if (!ax88796c_hard_xmit(ax_local))
>> + break;
>> + }
>> +
>> + clear_bit(EVENT_TX, &ax_local->flags);
>> +
>> + if (netif_queue_stopped(ax_local->ndev) &&
>> + (skb_queue_len(&ax_local->tx_wait_q) < TX_QUEUE_LOW_WATER))
>> + netif_wake_queue(ax_local->ndev);
>> + }
>> +
>> + mutex_unlock(&ax_local->spi_lock);
>> +}
>
>> +static void ax88796c_set_csums(struct ax88796c_device *ax_local)
>> +{
>> + struct net_device *ndev = ax_local->ndev;
>> +
>> + WARN_ON(!mutex_is_locked(&ax_local->spi_lock));
>
> lockdep_assert_held() in all those cases
>
Done.
>> +static void ax88796c_free_skb_queue(struct sk_buff_head *q)
>> +{
>> + struct sk_buff *skb;
>> +
>> + while (q->qlen) {
>> + skb = skb_dequeue(q);
>> + kfree_skb(skb);
>> + }
>
> __skb_queue_purge()
>
Done.
>> +}
>> +
>> +static int
>> +ax88796c_close(struct net_device *ndev)
>> +{
>> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +
>> + netif_stop_queue(ndev);
>> + phy_stop(ndev->phydev);
>> +
>> + mutex_lock(&ax_local->spi_lock);
>> +
>> + /* Disable MAC interrupts */
>> + AX_WRITE(&ax_local->ax_spi, IMR_MASKALL, P0_IMR);
>> + ax88796c_free_skb_queue(&ax_local->tx_wait_q);
>> + ax88796c_soft_reset(ax_local);
>> +
>> + mutex_unlock(&ax_local->spi_lock);
>> +
>> + free_irq(ndev->irq, ndev);
>> +
>> + return 0;
>> +}
>
>> +struct ax88796c_device {
>> + struct spi_device *spi;
>> + struct net_device *ndev;
>> + struct net_device_stats stats;
>
> You need to use 64 bit stats, like struct rtnl_link_stats64.
> On a 32bit system at 100Mbps ulong can wrap in minutes.
>
Let me see. At first glance
git grep -l ndo_get_stats\\\> drivers/net/ethernet/ | xargs grep -li SPEED_100\\\>
quite a number of Fast Ethernet drivers use net_device_stats. Let me
calculate.
- bytes
100Mbps is ~10MiB/s
sending 4GiB at 10MiB/s takes 27 minutes
- packets
minimum frame size is 84 bytes (840 bits on the wire) on 100Mbps means
119048 pps at this speed it takse 10 hours to transmit 2^32 packets
Anyway, I switched to rtnl_link_stats64. Tell me, is it OK to just
memcpy() in .ndo_get_stats64?
>> + struct work_struct ax_work;
>
> I don't see you ever canceling / flushing this work.
> You should do that at least on driver remove if not close.
Done.
Does it mean most drivers do it wrong?
git grep INIT_WORK drivers/net/ethernet/ | \
sed -e 's/\(^[^:]*\):[^>]*->\([^,]*\),.*/\1 \2/' | \
while read file var; do \
grep -H $var $file;
done | grep INIT_WORK\\\|cancel_work
>> + struct mutex spi_lock; /* device access */
>> +
>> + struct sk_buff_head tx_wait_q;
>> +
>> + struct axspi_data ax_spi;
>> +
>> + struct mii_bus *mdiobus;
>> + struct phy_device *phydev;
>> +
>> + int msg_enable;
>> +
>> + u16 seq_num;
>> +
>> + u8 multi_filter[AX_MCAST_FILTER_SIZE];
>> +
>> + int link;
>> + int speed;
>> + int duplex;
>> + int pause;
>> + int asym_pause;
>> + int flowctrl;
>> + #define AX_FC_NONE 0
>> + #define AX_FC_RX BIT(0)
>> + #define AX_FC_TX BIT(1)
>> + #define AX_FC_ANEG BIT(2)
>> +
>> + u32 priv_flags;
>> + #define AX_CAP_COMP BIT(0)
>> + #define AX_PRIV_FLAGS_MASK (AX_CAP_COMP)
>> +
>> + unsigned long flags;
>> + #define EVENT_INTR BIT(0)
>> + #define EVENT_TX BIT(1)
>> + #define EVENT_SET_MULTI BIT(2)
>> +
>> +};
>
>> +struct skb_data {
>> + enum skb_state state;
>> + struct net_device *ndev;
>> + struct sk_buff *skb;
>
> Don't think you ever use the skb or ndev from this structure.
>
Done.
>> + size_t len;
>> +};
>
>
--
Ł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