Re: [PATCH 14/52] net: cxgb4vf: remove unnecessary pci_set_drvdata()

[Casey Leedom <leedom@chelsio.com>]

   Ah, okay.  And I presume the same is true of the PCI Remove path. I 
wasn't aware of this but if it's the Linux pattern to let the 
surrounding support code do this work, then go for it.

   As I said, I'm something of a Mad Housewife when it comes to cleaning 
things up which were allocated/assigned in a particular routine.  
Basically I program like I drive, practising defensive programming ... :-)

Casey

On 09/11/13 18:19, Jingoo Han wrote:
> On Thursday, September 12, 2013 2:24 AM, Casey Leedom wrote:
>>     I agree that the redundant pci_set_drvdata(pdev, NULL) in
>> cxgb4vf_pci_probe() under the err_release_regions: label is unneeded,
>> but don't we need to NULL out the PCI Driver Data under the
>> err_free_adapter: label and also in cxgb4vf_pci_remove()?  Or is that
>> handled automatically in the PCI infrastructure code which calls the
>> Device Probe and Remove routines?  Mostly I was just being an
>> obsessively clean housewife assuming that we'd want to clean up these
>> references ...
>
> No, 'pci_set_drvdata(pdev, NULL) under err_free_adapter label' is not
> necessary.
>
> As you know, pci_set_drvdata(pdev, NULL) calls dev_set_drvdata() as below:
> pci_set_drvdata(pdev, NULL) is dev_set_drvdata(&pdev->dev, NULL).
>
> ./include/linux/pci.h
> 1504:static inline void pci_set_drvdata(struct pci_dev *pdev, void *data)
> 1505{
> 1506	dev_set_drvdata(&pdev->dev, data);
> 1507}
>
>
> However, when the driver goes to err_free_adapter label,
> The following sequence will be done.
>      kfree(adapter) -> .... -> return -ENOMEM;
>
> In this case,
> when probe() returns error value such as '-ENOMEM',
> really_probe() of driver core automatically calls 'dev_set_drvdata(dev, NULL)'
> as below:
>
> ./drivers/base/dd.c
> 303-probe_failed:
> 304	devres_release_all(dev);
> 305	driver_sysfs_remove(dev);
> 306	dev->driver = NULL;
> 307	dev_set_drvdata(dev, NULL);
>
> Thus, without 'pci_set_drvdata(pdev, NULL) under err_free_adapter label',
> dev_set_drvdata(dev, NULL) can be called.
>
> I already tested this with other drivers such as e1000e LAN card driver.
>
> Best regards,
> Jingoo Han
>
>