| Message ID | 20220223191310.347669-6-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix broken usage of driver_override (and kfree of static memory) | expand |
In subject, to match drivers/pci/ convention, do something like: PCI: Use driver_set_override() instead of open-coding On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote: > Use a helper for seting driver_override to reduce amount of duplicated > code. s/seting/setting/ > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > drivers/pci/pci-sysfs.c | 24 ++++-------------------- > 1 file changed, 4 insertions(+), 20 deletions(-) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 602f0fb0b007..16a163d4623e 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, > const char *buf, size_t count) > { > struct pci_dev *pdev = to_pci_dev(dev); > - char *driver_override, *old, *cp; > + int ret; > > /* We need to keep extra room for a newline */ > if (count >= (PAGE_SIZE - 1)) > return -EINVAL; This check makes no sense in the new function. Michael alluded to this as well. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > -- > 2.32.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 23/02/2022 22:51, Bjorn Helgaas wrote: > In subject, to match drivers/pci/ convention, do something like: > > PCI: Use driver_set_override() instead of open-coding > > On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote: >> Use a helper for seting driver_override to reduce amount of duplicated >> code. > > s/seting/setting/ > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >> --- >> drivers/pci/pci-sysfs.c | 24 ++++-------------------- >> 1 file changed, 4 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c >> index 602f0fb0b007..16a163d4623e 100644 >> --- a/drivers/pci/pci-sysfs.c >> +++ b/drivers/pci/pci-sysfs.c >> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, >> const char *buf, size_t count) >> { >> struct pci_dev *pdev = to_pci_dev(dev); >> - char *driver_override, *old, *cp; >> + int ret; >> >> /* We need to keep extra room for a newline */ >> if (count >= (PAGE_SIZE - 1)) >> return -EINVAL; > > This check makes no sense in the new function. Michael alluded to > this as well. I am not sure if I got your comment properly. You mean here: 1. Move this check to driver_set_override()? 2. Remove the check entirely? > >> - driver_override = kstrndup(buf, count, GFP_KERNEL); >> - if (!driver_override) >> - return -ENOMEM; >> - >> - cp = strchr(driver_override, '\n'); >> - if (cp) >> - *cp = '\0'; >> - >> - device_lock(dev); >> - old = pdev->driver_override; >> - if (strlen(driver_override)) { >> - pdev->driver_override = driver_override; >> - } else { >> - kfree(driver_override); >> - pdev->driver_override = NULL; >> - } >> - device_unlock(dev); >> - >> - kfree(old); >> + ret = driver_set_override(dev, &pdev->driver_override, buf); >> + if (ret) >> + return ret; >> >> return count; >> } >> -- >> 2.32.0 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Best regards, Krzysztof
On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote: > On 23/02/2022 22:51, Bjorn Helgaas wrote: > > In subject, to match drivers/pci/ convention, do something like: > > > > PCI: Use driver_set_override() instead of open-coding > > > > On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote: > >> Use a helper for seting driver_override to reduce amount of duplicated > >> code. > >> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, > >> const char *buf, size_t count) > >> { > >> struct pci_dev *pdev = to_pci_dev(dev); > >> - char *driver_override, *old, *cp; > >> + int ret; > >> > >> /* We need to keep extra room for a newline */ > >> if (count >= (PAGE_SIZE - 1)) > >> return -EINVAL; > > > > This check makes no sense in the new function. Michael alluded to > > this as well. > > I am not sure if I got your comment properly. You mean here: > 1. Move this check to driver_set_override()? > 2. Remove the check entirely? I was mistaken about the purpose of the comment and the check. I thought it had to do with *this* function, and this function doesn't add a newline, and there's no obvious connection with PAGE_SIZE. But looking closer, I think the "extra room for a newline" is really to make sure that *driver_override_show()* can add a newline and have it still fit within the PAGE_SIZE sysfs limit. Most driver_override_*() functions have the same comment, so maybe this was obvious to everybody except me :) I do see that spi.c adds "when displaying value" at the end, which helps a lot. Sorry for the wild goose chase. > >> - driver_override = kstrndup(buf, count, GFP_KERNEL); > >> - if (!driver_override) > >> - return -ENOMEM; > >> - > >> - cp = strchr(driver_override, '\n'); > >> - if (cp) > >> - *cp = '\0'; > >> - > >> - device_lock(dev); > >> - old = pdev->driver_override; > >> - if (strlen(driver_override)) { > >> - pdev->driver_override = driver_override; > >> - } else { > >> - kfree(driver_override); > >> - pdev->driver_override = NULL; > >> - } > >> - device_unlock(dev); > >> - > >> - kfree(old); > >> + ret = driver_set_override(dev, &pdev->driver_override, buf); > >> + if (ret) > >> + return ret; > >> > >> return count; > >> } > >> -- > >> 2.32.0 > >> > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > Best regards, > Krzysztof > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 25/02/2022 00:52, Bjorn Helgaas wrote: > On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote: >> On 23/02/2022 22:51, Bjorn Helgaas wrote: >>> In subject, to match drivers/pci/ convention, do something like: >>> >>> PCI: Use driver_set_override() instead of open-coding >>> >>> On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote: >>>> Use a helper for seting driver_override to reduce amount of duplicated >>>> code. >>>> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, >>>> const char *buf, size_t count) >>>> { >>>> struct pci_dev *pdev = to_pci_dev(dev); >>>> - char *driver_override, *old, *cp; >>>> + int ret; >>>> >>>> /* We need to keep extra room for a newline */ >>>> if (count >= (PAGE_SIZE - 1)) >>>> return -EINVAL; >>> >>> This check makes no sense in the new function. Michael alluded to >>> this as well. >> >> I am not sure if I got your comment properly. You mean here: >> 1. Move this check to driver_set_override()? >> 2. Remove the check entirely? > > I was mistaken about the purpose of the comment and the check. I > thought it had to do with *this* function, and this function doesn't > add a newline, and there's no obvious connection with PAGE_SIZE. > > But looking closer, I think the "extra room for a newline" is really > to make sure that *driver_override_show()* can add a newline and have > it still fit within the PAGE_SIZE sysfs limit. > > Most driver_override_*() functions have the same comment, so maybe > this was obvious to everybody except me :) I do see that spi.c adds > "when displaying value" at the end, which helps a lot. > > Sorry for the wild goose chase. I think I will move this check anyway to driver_set_override() helper, because there is no particular benefit to have duplicated all over. The helper will receive "count" argument so can perform all checks. Best regards, Krzysztof
On Fri, Feb 25, 2022 at 10:36:20AM +0100, Krzysztof Kozlowski wrote: > On 25/02/2022 00:52, Bjorn Helgaas wrote: > > On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote: > >> On 23/02/2022 22:51, Bjorn Helgaas wrote: > >>> In subject, to match drivers/pci/ convention, do something like: > >>> > >>> PCI: Use driver_set_override() instead of open-coding > >>> > >>> On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote: > >>>> Use a helper for seting driver_override to reduce amount of duplicated > >>>> code. > >>>> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, > >>>> const char *buf, size_t count) > >>>> { > >>>> struct pci_dev *pdev = to_pci_dev(dev); > >>>> - char *driver_override, *old, *cp; > >>>> + int ret; > >>>> > >>>> /* We need to keep extra room for a newline */ > >>>> if (count >= (PAGE_SIZE - 1)) > >>>> return -EINVAL; > >>> > >>> This check makes no sense in the new function. Michael alluded to > >>> this as well. > >> > >> I am not sure if I got your comment properly. You mean here: > >> 1. Move this check to driver_set_override()? > >> 2. Remove the check entirely? > > > > I was mistaken about the purpose of the comment and the check. I > > thought it had to do with *this* function, and this function doesn't > > add a newline, and there's no obvious connection with PAGE_SIZE. > > > > But looking closer, I think the "extra room for a newline" is really > > to make sure that *driver_override_show()* can add a newline and have > > it still fit within the PAGE_SIZE sysfs limit. > > > > Most driver_override_*() functions have the same comment, so maybe > > this was obvious to everybody except me :) I do see that spi.c adds > > "when displaying value" at the end, which helps a lot. > > > > Sorry for the wild goose chase. > > I think I will move this check anyway to driver_set_override() helper, > because there is no particular benefit to have duplicated all over. The > helper will receive "count" argument so can perform all checks. Thanks, I think that would be good! Bjorn
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 602f0fb0b007..16a163d4623e 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) { struct pci_dev *pdev = to_pci_dev(dev); - char *driver_override, *old, *cp; + int ret; /* We need to keep extra room for a newline */ if (count >= (PAGE_SIZE - 1)) return -EINVAL; - driver_override = kstrndup(buf, count, GFP_KERNEL); - if (!driver_override) - return -ENOMEM; - - cp = strchr(driver_override, '\n'); - if (cp) - *cp = '\0'; - - device_lock(dev); - old = pdev->driver_override; - if (strlen(driver_override)) { - pdev->driver_override = driver_override; - } else { - kfree(driver_override); - pdev->driver_override = NULL; - } - device_unlock(dev); - - kfree(old); + ret = driver_set_override(dev, &pdev->driver_override, buf); + if (ret) + return ret; return count; }
Use a helper for seting driver_override to reduce amount of duplicated code. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- drivers/pci/pci-sysfs.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)