Re: [PATCH 1/1] driver/net: add missing rtnl lock/unlock for benet

[Li, ZhenHua]

Because netif_running() is called in netif_device_detach and 
netif_device_attach. To avoid dev status changed while 
netif_device_detach/attach is not finished, I think a rtnl_lock and 
unlock should be called to avoid this.

Thanks
Zhenhua

On 04/15/2014 04:07 PM, Sathya Perla wrote:
> > -----Original Message-----
> > From: Li, Zhen-Hua [mailto:zhen-hual@xxxxxx]
> >
> > In benet driver, netif_device_detach and netif_device_attach should be
> > called between rtnl_lock and rtnl_unlock.
>
> Zhen, it's not clear to me why rtnl_lock is needed around netif_device_attach().
> Can you pls explain what exact data-structure you are protecting with the lock?
>
> Are you see warning stack trace? I don't see ASSERT_RTNL() called anywhere from netif_device_attach/detach()?
>
> thanks!
>
> > Signed-off-by: Li, Zhen-Hua <zhen-hual@xxxxxx>
> > ---
> >   drivers/net/ethernet/emulex/benet/be_main.c |   17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
> > b/drivers/net/ethernet/emulex/benet/be_main.c
> > index 3e6df47..9c44b3f 100644
> > --- a/drivers/net/ethernet/emulex/benet/be_main.c
> > +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> > @@ -4555,8 +4555,11 @@ static void be_func_recovery_task(struct work_struct *work)
> >   		rtnl_unlock();
> >
> >   		status = lancer_recover_func(adapter);
> > -		if (!status)
> > +		if (!status) {
> > +			rtnl_lock();
> >   			netif_device_attach(adapter->netdev);
> > +			rtnl_unlock();
> > +		}
> >   	}
> >
> >   	/* In Lancer, for all errors other than provisioning error (-EAGAIN),
> > @@ -4784,12 +4787,12 @@ static int be_suspend(struct pci_dev *pdev, pm_message_t
> > state)
> >   	be_intr_set(adapter, false);
> >   	cancel_delayed_work_sync(&adapter->func_recovery_work);
> >
> > +	rtnl_lock();
> >   	netif_device_detach(netdev);
> >   	if (netif_running(netdev)) {
> > -		rtnl_lock();
> >   		be_close(netdev);
> > -		rtnl_unlock();
> >   	}
> > +	rtnl_unlock();
> >   	be_clear(adapter);
> >
> >   	pci_save_state(pdev);
> > @@ -4804,7 +4807,9 @@ static int be_resume(struct pci_dev *pdev)
> >   	struct be_adapter *adapter = pci_get_drvdata(pdev);
> >   	struct net_device *netdev =  adapter->netdev;
> >
> > +	rtnl_lock();
> >   	netif_device_detach(netdev);
> > +	rtnl_unlock();
> >
> >   	status = pci_enable_device(pdev);
> >   	if (status)
> > @@ -4832,7 +4837,9 @@ static int be_resume(struct pci_dev *pdev)
> >
> >   	schedule_delayed_work(&adapter->func_recovery_work,
> >   			      msecs_to_jiffies(1000));
> > +	rtnl_lock();
> >   	netif_device_attach(netdev);
> > +	rtnl_unlock();
> >
> >   	if (adapter->wol_en)
> >   		be_setup_wol(adapter, false);
> > @@ -4853,7 +4860,9 @@ static void be_shutdown(struct pci_dev *pdev)
> >   	cancel_delayed_work_sync(&adapter->work);
> >   	cancel_delayed_work_sync(&adapter->func_recovery_work);
> >
> > +	rtnl_lock();
> >   	netif_device_detach(adapter->netdev);
> > +	rtnl_unlock();
> >
> >   	be_cmd_reset_function(adapter);
> >
> > @@ -4957,7 +4966,9 @@ static void be_eeh_resume(struct pci_dev *pdev)
> >
> >   	schedule_delayed_work(&adapter->func_recovery_work,
> >   			      msecs_to_jiffies(1000));
> > +	rtnl_lock();
> >   	netif_device_attach(netdev);
> > +	rtnl_unlock();
> >   	return;
> >   err:
> >   	dev_err(&adapter->pdev->dev, "EEH resume failed\n");
> > --
> > 1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/