Re: [PATCH net-next v9 3/6] net: airoha: Support multiple net_devices for a single FE GDM port

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

[...]
> 
> Are there similar issues with the ingress configuration?  Earlier in
> airoha_dev_open(), REG_GDM_INGRESS_CFG(port->id) is modified based on whether
> the specific netdev uses DSA:
> 
> 	if (netdev_uses_dsa(netdev))
> 		airoha_fe_set(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
> 			      GDM_STAG_EN_MASK);
> 	else
> 		airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
> 				GDM_STAG_EN_MASK);

This is not a real issue since just GDM1 can be used as cpu port of the dsa
switch (MT7630) and we do not support multiple net_devices for GDM1.

> 
> Also, do airoha_dev_stop() and airoha_dev_change_mtu() have similar
> regressions?  They appear to blindly overwrite shared registers like
> REG_GDM_FWD_CFG (setting it to FE_PSE_PORT_DROP) and REG_GDM_LEN_CFG(port->id).
> 
> I noticed the stop() and MTU issues might be fixed in a subsequent patch, but
> should this intermediate patch be self-contained to avoid breaking
> bisectability?

These issues are addressed in subsequent patches on the same series.

> 
> [ ... ]
> > @@ -3080,15 +3100,51 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> >  	u64_stats_init(&port->stats.syncp);
> >  	spin_lock_init(&port->stats.lock);
> >  	port->id = id;
> > -	/* XXX: Read nbq from DTS */
> > -	port->nbq = id == AIROHA_GDM3_IDX && airoha_is_7581(eth) ? 4 : 0;
> >  	eth->ports[p] = port;
> >  
> >  	err = airoha_metadata_dst_alloc(port);
> >  	if (err)
> >  		return err;
> >  
> > -	return airoha_alloc_gdm_device(eth, port, np);
> > +	/* Default nbq value to ensure backward compatibility */
> > +	nbq = id == AIROHA_GDM3_IDX && airoha_is_7581(eth) ? 4 : 0;
> > +
> > +	for_each_child_of_node(np, node) {
> > +		/* Multiple external serdes connected to the FE GDM port via an
> > +		 * external arbiter.
> > +		 */
> > +		const __be32 *nbq_ptr;
> 
> [Severity: Medium]
> Will these multiplexed devices report duplicated, incorrect hardware statistics?

hw statistics will be addressed with a dedicated patch not part of the series.

Regards,
Lorenzo

> 
> When userspace queries statistics for any single multiplexed device,
> airoha_dev_get_stats64() fetches from port->stats, which is updated by
> airoha_update_hw_stats() reading the clear-on-read MAC hardware counters for
> the entire physical GDM port.
> 
> Since the port->stats structure is shared across all net_devices attached to
> the port, each device will report the total traffic volume of all devices
> sharing the port rather than its own traffic.
> 
> Should the driver rely on per-queue software counters to provide accurate
> per-device statistics when hardware MACs are multiplexed?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260603-airoha-eth-multi-serdes-v9-0-5d476bc2f426@kernel.org?part=3

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