Re: [PATCH net-next v7 03/10] net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Deadlock risk due to using `spin_lock` instead of `spin_lock_bh` in `airoha_update_hw_stats`.
> --
> 
> commit af5b93ea60b28ce8a1ab1016b0911418480e766a
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct
> 
> This commit moves the airoha_qdma pointer from the airoha_gdm_port
> struct to airoha_gdm_dev, since the QDMA block depends on the net_device
> configuration. This is a preliminary patch to support multiple
> net_devices connected to the same port via an external hardware arbiter.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 3ca0dbd19275a..6c9f4b289dba7 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -1560,9 +1564,10 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
> >  	}
> >  }
> >  
> > -static void airoha_update_hw_stats(struct airoha_gdm_port *port)
> > +static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> >  {
> > -	struct airoha_eth *eth = port->qdma->eth;
> > +	struct airoha_gdm_port *port = dev->port;
> > +	struct airoha_eth *eth = dev->eth;
> >  	u32 val, i = 0;
> >  
> >  	spin_lock(&port->stats.lock);
> 
> This isn't a regression introduced by this patch, but does this spin_lock()
> need to use the _bh variant to prevent a deadlock?
> 
> This function is called from ethtool paths like airoha_ethtool_get_mac_stats()
> and airoha_ethtool_get_rmon_stats(), which run in process context. It is also
> called from airoha_dev_get_stats64() via ndo_get_stats64, which can be invoked
> from softirq context when queried by a bridging or bonding master or qdisc in
> the TX/RX paths.
> 
> If a process-context ethtool operation is preempted by a softirq on the same
> CPU that then invokes dev_get_stats(), could the softirq attempt to acquire
> the already-held port->stats.lock and cause a deadlock?

according to my understanding ndo_get_stats64() can't be invoked from softirq
context.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=3

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