[PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.

Jeremy Sowden jeremy at azazel.net
Fri May 17 12:18:11 UTC 2019


On 2019-05-17, at 13:54:51 +0200, Greg KH wrote:
> On Fri, May 17, 2019 at 12:03:12PM +0100, Jeremy Sowden wrote:
> > The call-backs used the same recipe to get the pcard from dev:
> >
> >   struct pci_dev *pdev = to_pci_dev(dev);
> >   struct kp2000_device *pcard;
> >
> >   if (!pdev) return -ENXIO;
> >   pcard = pci_get_drvdata(pdev);
> >   if (!pcard) return -ENXIO;
> >
> > where to_pci_dev is a wrapper for container_of.
> >
> > However, pci_set_drvdata is called before the sysfs files are
> > created:
> >
> >   int  kp2000_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >   {
> >     // ...
> >
> >     pcard = kzalloc(sizeof(struct kp2000_device), GFP_KERNEL);
> >
> >     // ...
> >
> >     pcard->pdev = pdev;
> >     pci_set_drvdata(pdev, pcard);
> >
> >     // ...
> >
> >     err = sysfs_create_files(&(pdev->dev.kobj), kp_attr_list);
> >
> > Therefore, to_pci_dev and pci_get_drvdata cannot return NULL, and
> > pcard can be initialized directly from dev:
> >
> >   struct kp2000_device *pcard = dev_get_drvdata(dev);
> >
> > Signed-off-by: Jeremy Sowden <jeremy at azazel.net>
> > ---
> >  drivers/staging/kpc2000/kpc2000/core.c | 24 +++---------------------
> >  1 file changed, 3 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
> > index eb8bac62d33d..9425c4dbc2f2 100644
> > --- a/drivers/staging/kpc2000/kpc2000/core.c
> > +++ b/drivers/staging/kpc2000/kpc2000/core.c
> > @@ -24,12 +24,7 @@
> >    ******************************************************/
> >  static ssize_t  show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> > -    struct pci_dev *pdev = to_pci_dev(dev);
> > -    struct kp2000_device *pcard;
> > -
> > -    if (!pdev)  return -ENXIO;
> > -    pcard = pci_get_drvdata(pdev);
> > -    if (!pcard)  return -ENXIO;
> > +    struct kp2000_device *pcard = dev_get_drvdata(dev);
>
> Wait, dev_get_drvdata() is not returning you the same pointer that
> pci_get_drvdata() does.  So I think this is now broken :(

I'm confused.  Perhaps I'm looking at the wrong code.

Here are pci_get_drvdata:

  static inline void *pci_get_drvdata(struct pci_dev *pdev)
  {
          return dev_get_drvdata(&pdev->dev);
  }

and dev_get_drvdata:

  static inline void *dev_get_drvdata(const struct device *dev)
  {
          return dev->driver_data;
  }

Starting withing with dev and using to_pci_dev, we have:

  pci_get_drvdata(to_pci_dev(dev))

which is the same as:

  dev_get_drvdata(&(to_pci_dev(dev)->dev)

which is the same as:

  dev_get_drvdata(dev)

isn't it?

> What this should look like is this:
> 	struct pci_dev *pdev = to_pci_dev(dev);
> 	struct kp200_device *pcard = pci_get_drvdata(pdev);
>
> 	if (!pcard)
> 		return -ENODEV;
>
> that is IF the driver really is setting the pci dev data to NULL when
> the device is removed from the driver.  Is it?

It sets it to NULL after removing the sysfs files and disabling the
device:

    lock_card(pcard);
    // ...
    sysfs_remove_files(&(pdev->dev.kobj), kp_attr_list);
    // ...
    pci_disable_device(pcard->pdev);
    pci_set_drvdata(pdev, NULL);
    unlock_card(pcard);
    kfree(pcard);

Can the show functions get called after pci_set_drvdata is called?

J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20190517/b99d8322/attachment.asc>


More information about the devel mailing list