Re: [PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.
On Tue, May 21, 2019 at 02:23:13PM +0300, Dan Carpenter wrote:
> On Fri, May 17, 2019 at 01:54:51PM +0200, Greg KH wrote:
> > On Fri, May 17, 2019 at 12:03:12PM +0100, Jeremy Sowden wrote:
> > > 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 :(
> >
>
> It looks sort of weird but it's fine.
>
> > 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?
>
> Yes. The pci_get_drvdata() is only set to NULL after we remove the
> sysfs files so pci_get_drvdata() always returns a valid pointer.
Ugh, I am wrong, it's not as if I didn't actually write the
dev_get_drvdata() and pci_get_drvdata() code 15+ years ago, you would
think I would have remembered something like this :(
Anyway, patches look good, sorry for the noise...
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.
On Fri, May 17, 2019 at 01:54:51PM +0200, Greg KH wrote:
> On Fri, May 17, 2019 at 12:03:12PM +0100, Jeremy Sowden wrote:
> > 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 :(
>
It looks sort of weird but it's fine.
> 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?
Yes. The pci_get_drvdata() is only set to NULL after we remove the
sysfs files so pci_get_drvdata() always returns a valid pointer.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.
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
> > ---
> > 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(>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.
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.
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
> ---
> 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 :(
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?
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 3/6] staging: kpc2000: simplified kp2000_device retrieval in device attributes call-backs.
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
---
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);
if (strcmp("ssid", attr->attr.name) == 0){ return scnprintf(buf,
PAGE_SIZE, "%016llx\n", pcard->ssid); } else
if (strcmp("ddna", attr->attr.name) == 0){ return scnprintf(buf,
PAGE_SIZE, "%016llx\n", pcard->ddna); } else
@@ -43,31 +38,18 @@ static ssize_t show_attr(struct device *dev, struct
device_attribute *attr, cha
static ssize_t show_cpld_config_reg(struct device *dev, struct
device_attribute *attr, char *buf)
{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct kp2000_device *pcard;
+ struct kp2000_device *pcard = dev_get_drvdata(dev);
u64 val;
- if (!pdev)
- return -ENXIO;
-
- pcard = pci_get_drvdata(pdev);
- if (!pcard)
- return -ENXIO;
-
val = readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);
return scnprintf(buf, PAGE_SIZE, "%016llx\n", val);
}
static ssize_t cpld_reconfigure(struct device *dev, struct device_attribute
*attr, const char *buf, size_t count)
{
-struct pci_dev *pdev = to_pci_dev(dev);
+struct kp2000_device *pcard = dev_get_drvdata(dev);
long wr_val;
-struct kp2000_device *pcard;
int rv;
-if (!pdev) return -ENXIO;
-pcard = pci_get_drvdata(pdev);
-if (!pcard) return -ENXIO;
-
rv = kstrtol(buf, 0, _val);
if (rv < 0) return rv;
if (wr_val > 7) return -EINVAL;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
