Re: [PATCH net-next v5 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller

[Justin Chen]

On 5/25/23 8:54 PM, Jakub Kicinski wrote:
> On Wed, 24 May 2023 16:01:50 -0700 Justin Chen wrote:
> > Add support for the Broadcom ASP 2.0 Ethernet controller which is first
> > introduced with 72165. This controller features two distinct Ethernet
> > ports that can be independently operated.
> >
> > This patch supports:
> >
> > - Wake-on-LAN using magic packets
> > - basic ethtool operations (link, counters, message level)
> > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.)
>
> > +static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	struct bcmasp_intf *intf = netdev_priv(dev);
> > +	int spb_index, nr_frags, ret, i, j;
> > +	unsigned int total_bytes, size;
> > +	struct bcmasp_tx_cb *txcb;
> > +	dma_addr_t mapping, valid;
> > +	struct bcmasp_desc *desc;
> > +	bool csum_hw = false;
> > +	struct device *kdev;
> > +	skb_frag_t *frag;
> > +
> > +	kdev = &intf->parent->pdev->dev;
> > +
> > +	spin_lock(&intf->tx_lock);
>
> What is the tx_lock for? netdevs already have a tx lock, unless you
> declare the device as lockless.

Will remove.

> > +static void bcmasp_tx_timeout(struct net_device *dev, unsigned int txqueue)
> > +{
> > +	struct bcmasp_intf *intf = netdev_priv(dev);
> > +
> > +	netif_dbg(intf, tx_err, dev, "transmit timeout!\n");
> > +
> > +	netif_trans_update(dev);
> > +	dev->stats.tx_errors++;
> > +
> > +	netif_wake_queue(dev);
>
> If the queue is full xmit will just put it back to sleep.
> You want to try to reap completions if anything, no?

I can remove the wake. As you mentioned it won't do anything here. There 
isn't anything to reap if we are in the timeout condition. If it is some 
HW stall, we could flush and restart the ring, but if that is the case I 
rather figure out why the HW is stalling. I think we can leave it as a 
"tell the user we are stalled" and leave it as that.

> > +static struct net_device_stats *bcmasp_get_stats(struct net_device *dev)
> > +{
> > +	return &dev->stats;
> > +}
>
> you don't have to do this, core will use device stats if there's no ndo
>
> > +	ndev = alloc_etherdev(sizeof(struct bcmasp_intf));
> > +	if (!dev) {
>
> *blink* condition is typo'ed

Oops. Good catch.

Thanks,
Justin

> > +		dev_warn(dev, "%s: unable to alloc ndev\n", ndev_dn->name);
> > +		goto err;
> > +	}

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature