[REGRESSION] changes to driver_override parsing broke DPDK script

[REGRESSION] changes to driver_override parsing broke DPDK script

[Stephen Hemminger]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Bjorn Helgaas]

[+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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Krzysztof Kozlowski]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Greg KH]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Greg KH]

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);

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[lihuisong (C)]

在 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>

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Greg KH]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Greg KH]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Krzysztof Kozlowski]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Stephen Hemminger]

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

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Stephen Hemminger]

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>

Re: [REGRESSION] changes to driver_override parsing broke DPDK script

[Greg KH]

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