Re: [PATCH net-next v8 02/10] net: airoha: Introduce airoha_gdm_dev struct

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Potential deadlock in `airoha_update_hw_stats()` due to using `spin_lock()` instead of `spin_lock_bh()`.
> --
> 
> commit 6823c137925a562390270eb1ce7967caaa011b7b
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Introduce airoha_gdm_dev struct
> 
> This commit introduces the airoha_gdm_dev struct to collect net_device related
> info, accommodating future support for connecting multiple external SerDes to
> GDM3 or GDM4 ports via a hardware arbiter.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 5a027cc7ffcb1..5335271ea8654 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> 
> [ ... ]
> 
> > @@ -1877,10 +1888,11 @@ static int airoha_dev_init(struct net_device *dev)
> >  	return 0;
> >  }
> >  
> > -static void airoha_dev_get_stats64(struct net_device *dev,
> > +static void airoha_dev_get_stats64(struct net_device *netdev,
> >  				   struct rtnl_link_stats64 *storage)
> >  {
> > -	struct airoha_gdm_port *port = netdev_priv(dev);
> > +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > +	struct airoha_gdm_port *port = dev->port;
> >  	unsigned int start;
> >  
> >  	airoha_update_hw_stats(port);
> 
> This isn't a bug introduced by this patch, but could this lead to a deadlock?
> 
> Because airoha_dev_get_stats64() is registered as the ndo_get_stats64 callback,
> it can be called from both process context (e.g., via user-space netlink dumps)
> and software interrupt context (BH) (e.g., via bonding or teaming drivers).
> 
> Looking at airoha_update_hw_stats(), it uses spin_lock() to protect the
> hardware statistics update:
> 
> drivers/net/ethernet/airoha/airoha_eth.c:airoha_update_hw_stats() {
> 	...
> 	spin_lock(&port->stats.lock);
> 	u64_stats_update_begin(&port->stats.syncp);
> 	...
> }
> 
> Because spin_lock() disables preemption but not softirqs, if a process context
> call is interrupted by a BH call on the same CPU, the BH handler will attempt
> to acquire the already-held lock.
> 
> Should this be upgraded to spin_lock_bh() to prevent a potential deadlock?

As pointed out before, according to my understanding, ndo_get_stats64()
callback can't run from software interrupt context (BH), so this is not a real
issue.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6@kernel.org?part=2

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