diff mbox series

[v6,5/6] PCI/sysfs: Only show value when driver_override is not NULL

Message ID 20210603000112.703037-6-kw@linux.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions | expand

Commit Message

Krzysztof Wilczyński June 3, 2021, 12:01 a.m. UTC 
Only expose the value of the "driver_override" variable through the
corresponding sysfs object when a value is actually set.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas June 3, 2021, 9:17 p.m. UTC | #1 
On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> Only expose the value of the "driver_override" variable through the
> corresponding sysfs object when a value is actually set.

What's the reason for this change?  The above tells me what it *does*,
but not *why* or whether it affects users.

Is this to avoid trying to print a NULL pointer as %s?  Do we print
"(null)" or something in that case now?  I assume sprintf() doesn't
actually oops.  If we change what appears in sysfs, we should mention
that here.  And maybe consider whether there's any chance of breaking
user code that might know what to do with "(null)" but not with an
empty string.

There are six other driver_override_show() methods.  Five don't check
the ->driver_override pointer at all; one (spi.c) checks like this:

  len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

Do the others need similar fixes?  Most of them still use sprintf()
also.

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci-sysfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5d63df7c1820..4e9f582ca10f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	ssize_t len;
> +	ssize_t len = 0;
>  
>  	device_lock(dev);
> -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> +	if (pdev->driver_override)
> +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
>  	device_unlock(dev);
>  	return len;
>  }
> -- 
> 2.31.1
>
Bjorn Helgaas June 3, 2021, 9:37 p.m. UTC | #2 
On Thu, Jun 03, 2021 at 04:17:41PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.
> 
> What's the reason for this change?  The above tells me what it *does*,
> but not *why* or whether it affects users.
> 
> Is this to avoid trying to print a NULL pointer as %s?  Do we print
> "(null)" or something in that case now?  I assume sprintf() doesn't
> actually oops.  If we change what appears in sysfs, we should mention
> that here.  And maybe consider whether there's any chance of breaking
> user code that might know what to do with "(null)" but not with an
> empty string.
> 
> There are six other driver_override_show() methods.  Five don't check
> the ->driver_override pointer at all; one (spi.c) checks like this:
> 
>   len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");
> 
> Do the others need similar fixes?  Most of them still use sprintf()
> also.

I can't remember if there's a reason for holding device_lock() around
this.  Of the seven: amba, platform, vmbus, pci, s390, and spi hold
it, while fsl-mc does not.

Since we're only reading a single scalar, I don't see the reason for
device_lock().  If we do need it, it would be nice to have a brief
comment explaining why.  Obviously not an issue with this patch :)

> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5d63df7c1820..4e9f582ca10f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
> >  				    struct device_attribute *attr, char *buf)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	ssize_t len;
> > +	ssize_t len = 0;
> >  
> >  	device_lock(dev);
> > -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> > +	if (pdev->driver_override)
> > +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> >  	device_unlock(dev);
> >  	return len;
> >  }
> > -- 
> > 2.31.1
> >
Krzysztof Wilczyński June 3, 2021, 10:19 p.m. UTC | #3 
Hi Bjorn,

Thank you for the review!

[...]
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.
> 
> What's the reason for this change?  The above tells me what it *does*,
> but not *why* or whether it affects users.

Good point!  I am going to update the commit message with more details
about why this change in the next version.  Sorry about this!

> Is this to avoid trying to print a NULL pointer as %s?  Do we print
> "(null)" or something in that case now?  I assume sprintf() doesn't
> actually oops.  If we change what appears in sysfs, we should mention
> that here.  And maybe consider whether there's any chance of breaking
> user code that might know what to do with "(null)" but not with an
> empty string.

Yes, sprintf() deals with a NULL pointer and prints "(null)", and
I personally think that this is not very useful, especially in the case
of this particular sysfs object - I would also prefer not to expose the
notion of a NULL-value to the userspace this way, something we ought to
handle properly, see for example how the "resource_alignment" sysfs
object behaves when there is no value set, as per:

  # wc /sys/bus/pci/resource_alignment
     0    0    0 /sys/bus/pci/resource_alignment

