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@xxxxxxxxx>
> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
> 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
>
