netdev - Re: [EXTERNAL] [PATCH net-next 3/3] net: airoha: Introduce ethernet support for EN7581 SoC

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: <Zl12e1LjSqf-M7cb@lore-desk>
Date: Mon, 3 Jun 2024 09:53:31 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Sunil Kovvuri Goutham <sgoutham@...vell.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"nbd@....name" <nbd@....name>,
	"lorenzo.bianconi83@...il.com" <lorenzo.bianconi83@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"conor@...nel.org" <conor@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"catalin.marinas@....com" <catalin.marinas@....com>,
	"will@...nel.org" <will@...nel.org>,
	"upstream@...oha.com" <upstream@...oha.com>,
	"angelogioacchino.delregno@...labora.com" <angelogioacchino.delregno@...labora.com>,
	"benjamin.larsson@...exis.eu" <benjamin.larsson@...exis.eu>
Subject: Re: [EXTERNAL] [PATCH net-next 3/3] net: airoha: Introduce ethernet
 support for EN7581 SoC

> 
> 
> >-----Original Message-----
> >From: Lorenzo Bianconi <lorenzo@...nel.org>
> >Sent: Friday, May 31, 2024 3:52 PM
> >To: netdev@...r.kernel.org
> >Cc: nbd@....name; lorenzo.bianconi83@...il.com; davem@...emloft.net;
> >edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> >conor@...nel.org; linux-arm-kernel@...ts.infradead.org; robh+dt@...nel.org;
> >krzysztof.kozlowski+dt@...aro.org; conor+dt@...nel.org;
> >devicetree@...r.kernel.org; catalin.marinas@....com; will@...nel.org;
> >upstream@...oha.com; angelogioacchino.delregno@...labora.com;
> >benjamin.larsson@...exis.eu
> >Subject: [EXTERNAL] [PATCH net-next 3/3] net: airoha: Introduce ethernet
> >support for EN7581 SoC
> >
> >Prioritize security for external emails: Confirm sender and content safety before
> >clicking links or opening attachments
> >
> >----------------------------------------------------------------------
> >Add airoha_eth driver in order to introduce ethernet support for
> >Airoha EN7581 SoC available on EN7581 development board (en7581-evb).
> >en7581-evb networking architecture is composed by airoha_eth as mac
> >controller (cpu port) and a mt7530 dsa based switch.
> >EN7581 mac controller is mainly composed by Frame Engine (FE) and
> >QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic
> >functionalities are supported now) while QDMA is used for DMA operation
> >and QOS functionalities between mac layer and the dsa switch (hw QoS is
> >not available yet and it will be added in the future).
> >Currently only hw lan features are available, hw wan will be added with
> >subsequent patches.
> >
> >Tested-by: Benjamin Larsson <benjamin.larsson@...exis.eu>
> >Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> >---
> ......
> >+
> >+static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
> >+{
> >+	struct airoha_eth *eth = q->eth;
> >+	struct device *dev = eth->net_dev->dev.parent;
> >+	int done = 0, qid = q - &eth->q_rx[0];
> >+
> >+	spin_lock_bh(&q->lock);
> 
> There is one napi per queue, why lock ?

we can get rid of it for rx queues (I will do in v2) but not for xmit ones
since airoha_qdma_tx_napi_poll() can run in parallel with airoha_dev_xmit()

> 
> ...........................
> >+
> >+	q = &eth->q_tx[qid];
> >+	spin_lock_bh(&q->lock);
> 
> Same here, is this lock needed ?
> If yes, can you please elaborate why.

ndo_start_xmit callback can run in parallel with airoha_qdma_tx_napi_poll()

> 
> >+
> >+	if (q->queued + nr_frags > q->ndesc) {
> >+		/* not enough space in the queue */
> >+		spin_unlock_bh(&q->lock);
> >+		return NETDEV_TX_BUSY;
> >+	}
> >+
> 
> I do not see netif_set_tso_max_segs() being set, so HW doesn't have any limit wrt
> number of TSO segs and number of fragments in skb, is it ??

I do not think there is any specific limitation for it

> 
> ...........
> >+static int airoha_probe(struct platform_device *pdev)
> >+{
> >+	struct device_node *np = pdev->dev.of_node;
> >+	struct net_device *dev;
> >+	struct airoha_eth *eth;
> >+	int err;
> >+
> >+	dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*eth),
> >+				      AIROHA_NUM_TX_RING,
> >AIROHA_NUM_RX_RING);
> 
> Always 32 queues, even if kernel is booted with less number cores ?

ethtool is not supported yet, I will add it with followup patches

> 
> 
> Overall this is a big patch deserving to be split, probably separate patches for init and datapath logic.

I guess specific parts (initialization, tx or rx code) are not big enough to deserve a dedicated patches.

> Also I do not see basic functionality like BQL not being supported, is that intentional ?

ack, I will add it in v2.

Regards,
Lorenzo

> 
> Thanks,
> Sunil.
> 

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