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

[Simon Horman <horms@kernel.org>]

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.

> +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?

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

...

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

> +	int err, index;
> +	u32 id;

...

-- 
pw-bot: changes-requested