* [REGRESSION] changes to driver_override parsing broke DPDK script @ 2022-08-09 18:29 Stephen Hemminger 2022-08-09 19:21 ` Bjorn Helgaas 2022-08-10 5:45 ` Greg KH 0 siblings, 2 replies; 13+ messages in thread From: Stephen Hemminger @ 2022-08-09 18:29 UTC (permalink / raw) To: Krzysztof Kozlowski, bhelgaas, gregkh; +Cc: linux-pci This commit broke the driver override script in DPDK. This is an API/ABI breakage, please revert or fix the commit. Report of problem: http://mails.dpdk.org/archives/dev/2022-August/247794.html commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Date: Tue Apr 19 13:34:28 2022 +0200 PCI: Use driver_set_override() instead of open-coding Use a helper to set driver_override to the reduce amount of duplicated code. Make the driver_override field const char, because it is not modified by the core and it matches other subsystems. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> The script is sending single nul character to remove override and that no longer works. Source code to dpdk-devbind https://github.com/DPDK/dpdk/blob/main/usertools/dpdk-devbind.py ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-09 18:29 [REGRESSION] changes to driver_override parsing broke DPDK script Stephen Hemminger @ 2022-08-09 19:21 ` Bjorn Helgaas 2022-08-10 5:54 ` Krzysztof Kozlowski 2022-08-10 5:45 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2022-08-09 19:21 UTC (permalink / raw) To: Stephen Hemminger Cc: Krzysztof Kozlowski, bhelgaas, gregkh, linux-pci, regressions [+cc regressions list] 23d99baf9d72 appeared in v5.19-rc1. On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > This commit broke the driver override script in DPDK. > This is an API/ABI breakage, please revert or fix the commit. > > Report of problem: > http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Date: Tue Apr 19 13:34:28 2022 +0200 > > PCI: Use driver_set_override() instead of open-coding > > Use a helper to set driver_override to the reduce amount of duplicated > code. Make the driver_override field const char, because it is not > modified by the core and it matches other subsystems. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > The script is sending single nul character to remove override > and that no longer works. > > Source code to dpdk-devbind > https://github.com/DPDK/dpdk/blob/main/usertools/dpdk-devbind.py ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-09 19:21 ` Bjorn Helgaas @ 2022-08-10 5:54 ` Krzysztof Kozlowski 2022-08-10 6:11 ` Greg KH ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Krzysztof Kozlowski @ 2022-08-10 5:54 UTC (permalink / raw) To: Bjorn Helgaas, Stephen Hemminger; +Cc: bhelgaas, gregkh, linux-pci, regressions On 09/08/2022 22:21, Bjorn Helgaas wrote: > [+cc regressions list] > > 23d99baf9d72 appeared in v5.19-rc1. > > On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: >> This commit broke the driver override script in DPDK. >> This is an API/ABI breakage, please revert or fix the commit. >> >> Report of problem: >> http://mails.dpdk.org/archives/dev/2022-August/247794.html Thanks for the report. I'll take a look. >> >> >> commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 >> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Date: Tue Apr 19 13:34:28 2022 +0200 >> >> PCI: Use driver_set_override() instead of open-coding >> >> Use a helper to set driver_override to the reduce amount of duplicated >> code. Make the driver_override field const char, because it is not >> modified by the core and it matches other subsystems. >> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> >> The script is sending single nul character to remove override >> and that no longer works. The sysfs API clearly states: "and may be cleared with an empty string (echo > driver_override)." Documentation/ABI/testing/sysfs-bus-pci Sending other data and expecting the same result is not conforming to API. Therefore we have usual example of some undocumented behavior which user-space started relying on and instead using API, user-space expect that undocumented behavior to be back. Yay! I wonder what is the point to even describe the ABI if user-space can simply ignore it? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 5:54 ` Krzysztof Kozlowski @ 2022-08-10 6:11 ` Greg KH 2022-08-10 8:21 ` Greg KH 2022-08-10 6:13 ` Krzysztof Kozlowski 2022-08-10 14:06 ` Stephen Hemminger 2 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2022-08-10 6:11 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Bjorn Helgaas, Stephen Hemminger, bhelgaas, linux-pci, regressions On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: > On 09/08/2022 22:21, Bjorn Helgaas wrote: > > [+cc regressions list] > > > > 23d99baf9d72 appeared in v5.19-rc1. > > > > On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > >> This commit broke the driver override script in DPDK. > >> This is an API/ABI breakage, please revert or fix the commit. > >> > >> Report of problem: > >> http://mails.dpdk.org/archives/dev/2022-August/247794.html > > Thanks for the report. I'll take a look. > > >> > >> > >> commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > >> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Date: Tue Apr 19 13:34:28 2022 +0200 > >> > >> PCI: Use driver_set_override() instead of open-coding > >> > >> Use a helper to set driver_override to the reduce amount of duplicated > >> code. Make the driver_override field const char, because it is not > >> modified by the core and it matches other subsystems. > >> > >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> > >> > >> The script is sending single nul character to remove override > >> and that no longer works. > > The sysfs API clearly states: > "and > may be cleared with an empty string (echo > driver_override)." > Documentation/ABI/testing/sysfs-bus-pci > > Sending other data and expecting the same result is not conforming to > API. Therefore we have usual example of some undocumented behavior which > user-space started relying on and instead using API, user-space expect > that undocumented behavior to be back. > > Yay! I wonder what is the point to even describe the ABI if user-space > can simply ignore it? One can argue that a string of just '\0' is an "empty string" and we should be able to properly handle this in the kernel. Heck, "\0\0\0\0\0\0" is also an "empty string", right? I don't have an issue with fixing the kernel up here, it should be able to handle this. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 6:11 ` Greg KH @ 2022-08-10 8:21 ` Greg KH 2022-08-12 1:48 ` Dongdong Liu 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2022-08-10 8:21 UTC (permalink / raw) To: Krzysztof Kozlowski, Stephen Hemminger Cc: Bjorn Helgaas, bhelgaas, linux-pci, regressions On Wed, Aug 10, 2022 at 08:11:33AM +0200, Greg KH wrote: > On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: > > On 09/08/2022 22:21, Bjorn Helgaas wrote: > > > [+cc regressions list] > > > > > > 23d99baf9d72 appeared in v5.19-rc1. > > > > > > On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > > >> This commit broke the driver override script in DPDK. > > >> This is an API/ABI breakage, please revert or fix the commit. > > >> > > >> Report of problem: > > >> http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > > Thanks for the report. I'll take a look. > > > > >> > > >> > > >> commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > > >> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > >> Date: Tue Apr 19 13:34:28 2022 +0200 > > >> > > >> PCI: Use driver_set_override() instead of open-coding > > >> > > >> Use a helper to set driver_override to the reduce amount of duplicated > > >> code. Make the driver_override field const char, because it is not > > >> modified by the core and it matches other subsystems. > > >> > > >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > >> Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > >> > > >> > > >> The script is sending single nul character to remove override > > >> and that no longer works. > > > > The sysfs API clearly states: > > "and > > may be cleared with an empty string (echo > driver_override)." > > Documentation/ABI/testing/sysfs-bus-pci > > > > Sending other data and expecting the same result is not conforming to > > API. Therefore we have usual example of some undocumented behavior which > > user-space started relying on and instead using API, user-space expect > > that undocumented behavior to be back. > > > > Yay! I wonder what is the point to even describe the ABI if user-space > > can simply ignore it? > > One can argue that a string of just '\0' is an "empty string" and we > should be able to properly handle this in the kernel. Heck, > "\0\0\0\0\0\0" is also an "empty string", right? > > I don't have an issue with fixing the kernel up here, it should be able > to handle this. Stephen, does the patch below fix this for you? thanks, greg k-h ----------------- diff --git a/drivers/base/driver.c b/drivers/base/driver.c index 15a75afe6b84..676b6275d5b5 100644 --- a/drivers/base/driver.c +++ b/drivers/base/driver.c @@ -63,6 +63,12 @@ int driver_set_override(struct device *dev, const char **override, if (len >= (PAGE_SIZE - 1)) return -EINVAL; + /* + * Compute the real length of the string in case userspace sends us a + * bunch of \0 characters like python likes to do. + */ + len = strlen(s); + if (!len) { /* Empty string passed - clear override */ device_lock(dev); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 8:21 ` Greg KH @ 2022-08-12 1:48 ` Dongdong Liu 2022-08-12 2:54 ` lihuisong (C) 0 siblings, 1 reply; 13+ messages in thread From: Dongdong Liu @ 2022-08-12 1:48 UTC (permalink / raw) To: Greg KH, Krzysztof Kozlowski, Stephen Hemminger, Huisong Li Cc: Bjorn Helgaas, bhelgaas, linux-pci, regressions cc Huisong who found the issue. On 2022/8/10 16:21, Greg KH wrote: > On Wed, Aug 10, 2022 at 08:11:33AM +0200, Greg KH wrote: >> On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: >>> On 09/08/2022 22:21, Bjorn Helgaas wrote: >>>> [+cc regressions list] >>>> >>>> 23d99baf9d72 appeared in v5.19-rc1. >>>> >>>> On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: >>>>> This commit broke the driver override script in DPDK. >>>>> This is an API/ABI breakage, please revert or fix the commit. >>>>> >>>>> Report of problem: >>>>> http://mails.dpdk.org/archives/dev/2022-August/247794.html >>> >>> Thanks for the report. I'll take a look. >>> >>>>> >>>>> >>>>> commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 >>>>> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>> Date: Tue Apr 19 13:34:28 2022 +0200 >>>>> >>>>> PCI: Use driver_set_override() instead of open-coding >>>>> >>>>> Use a helper to set driver_override to the reduce amount of duplicated >>>>> code. Make the driver_override field const char, because it is not >>>>> modified by the core and it matches other subsystems. >>>>> >>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>> Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org >>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>> >>>>> >>>>> The script is sending single nul character to remove override >>>>> and that no longer works. >>> >>> The sysfs API clearly states: >>> "and >>> may be cleared with an empty string (echo > driver_override)." >>> Documentation/ABI/testing/sysfs-bus-pci >>> >>> Sending other data and expecting the same result is not conforming to >>> API. Therefore we have usual example of some undocumented behavior which >>> user-space started relying on and instead using API, user-space expect >>> that undocumented behavior to be back. >>> >>> Yay! I wonder what is the point to even describe the ABI if user-space >>> can simply ignore it? >> >> One can argue that a string of just '\0' is an "empty string" and we >> should be able to properly handle this in the kernel. Heck, >> "\0\0\0\0\0\0" is also an "empty string", right? >> >> I don't have an issue with fixing the kernel up here, it should be able >> to handle this. > > Stephen, does the patch below fix this for you? > > thanks, > > greg k-h > > ----------------- > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 15a75afe6b84..676b6275d5b5 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -63,6 +63,12 @@ int driver_set_override(struct device *dev, const char **override, > if (len >= (PAGE_SIZE - 1)) > return -EINVAL; > > + /* > + * Compute the real length of the string in case userspace sends us a > + * bunch of \0 characters like python likes to do. > + */ > + len = strlen(s); > + > if (!len) { > /* Empty string passed - clear override */ > device_lock(dev); > . > This patch looks good, @huisong, please help to test the patch. Thanks, Dongdong ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-12 1:48 ` Dongdong Liu @ 2022-08-12 2:54 ` lihuisong (C) 2022-08-12 5:46 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: lihuisong (C) @ 2022-08-12 2:54 UTC (permalink / raw) To: Dongdong Liu, Greg KH, Krzysztof Kozlowski, Stephen Hemminger Cc: Bjorn Helgaas, bhelgaas, linux-pci, regressions 在 2022/8/12 9:48, Dongdong Liu 写道: > cc Huisong who found the issue. > > On 2022/8/10 16:21, Greg KH wrote: >> On Wed, Aug 10, 2022 at 08:11:33AM +0200, Greg KH wrote: >>> On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: >>>> On 09/08/2022 22:21, Bjorn Helgaas wrote: >>>>> [+cc regressions list] >>>>> >>>>> 23d99baf9d72 appeared in v5.19-rc1. >>>>> >>>>> On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: >>>>>> This commit broke the driver override script in DPDK. >>>>>> This is an API/ABI breakage, please revert or fix the commit. >>>>>> >>>>>> Report of problem: >>>>>> http://mails.dpdk.org/archives/dev/2022-August/247794.html >>>> >>>> Thanks for the report. I'll take a look. >>>> >>>>>> >>>>>> >>>>>> commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 >>>>>> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> Date: Tue Apr 19 13:34:28 2022 +0200 >>>>>> >>>>>> PCI: Use driver_set_override() instead of open-coding >>>>>> >>>>>> Use a helper to set driver_override to the reduce amount of >>>>>> duplicated >>>>>> code. Make the driver_override field const char, because it >>>>>> is not >>>>>> modified by the core and it matches other subsystems. >>>>>> >>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >>>>>> Signed-off-by: Krzysztof Kozlowski >>>>>> <krzysztof.kozlowski@linaro.org> >>>>>> Link: >>>>>> https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org >>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>>> >>>>>> >>>>>> The script is sending single nul character to remove override >>>>>> and that no longer works. >>>> >>>> The sysfs API clearly states: >>>> "and >>>> may be cleared with an empty string (echo > driver_override)." >>>> Documentation/ABI/testing/sysfs-bus-pci >>>> >>>> Sending other data and expecting the same result is not conforming to >>>> API. Therefore we have usual example of some undocumented behavior >>>> which >>>> user-space started relying on and instead using API, user-space expect >>>> that undocumented behavior to be back. >>>> >>>> Yay! I wonder what is the point to even describe the ABI if user-space >>>> can simply ignore it? >>> >>> One can argue that a string of just '\0' is an "empty string" and we >>> should be able to properly handle this in the kernel. Heck, >>> "\0\0\0\0\0\0" is also an "empty string", right? >>> >>> I don't have an issue with fixing the kernel up here, it should be able >>> to handle this. >> >> Stephen, does the patch below fix this for you? >> >> thanks, >> >> greg k-h >> >> ----------------- >> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c >> index 15a75afe6b84..676b6275d5b5 100644 >> --- a/drivers/base/driver.c >> +++ b/drivers/base/driver.c >> @@ -63,6 +63,12 @@ int driver_set_override(struct device *dev, const >> char **override, >> if (len >= (PAGE_SIZE - 1)) >> return -EINVAL; >> >> + /* >> + * Compute the real length of the string in case userspace sends >> us a >> + * bunch of \0 characters like python likes to do. >> + */ >> + len = strlen(s); >> + >> if (!len) { >> /* Empty string passed - clear override */ >> device_lock(dev); >> . >> > This patch looks good, @huisong, please help to test the patch. > > Thanks, > Dongdong > . Tested-by: Huisong Li <lihuisong@huawei.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-12 2:54 ` lihuisong (C) @ 2022-08-12 5:46 ` Greg KH 2022-09-01 16:41 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2022-08-12 5:46 UTC (permalink / raw) To: lihuisong (C) Cc: Dongdong Liu, Krzysztof Kozlowski, Stephen Hemminger, Bjorn Helgaas, bhelgaas, linux-pci, regressions On Fri, Aug 12, 2022 at 10:54:38AM +0800, lihuisong (C) wrote: > > 在 2022/8/12 9:48, Dongdong Liu 写道: > > cc Huisong who found the issue. > > > > On 2022/8/10 16:21, Greg KH wrote: > > > On Wed, Aug 10, 2022 at 08:11:33AM +0200, Greg KH wrote: > > > > On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: > > > > > On 09/08/2022 22:21, Bjorn Helgaas wrote: > > > > > > [+cc regressions list] > > > > > > > > > > > > 23d99baf9d72 appeared in v5.19-rc1. > > > > > > > > > > > > On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > > > > > > > This commit broke the driver override script in DPDK. > > > > > > > This is an API/ABI breakage, please revert or fix the commit. > > > > > > > > > > > > > > Report of problem: > > > > > > > http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > > > > > > > > Thanks for the report. I'll take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > > > > > > > Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > > > > Date: Tue Apr 19 13:34:28 2022 +0200 > > > > > > > > > > > > > > PCI: Use driver_set_override() instead of open-coding > > > > > > > > > > > > > > Use a helper to set driver_override to the > > > > > > > reduce amount of duplicated > > > > > > > code. Make the driver_override field const > > > > > > > char, because it is not > > > > > > > modified by the core and it matches other subsystems. > > > > > > > > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > Signed-off-by: Krzysztof Kozlowski > > > > > > > <krzysztof.kozlowski@linaro.org> > > > > > > > Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > > > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > > > > > > > > > > > > > > > The script is sending single nul character to remove override > > > > > > > and that no longer works. > > > > > > > > > > The sysfs API clearly states: > > > > > "and > > > > > may be cleared with an empty string (echo > driver_override)." > > > > > Documentation/ABI/testing/sysfs-bus-pci > > > > > > > > > > Sending other data and expecting the same result is not conforming to > > > > > API. Therefore we have usual example of some undocumented > > > > > behavior which > > > > > user-space started relying on and instead using API, user-space expect > > > > > that undocumented behavior to be back. > > > > > > > > > > Yay! I wonder what is the point to even describe the ABI if user-space > > > > > can simply ignore it? > > > > > > > > One can argue that a string of just '\0' is an "empty string" and we > > > > should be able to properly handle this in the kernel. Heck, > > > > "\0\0\0\0\0\0" is also an "empty string", right? > > > > > > > > I don't have an issue with fixing the kernel up here, it should be able > > > > to handle this. > > > > > > Stephen, does the patch below fix this for you? > > > > > > thanks, > > > > > > greg k-h > > > > > > ----------------- > > > > > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > > > index 15a75afe6b84..676b6275d5b5 100644 > > > --- a/drivers/base/driver.c > > > +++ b/drivers/base/driver.c > > > @@ -63,6 +63,12 @@ int driver_set_override(struct device *dev, const > > > char **override, > > > if (len >= (PAGE_SIZE - 1)) > > > return -EINVAL; > > > > > > + /* > > > + * Compute the real length of the string in case userspace > > > sends us a > > > + * bunch of \0 characters like python likes to do. > > > + */ > > > + len = strlen(s); > > > + > > > if (!len) { > > > /* Empty string passed - clear override */ > > > device_lock(dev); > > > . > > > > > This patch looks good, @huisong, please help to test the patch. > > > > Thanks, > > Dongdong > > . > Tested-by: Huisong Li <lihuisong@huawei.com> Wonderful, thanks! I'll queue this up to send to Linus after -rc1 is out. greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-12 5:46 ` Greg KH @ 2022-09-01 16:41 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2022-09-01 16:41 UTC (permalink / raw) To: lihuisong (C) Cc: Dongdong Liu, Krzysztof Kozlowski, Stephen Hemminger, Bjorn Helgaas, bhelgaas, linux-pci, regressions On Fri, Aug 12, 2022 at 07:46:42AM +0200, Greg KH wrote: > On Fri, Aug 12, 2022 at 10:54:38AM +0800, lihuisong (C) wrote: > > > > 在 2022/8/12 9:48, Dongdong Liu 写道: > > > cc Huisong who found the issue. > > > > > > On 2022/8/10 16:21, Greg KH wrote: > > > > On Wed, Aug 10, 2022 at 08:11:33AM +0200, Greg KH wrote: > > > > > On Wed, Aug 10, 2022 at 08:54:36AM +0300, Krzysztof Kozlowski wrote: > > > > > > On 09/08/2022 22:21, Bjorn Helgaas wrote: > > > > > > > [+cc regressions list] > > > > > > > > > > > > > > 23d99baf9d72 appeared in v5.19-rc1. > > > > > > > > > > > > > > On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > > > > > > > > This commit broke the driver override script in DPDK. > > > > > > > > This is an API/ABI breakage, please revert or fix the commit. > > > > > > > > > > > > > > > > Report of problem: > > > > > > > > http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > > > > > > > > > > Thanks for the report. I'll take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > > > > > > > > Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > > > > > Date: Tue Apr 19 13:34:28 2022 +0200 > > > > > > > > > > > > > > > > PCI: Use driver_set_override() instead of open-coding > > > > > > > > > > > > > > > > Use a helper to set driver_override to the > > > > > > > > reduce amount of duplicated > > > > > > > > code. Make the driver_override field const > > > > > > > > char, because it is not > > > > > > > > modified by the core and it matches other subsystems. > > > > > > > > > > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > > Signed-off-by: Krzysztof Kozlowski > > > > > > > > <krzysztof.kozlowski@linaro.org> > > > > > > > > Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > > > > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > > > > > > > > > > > > > > > > > > The script is sending single nul character to remove override > > > > > > > > and that no longer works. > > > > > > > > > > > > The sysfs API clearly states: > > > > > > "and > > > > > > may be cleared with an empty string (echo > driver_override)." > > > > > > Documentation/ABI/testing/sysfs-bus-pci > > > > > > > > > > > > Sending other data and expecting the same result is not conforming to > > > > > > API. Therefore we have usual example of some undocumented > > > > > > behavior which > > > > > > user-space started relying on and instead using API, user-space expect > > > > > > that undocumented behavior to be back. > > > > > > > > > > > > Yay! I wonder what is the point to even describe the ABI if user-space > > > > > > can simply ignore it? > > > > > > > > > > One can argue that a string of just '\0' is an "empty string" and we > > > > > should be able to properly handle this in the kernel. Heck, > > > > > "\0\0\0\0\0\0" is also an "empty string", right? > > > > > > > > > > I don't have an issue with fixing the kernel up here, it should be able > > > > > to handle this. > > > > > > > > Stephen, does the patch below fix this for you? > > > > > > > > thanks, > > > > > > > > greg k-h > > > > > > > > ----------------- > > > > > > > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > > > > index 15a75afe6b84..676b6275d5b5 100644 > > > > --- a/drivers/base/driver.c > > > > +++ b/drivers/base/driver.c > > > > @@ -63,6 +63,12 @@ int driver_set_override(struct device *dev, const > > > > char **override, > > > > if (len >= (PAGE_SIZE - 1)) > > > > return -EINVAL; > > > > > > > > + /* > > > > + * Compute the real length of the string in case userspace > > > > sends us a > > > > + * bunch of \0 characters like python likes to do. > > > > + */ > > > > + len = strlen(s); > > > > + > > > > if (!len) { > > > > /* Empty string passed - clear override */ > > > > device_lock(dev); > > > > . > > > > > > > This patch looks good, @huisong, please help to test the patch. > > > > > > Thanks, > > > Dongdong > > > . > > Tested-by: Huisong Li <lihuisong@huawei.com> > > Wonderful, thanks! I'll queue this up to send to Linus after -rc1 is > out. Sorry for the delay, now sent to lkml: https://lore.kernel.org/r/20220901163734.3583106-1-gregkh@linuxfoundation.org ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 5:54 ` Krzysztof Kozlowski 2022-08-10 6:11 ` Greg KH @ 2022-08-10 6:13 ` Krzysztof Kozlowski 2022-08-10 14:03 ` Stephen Hemminger 2022-08-10 14:06 ` Stephen Hemminger 2 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2022-08-10 6:13 UTC (permalink / raw) To: Bjorn Helgaas, Stephen Hemminger; +Cc: bhelgaas, gregkh, linux-pci, regressions On 10/08/2022 08:54, Krzysztof Kozlowski wrote: > On 09/08/2022 22:21, Bjorn Helgaas wrote: >> [+cc regressions list] >> >> 23d99baf9d72 appeared in v5.19-rc1. >> >> On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: >>> This commit broke the driver override script in DPDK. >>> This is an API/ABI breakage, please revert or fix the commit. >>> >>> Report of problem: >>> http://mails.dpdk.org/archives/dev/2022-August/247794.html > > Thanks for the report. I'll take a look. > I could not find in the report (neither here) steps to reproduce it. Can you provide me some short description (what kernel options are required, what commands to run)? I tried to run: $ usertools/dpdk-devbind.py --status $ usertools/dpdk-devbind.py --bind '0000:00:03.0' Error: No devices specified. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 6:13 ` Krzysztof Kozlowski @ 2022-08-10 14:03 ` Stephen Hemminger 0 siblings, 0 replies; 13+ messages in thread From: Stephen Hemminger @ 2022-08-10 14:03 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Bjorn Helgaas, bhelgaas, gregkh, linux-pci, regressions On Wed, 10 Aug 2022 09:13:40 +0300 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 10/08/2022 08:54, Krzysztof Kozlowski wrote: > > On 09/08/2022 22:21, Bjorn Helgaas wrote: > >> [+cc regressions list] > >> > >> 23d99baf9d72 appeared in v5.19-rc1. > >> > >> On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > >>> This commit broke the driver override script in DPDK. > >>> This is an API/ABI breakage, please revert or fix the commit. > >>> > >>> Report of problem: > >>> http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > > Thanks for the report. I'll take a look. > > > > I could not find in the report (neither here) steps to reproduce it. Can > you provide me some short description (what kernel options are required, > what commands to run)? > > I tried to run: > $ usertools/dpdk-devbind.py --status > $ usertools/dpdk-devbind.py --bind '0000:00:03.0' > Error: No devices specified. > > > Best regards, > Krzysztof To test, you need to be willing to have one network device disappear from kernel. The bug is in the unbind step this is an example of it working with 5.17 kernel. ~/DPDK/main $ ./usertools/dpdk-devbind.py --status Network devices using kernel driver =================================== 0000:01:00.0 'Wi-Fi 6 AX200 2723' if=wlo1 drv=iwlwifi unused= 0000:02:00.0 'RTL8125 2.5GbE Controller 8125' if=enp2s0 drv=r8169 unused= *Active* ~/DPDK/main $ sudo modprobe vfio-pci ~/DPDK/main $ sudo ./usertools/dpdk-devbind.py --bind=vfio-pci enp2s0 Warning: routing table indicates that interface 0000:02:00.0 is active. Not modifying ~/DPDK/main $ ip li set dev enp2s0 down RTNETLINK answers: Operation not permitted ~/DPDK/main $ sudo ip li set dev enp2s0 down ~/DPDK/main $ sudo ./usertools/dpdk-devbind.py --bind=vfio-pci enp2s0 ~/DPDK/main $ ./usertools/dpdk-devbind.py --status Network devices using DPDK-compatible driver ============================================ 0000:02:00.0 'RTL8125 2.5GbE Controller 8125' drv=vfio-pci unused=r8169 Network devices using kernel driver =================================== 0000:01:00.0 'Wi-Fi 6 AX200 2723' if=wlo1 drv=iwlwifi unused=vfio-pci ~/DPDK/main $ sudo ./usertools/dpdk-devbind.py -u 0000:02:00.0 ~/DPDK/main $ ./usertools/dpdk-devbind.py --status Network devices using kernel driver =================================== 0000:01:00.0 'Wi-Fi 6 AX200 2723' if=wlo1 drv=iwlwifi unused=vfio-pci Other Network devices ===================== 0000:02:00.0 'RTL8125 2.5GbE Controller 8125' unused=r8169,vfio-pci ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-10 5:54 ` Krzysztof Kozlowski 2022-08-10 6:11 ` Greg KH 2022-08-10 6:13 ` Krzysztof Kozlowski @ 2022-08-10 14:06 ` Stephen Hemminger 2 siblings, 0 replies; 13+ messages in thread From: Stephen Hemminger @ 2022-08-10 14:06 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Bjorn Helgaas, bhelgaas, gregkh, linux-pci, regressions On Wed, 10 Aug 2022 08:54:36 +0300 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> The script is sending single nul character to remove override > >> and that no longer works. > > The sysfs API clearly states: > "and > may be cleared with an empty string (echo > driver_override)." > Documentation/ABI/testing/sysfs-bus-pci > > Sending other data and expecting the same result is not conforming to > API. Therefore we have usual example of some undocumented behavior which > user-space started relying on and instead using API, user-space expect > that undocumented behavior to be back. > > Yay! I wonder what is the point to even describe the ABI if user-space > can simply ignore it? > > Best regards, > Krzysztof The code that does this in the python script is relatively old. The writing of null was introduced back in 2017 by: commit 720b7a058260dfd6ae0fcce956bfe44c18681499 Author: Guduri Prathyusha <gprathyusha@caviumnetworks.com> Date: Wed Apr 26 19:22:19 2017 +0530 usertools: fix device binding with kernel tools The following sequence of operation gives error in binding devices 1) Bind a device using dpdk-devbind.py 2) Unbind the device using kernel tools(/sys/bus/pci/device/driver/unbind) 3) Bind the device using kernel tools(/sys/bus/pci/driver/new_id and /sys/bus/pci/driver/bind) The bind failure was due to cached driver name in 'driver_override'. Fix it by writing 'null' to driver_override just after binding a device so that any method of binding/unbinding can be used. Fixes: 2fc350293570 ("usertools: use optimized driver override scheme to bind") Reported-by: Lijuan A Tu <lijuanx.a.tu@intel.com> Signed-off-by: Guduri Prathyusha <gprathyusha@caviumnetworks.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [REGRESSION] changes to driver_override parsing broke DPDK script 2022-08-09 18:29 [REGRESSION] changes to driver_override parsing broke DPDK script Stephen Hemminger 2022-08-09 19:21 ` Bjorn Helgaas @ 2022-08-10 5:45 ` Greg KH 1 sibling, 0 replies; 13+ messages in thread From: Greg KH @ 2022-08-10 5:45 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Krzysztof Kozlowski, bhelgaas, linux-pci On Tue, Aug 09, 2022 at 11:29:43AM -0700, Stephen Hemminger wrote: > This commit broke the driver override script in DPDK. > This is an API/ABI breakage, please revert or fix the commit. > > Report of problem: > http://mails.dpdk.org/archives/dev/2022-August/247794.html > > > commit 23d99baf9d729ca30b2fb6798a7b403a37bfb800 > Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Date: Tue Apr 19 13:34:28 2022 +0200 > > PCI: Use driver_set_override() instead of open-coding > > Use a helper to set driver_override to the reduce amount of duplicated > code. Make the driver_override field const char, because it is not > modified by the core and it matches other subsystems. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Link: https://lore.kernel.org/r/20220419113435.246203-6-krzysztof.kozlowski@linaro.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > The script is sending single nul character to remove override > and that no longer works. > > Source code to dpdk-devbind > https://github.com/DPDK/dpdk/blob/main/usertools/dpdk-devbind.py Ah, that's messy. I'll try to fix up driver_override() to handle a \0 being sent as the string, we should treat that as an empty write. Let me think about that later today... thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-09-01 16:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-09 18:29 [REGRESSION] changes to driver_override parsing broke DPDK script Stephen Hemminger 2022-08-09 19:21 ` Bjorn Helgaas 2022-08-10 5:54 ` Krzysztof Kozlowski 2022-08-10 6:11 ` Greg KH 2022-08-10 8:21 ` Greg KH 2022-08-12 1:48 ` Dongdong Liu 2022-08-12 2:54 ` lihuisong (C) 2022-08-12 5:46 ` Greg KH 2022-09-01 16:41 ` Greg KH 2022-08-10 6:13 ` Krzysztof Kozlowski 2022-08-10 14:03 ` Stephen Hemminger 2022-08-10 14:06 ` Stephen Hemminger 2022-08-10 5:45 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.