netdev - Re: [PATCH v4 2/2] 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: <ZoK8glSv-mtNPOLX@lore-desk>
Date: Mon, 1 Jul 2024 16:26:10 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, 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, rkannoth@...vell.com,
	sgoutham@...vell.com, andrew@...n.ch, arnd@...db.de
Subject: Re: [PATCH v4 2/2] net: airoha: Introduce ethernet support for
 EN7581 SoC

> On Sat, Jun 29, 2024 at 05:01:38PM +0200, Lorenzo Bianconi wrote:
> > 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>
> 
> Hi Lorenzo,
> 
> Some minor feedback from my side.

Hi Simon,

> 
> > +static void airoha_qdma_set_irqmask(struct airoha_eth *eth, int index,
> > +				    u32 clear, u32 set)
> > +{
> > +	unsigned long flags;
> > +
> > +	if (WARN_ON_ONCE(index >= ARRAY_SIZE(eth->irqmask)))
> > +		return;
> > +
> > +	spin_lock_irqsave(&eth->irq_lock, flags);
> > +
> > +	eth->irqmask[index] &= ~clear;
> > +	eth->irqmask[index] |= set;
> > +	airoha_qdma_wr(eth, REG_INT_ENABLE(index), eth->irqmask[index]);
> > +	/* Read irq_enable register in order to guarantee the update above
> > +	 * completes in the spinlock critical section.
> > +	 */
> > +	airoha_rr(eth, REG_INT_ENABLE(index));
> 
> airoha_rr() expects an __iomem pointer as it's first argument,
> but the type of eth is struct airoha_eth *eth.
> 
> Should this be using airoha_qdma_rr() instead?

ack, right. Thx for pointing this out. I will fix it in v5.

> 
> Flagged by Sparse.
> 
> > +
> > +	spin_unlock_irqrestore(&eth->irq_lock, flags);
> > +}
> 
> ...
> 
> > +static void airoha_ethtool_get_strings(struct net_device *dev, u32 sset,
> > +				       u8 *data)
> > +{
> > +	int i;
> > +
> > +	if (sset != ETH_SS_STATS)
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++) {
> > +		memcpy(data + i * ETH_GSTRING_LEN,
> > +		       airoha_ethtool_stats_name[i], ETH_GSTRING_LEN);
> > +	}
> > +
> > +	data += ETH_GSTRING_LEN * ARRAY_SIZE(airoha_ethtool_stats_name);
> > +	page_pool_ethtool_stats_get_strings(data);
> > +}
> 
> W=1 allmodconfig builds on x86_64 with gcc-13 complain about the use
> of memcpy above because the source is (often?) less than ETH_GSTRING_LEN
> bytes long.
> 
> I think the preferred solution is to use ethtool_puts(),
> something like this (compile tested only!):
> 
> @@ -2291,12 +2291,9 @@ static void airoha_ethtool_get_strings(struct net_device *dev, u32 sset,
>  	if (sset != ETH_SS_STATS)
>  		return;
>  
> -	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++) {
> -		memcpy(data + i * ETH_GSTRING_LEN,
> -		       airoha_ethtool_stats_name[i], ETH_GSTRING_LEN);
> -	}
> +	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++)
> +		ethtool_puts(&data, airoha_ethtool_stats_name[i]);
>  
> -	data += ETH_GSTRING_LEN * ARRAY_SIZE(airoha_ethtool_stats_name);
>  	page_pool_ethtool_stats_get_strings(data);
>  }
>  

ack, I will fix it in v5.

> 
> ...
> 
> > +static int airoha_alloc_gdm_port(struct airoha_eth *eth, struct device_node *np)
> > +{
> > +	const __be32 *id_ptr = of_get_property(np, "reg", NULL);
> > +	struct net_device *dev;
> > +	struct airoha_gdm_port *port;
> 
> nit: reverse xmas tree

ack, I will fix it in v5.

Regards,
Lorenzo

> 
> > +	int err, index;
> > +	u32 id;
> 
> ...
> 
> -- 
> pw-bot: changes-requested

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