> There are six other driver_override_show() methods.  Five don't check
> the ->driver_override pointer at all; one (spi.c) checks like this:
> 
>   len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

For an empty sysfs object, as per the sysfs recommendations, I believe
that having 0 bytes might be more appropriate to indicate lack of any
sensible value.  Using "" (empty string) like in the example above would
still warrant adding a trailing newline character, which would make it
empty (technically), but certainly not 0 bytes.  Having said that, some
drivers opt not to add a particular sysfs object at all should there be
no value for it.

> Do the others need similar fixes?  Most of them still use sprintf()
> also.
[...]

I would like to think that we should handle NULL-value in sysfs objects
sensibly everywhere, so the other cases you were able to find could be
updated.

	Krzysztof
Bjorn Helgaas June 3, 2021, 11:23 p.m. UTC | #4 
[+cc Alex, FYI]

On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> Only expose the value of the "driver_override" variable through the
> corresponding sysfs object when a value is actually set.

This changes the attribute contents from "(null)" to an empty
(zero-length) file when no driver override has been set.

There are a few other driver_override_show() functions.  Most don't
check the pointer so they'll show "(null)".  One (spi.c) checks and
shows an empty string ("", file containing a single NULL character)
instead of an empty (zero-length) file.

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci-sysfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5d63df7c1820..4e9f582ca10f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	ssize_t len;
> +	ssize_t len = 0;
>  
>  	device_lock(dev);
> -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> +	if (pdev->driver_override)
> +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
>  	device_unlock(dev);
>  	return len;
>  }
> -- 
> 2.31.1
>
Alex Williamson June 4, 2021, 12:47 a.m. UTC | #5 
On Thu, 3 Jun 2021 18:23:34 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex, FYI]
> 
> On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.  
> 
> This changes the attribute contents from "(null)" to an empty
> (zero-length) file when no driver override has been set.
> 
> There are a few other driver_override_show() functions.  Most don't
> check the pointer so they'll show "(null)".  One (spi.c) checks and
> shows an empty string ("", file containing a single NULL character)
> instead of an empty (zero-length) file.

Yeah, "(null)" was the expected output in this case.  It looks like
this might break driverctl.  Thanks,

Alex
 
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5d63df7c1820..4e9f582ca10f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
> >  				    struct device_attribute *attr, char *buf)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	ssize_t len;
> > +	ssize_t len = 0;
> >  
> >  	device_lock(dev);
> > -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> > +	if (pdev->driver_override)
> > +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> >  	device_unlock(dev);
> >  	return len;
> >  }
> > -- 
> > 2.31.1
> >   
>
Krzysztof Wilczyński June 4, 2021, 1:10 a.m. UTC | #6 
Hi Alex and Bjorn,

[...]
> > This changes the attribute contents from "(null)" to an empty
> > (zero-length) file when no driver override has been set.
> > 
> > There are a few other driver_override_show() functions.  Most don't
> > check the pointer so they'll show "(null)".  One (spi.c) checks and
> > shows an empty string ("", file containing a single NULL character)
> > instead of an empty (zero-length) file.
> 
> Yeah, "(null)" was the expected output in this case.  It looks like
> this might break driverctl.  Thanks,
[...]

I had a look and it does, indeed, rely on explicitly checking whether
the value is "(null)", as per:

  https://gitlab.com/driverctl/driverctl/-/blob/master/driverctl#L96-131

I think, to avoid breaking this and other userspace tools that might
rely on this, we should drop this patch.

Having said that, I would love to be able to correct the behaviour we
have at the moment, but it seems that this ship has sail, so to speak.

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5d63df7c1820..4e9f582ca10f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -580,10 +580,11 @@  static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }