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

Li, ZhenHua
2014-04-15T09:03:19Z

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@hp.com]
>>
>> 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@hp.com>
>> ---
>> 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
>