Re: [PATCH v4 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3812 bytes --]

> 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@genexis.eu>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]