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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0b4ef62-2d80-d5b2-e0b0-b15c00b75143@broadcom.com>
Date:   Fri, 26 May 2023 14:46:30 -0700
From:   Justin Chen <justin.chen@...adcom.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        bcm-kernel-feedback-list@...adcom.com,
        florian.fainelli@...adcom.com, davem@...emloft.net,
        edumazet@...gle.com, pabeni@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        opendmb@...il.com, andrew@...n.ch, hkallweit1@...il.com,
        linux@...linux.org.uk, richardcochran@...il.com,
        sumit.semwal@...aro.org, christian.koenig@....com,
        simon.horman@...igine.com
Subject: Re: [PATCH net-next v5 3/6] net: bcmasp: Add support for ASP2.0
 Ethernet controller



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;
>> +	}
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4206 bytes)