Re: [PATCH net v2 1/2] net: ravb: Fix missing rtnl lock in suspend path

[Kory Maincent <kory.maincent@bootlin.com>]

On Thu, 23 Jan 2025 18:33:58 +0100
Kory Maincent <kory.maincent@bootlin.com> wrote:
 
> > >  
> > > @@ -3247,7 +3253,9 @@ static int ravb_resume(struct device *dev)
> > >  
> > >  	/* If WoL is enabled restore the interface. */
> > >  	if (priv->wol_enabled) {
> > > +		rtnl_lock();
> > >  		ret = ravb_wol_restore(ndev);
> > > +		rtnl_unlock();
> > >  		if (ret)
> > >  			return ret;
> > >  	} else {
> > > @@ -3257,7 +3265,9 @@ static int ravb_resume(struct device *dev)
> > >  	}
> > >  
> > >  	/* Reopening the interface will restore the device to the working
> > > state. */
> > > +	rtnl_lock();
> > >  	ret = ravb_open(ndev);
> > > +	rtnl_unlock();
> > >  	if (ret < 0)
> > >  		goto out_rpm_put;
>
> > I don't like the multiple lock/unlock calls in each function. I think v1
> > was better, where we take the lock once in each function and then unlock
> > when it is no longer needed or when we're about to return.
> 
> You will need to achieve a consensus on it with Claudiu. His point of view has
> that the locking scheme looks complicated.
> 
> On my side I don't have really an opinion, maybe a small preference for v1
> which is protecting wol_enabled flag even if it is not needed.

Claudiu any remarks?

If not I will come back to the first version as asked by Paul who is the
Maintainer of the ravb driver.

Sergey have asked to remove the duplicate of the if condition.
Paul is this ok for you?

@@ -3245,19 +3250,21 @@ static int ravb_resume(struct device *dev)
        if (!netif_running(ndev))
                return 0;
 
+       rtnl_lock();
        /* If WoL is enabled restore the interface. */
-       if (priv->wol_enabled) {
+       if (priv->wol_enabled)
                ret = ravb_wol_restore(ndev);
-               if (ret)
-                       return ret;
-       } else {
+       else
                ret = pm_runtime_force_resume(dev);
-               if (ret)
-                       return ret;
+
+       if (ret) {
+               rtnl_unlock();
+               return ret;
        }
 
        /* Reopening the interface will restore the device to the working
state. */
        ret = ravb_open(ndev);
+       rtnl_unlock();
        if (ret < 0)
                goto out_rpm_put;


Note: Sergey, I have received your mail as spam. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com