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