[PATCH] pci: use kstrtobool over ad hoc string parsing

[PATCH] pci: use kstrtobool over ad hoc string parsing

[Kenny Ballou]

Convert ROM read access enable/disable string parsing to use the
`kstrtobool` function.

This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
not match documentation.

bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301

Reported-by: googlegot@yahoo.com
Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
---
 drivers/pci/pci-sysfs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index eb6bee8724cc..3cde1f25e786 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
 {
 	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
 
-	if ((off ==  0) && (*buf == '0') && (count == 2))
-		pdev->rom_attr_enabled = 0;
-	else
+	bool res = false;
+
+	if (kstrtobool(buf, &res) == 0 && res)
 		pdev->rom_attr_enabled = 1;
+	else
+		pdev->rom_attr_enabled = 0;
 
 	return count;
 }
-- 
2.13.6

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Kenny Ballou]

On 2018年02月12日 23:04 GMT, Kenny Ballou wrote:
> Convert ROM read access enable/disable string parsing to use the
> `kstrtobool` function.
>
> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
> not match documentation.
>
> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
>
> Reported-by: googlegot@yahoo.com
> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
> ---
>  drivers/pci/pci-sysfs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index eb6bee8724cc..3cde1f25e786 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
>  {
>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>  
> -	if ((off ==  0) && (*buf == '0') && (count == 2))
> -		pdev->rom_attr_enabled = 0;
> -	else
> +	bool res = false;
> +
> +	if (kstrtobool(buf, &res) == 0 && res)
>  		pdev->rom_attr_enabled = 1;
> +	else
> +		pdev->rom_attr_enabled = 0;
>  
>  	return count;
>  }

Bjorn,

How does one know if a patch was accepted?  Should I be monitoring
patchwork?  What's the status of the quoted patch?  Patchwork doesn't
seem to show any changes for this patch.

Regards,
-Kenny

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Bjorn Helgaas]

On Mon, Feb 19, 2018 at 2:56 PM, Kenny Ballou <kballou@devnulllabs.io> wrot=
e:
>
> On 2018=E5=B9=B402=E6=9C=8812=E6=97=A5 23:04 GMT, Kenny Ballou wrote:
>> Convert ROM read access enable/disable string parsing to use the
>> `kstrtobool` function.
>>
>> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
>> not match documentation.
>>
>> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=3D111301
>>
>> Reported-by: googlegot@yahoo.com
>> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
>> ---
>>  drivers/pci/pci-sysfs.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index eb6bee8724cc..3cde1f25e786 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, =
struct kobject *kobj,
>>  {
>>       struct pci_dev *pdev =3D to_pci_dev(kobj_to_dev(kobj));
>>
>> -     if ((off =3D=3D  0) && (*buf =3D=3D '0') && (count =3D=3D 2))
>> -             pdev->rom_attr_enabled =3D 0;
>> -     else
>> +     bool res =3D false;
>> +
>> +     if (kstrtobool(buf, &res) =3D=3D 0 && res)
>>               pdev->rom_attr_enabled =3D 1;
>> +     else
>> +             pdev->rom_attr_enabled =3D 0;
>>
>>       return count;
>>  }
>
> Bjorn,
>
> How does one know if a patch was accepted?  Should I be monitoring
> patchwork?  What's the status of the quoted patch?  Patchwork doesn't
> seem to show any changes for this patch.

When I apply or review a patch, I always respond to the original email
posting of it.  So you don't need to do anything more; I just haven't
gotten to this one.  I don't see any real issues with this, although
I'll probably expand the changelog to contain a little more detail
about exactly what the problem is and how this fixes it.  The bugzilla
link is great but the changelog itself should have enough information
to justify the change.

Bjorn

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Bjorn Helgaas]

On Mon, Feb 12, 2018 at 04:04:57PM -0700, Kenny Ballou wrote:
> Convert ROM read access enable/disable string parsing to use the
> `kstrtobool` function.
> 
> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
> not match documentation.
> 
> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
> 
> Reported-by: googlegot@yahoo.com
> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
> ---
>  drivers/pci/pci-sysfs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index eb6bee8724cc..3cde1f25e786 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
>  {
>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>  
> -	if ((off ==  0) && (*buf == '0') && (count == 2))
> -		pdev->rom_attr_enabled = 0;
> -	else
> +	bool res = false;
> +
> +	if (kstrtobool(buf, &res) == 0 && res)
>  		pdev->rom_attr_enabled = 1;
> +	else
> +		pdev->rom_attr_enabled = 0;
>  
>  	return count;
>  }

I know I proposed kstrtobool().  But looking closer, I don't think
it's the right answer because:

  - kstrtobool() assumes a NULL-terminated string, and sysfs does not
    guarantee that.  This is a binary file and we can write arbitrary
    data to it.  kstrtobool_from_user() makes sure the string is
    NULL-terminated, but it does a copy_from_user() that we don't want
    in the sysfs case.

  - The current behavior is that only 2-byte writes starting with '0'
    disable the ROM, and all other writes enable it.
    
    Using kstrtobool() would enable the ROM only for writes starting
    with y/Y/1/on/oN/On/ON, and all other writes would disable it.

    That changes the behavior for most writes, e.g., writing "2"
    currently enables, but would now disable.  This feels
    unnecessarily risky because we don't know what programs are
    writing to enable the ROM.  The doc says to write "1", but the
    code comment says "anything except 0".

We *could* change the code to accept a single-byte '0' write, but I
think the simplest solution would be to change the documentation to
explicitly require a 2-byte "0\n" write to disable the ROM. 

Sorry for dithering on this.

Bjorn

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Kenny Ballou]

On 2018年02月28日 03:32 GMT, Bjorn Helgaas wrote:
> On Mon, Feb 12, 2018 at 04:04:57PM -0700, Kenny Ballou wrote:
>> Convert ROM read access enable/disable string parsing to use the
>> `kstrtobool` function.
>>
>> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
>> not match documentation.
>>
>> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
>>
>> Reported-by: googlegot@yahoo.com
>> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
>> ---
>>  drivers/pci/pci-sysfs.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index eb6bee8724cc..3cde1f25e786 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
>>  {
>>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>>
>> -	if ((off ==  0) && (*buf == '0') && (count == 2))
>> -		pdev->rom_attr_enabled = 0;
>> -	else
>> +	bool res = false;
>> +
>> +	if (kstrtobool(buf, &res) == 0 && res)
>>  		pdev->rom_attr_enabled = 1;
>> +	else
>> +		pdev->rom_attr_enabled = 0;
>>
>>  	return count;
>>  }
>
> I know I proposed kstrtobool().  But looking closer, I don't think
> it's the right answer because:
>
>   - kstrtobool() assumes a NULL-terminated string, and sysfs does not
>     guarantee that.  This is a binary file and we can write arbitrary
>     data to it.  kstrtobool_from_user() makes sure the string is
>     NULL-terminated, but it does a copy_from_user() that we don't want
>     in the sysfs case.
>
>   - The current behavior is that only 2-byte writes starting with '0'
>     disable the ROM, and all other writes enable it.
>
>     Using kstrtobool() would enable the ROM only for writes starting
>     with y/Y/1/on/oN/On/ON, and all other writes would disable it.
>
>     That changes the behavior for most writes, e.g., writing "2"
>     currently enables, but would now disable.  This feels
>     unnecessarily risky because we don't know what programs are
>     writing to enable the ROM.  The doc says to write "1", but the
>     code comment says "anything except 0".
>
> We *could* change the code to accept a single-byte '0' write, but I
> think the simplest solution would be to change the documentation to
> explicitly require a 2-byte "0\n" write to disable the ROM.
>
> Sorry for dithering on this.
>
> Bjorn

I was not under the impression that this particular device needed data
written to it(?).  If it is the case that arbitrary data needs to be
written, your argument makes sense, and I can put together a patch for
the documentation instead.

-Kenny

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Bjorn Helgaas]

On Fri, Mar 02, 2018 at 04:47:16PM -0700, Kenny Ballou wrote:
> 
> On 2018年02月28日 03:32 GMT, Bjorn Helgaas wrote:
> > On Mon, Feb 12, 2018 at 04:04:57PM -0700, Kenny Ballou wrote:
> >> Convert ROM read access enable/disable string parsing to use the
> >> `kstrtobool` function.
> >>
> >> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
> >> not match documentation.
> >>
> >> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
> >>
> >> Reported-by: googlegot@yahoo.com
> >> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
> >> ---
> >>  drivers/pci/pci-sysfs.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> index eb6bee8724cc..3cde1f25e786 100644
> >> --- a/drivers/pci/pci-sysfs.c
> >> +++ b/drivers/pci/pci-sysfs.c
> >> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
> >>  {
> >>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> >>
> >> -	if ((off ==  0) && (*buf == '0') && (count == 2))
> >> -		pdev->rom_attr_enabled = 0;
> >> -	else
> >> +	bool res = false;
> >> +
> >> +	if (kstrtobool(buf, &res) == 0 && res)
> >>  		pdev->rom_attr_enabled = 1;
> >> +	else
> >> +		pdev->rom_attr_enabled = 0;
> >>
> >>  	return count;
> >>  }
> >
> > I know I proposed kstrtobool().  But looking closer, I don't think
> > it's the right answer because:
> >
> >   - kstrtobool() assumes a NULL-terminated string, and sysfs does not
> >     guarantee that.  This is a binary file and we can write arbitrary
> >     data to it.  kstrtobool_from_user() makes sure the string is
> >     NULL-terminated, but it does a copy_from_user() that we don't want
> >     in the sysfs case.
> >
> >   - The current behavior is that only 2-byte writes starting with '0'
> >     disable the ROM, and all other writes enable it.
> >
> >     Using kstrtobool() would enable the ROM only for writes starting
> >     with y/Y/1/on/oN/On/ON, and all other writes would disable it.
> >
> >     That changes the behavior for most writes, e.g., writing "2"
> >     currently enables, but would now disable.  This feels
> >     unnecessarily risky because we don't know what programs are
> >     writing to enable the ROM.  The doc says to write "1", but the
> >     code comment says "anything except 0".
> >
> > We *could* change the code to accept a single-byte '0' write, but I
> > think the simplest solution would be to change the documentation to
> > explicitly require a 2-byte "0\n" write to disable the ROM.
> >
> > Sorry for dithering on this.
> >
> > Bjorn
> 
> I was not under the impression that this particular device needed data
> written to it(?).  If it is the case that arbitrary data needs to be
> written, your argument makes sense, and I can put together a patch for
> the documentation instead.

I'm not sure I understand your question, but here's a stab at
answering it:

The sysfs "rom" file has a read method (pci_read_rom()) and a write
method (pci_write_rom()).  User-space software can open the file and
do a read() system call to read the contents of the ROM from the
device, so pci_read_rom() is directly connected to the PCI hardware
device.

The write method doesn't touch the hardware device at all; it's only a
software switch, and the data written to the sysfs "rom" file is never
written to the PCI hardware device at all.  It doesn't need any
particular data written to it.  The data can be whatever we decide it
should be, and we can interpret that data in any way we like since
it's purely a software construct.  

However, there is user-space software that assumes certain behavior
(enabling/disabling read access to the ROM).  We don't want to break
that software.  Using kstrtobool() would change the interpretation of
some writes.  We *could* assume that user-space software always writes
"1" to enable reads of the ROM and "0" to disable reads, but it's
impossible to be certain of that.

So the safest thing in terms of not breaking the ABI is to leave the
code unchanged but update the documentation.  Does that make sense?

Bjorn

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Kenny Ballou]

On 2018年03月05日 14:28 GMT, Bjorn Helgaas wrote:
> On Fri, Mar 02, 2018 at 04:47:16PM -0700, Kenny Ballou wrote:
>>
>> On 2018年02月28日 03:32 GMT, Bjorn Helgaas wrote:
>> > On Mon, Feb 12, 2018 at 04:04:57PM -0700, Kenny Ballou wrote:
>> >> Convert ROM read access enable/disable string parsing to use the
>> >> `kstrtobool` function.
>> >>
>> >> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
>> >> not match documentation.
>> >>
>> >> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
>> >>
>> >> Reported-by: googlegot@yahoo.com
>> >> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
>> >> ---
>> >>  drivers/pci/pci-sysfs.c | 8 +++++---
>> >>  1 file changed, 5 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> >> index eb6bee8724cc..3cde1f25e786 100644
>> >> --- a/drivers/pci/pci-sysfs.c
>> >> +++ b/drivers/pci/pci-sysfs.c
>> >> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
>> >>  {
>> >>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>> >>
>> >> -	if ((off ==  0) && (*buf == '0') && (count == 2))
>> >> -		pdev->rom_attr_enabled = 0;
>> >> -	else
>> >> +	bool res = false;
>> >> +
>> >> +	if (kstrtobool(buf, &res) == 0 && res)
>> >>  		pdev->rom_attr_enabled = 1;
>> >> +	else
>> >> +		pdev->rom_attr_enabled = 0;
>> >>
>> >>  	return count;
>> >>  }
>> >
>> > I know I proposed kstrtobool().  But looking closer, I don't think
>> > it's the right answer because:
>> >
>> >   - kstrtobool() assumes a NULL-terminated string, and sysfs does not
>> >     guarantee that.  This is a binary file and we can write arbitrary
>> >     data to it.  kstrtobool_from_user() makes sure the string is
>> >     NULL-terminated, but it does a copy_from_user() that we don't want
>> >     in the sysfs case.
>> >
>> >   - The current behavior is that only 2-byte writes starting with '0'
>> >     disable the ROM, and all other writes enable it.
>> >
>> >     Using kstrtobool() would enable the ROM only for writes starting
>> >     with y/Y/1/on/oN/On/ON, and all other writes would disable it.
>> >
>> >     That changes the behavior for most writes, e.g., writing "2"
>> >     currently enables, but would now disable.  This feels
>> >     unnecessarily risky because we don't know what programs are
>> >     writing to enable the ROM.  The doc says to write "1", but the
>> >     code comment says "anything except 0".
>> >
>> > We *could* change the code to accept a single-byte '0' write, but I
>> > think the simplest solution would be to change the documentation to
>> > explicitly require a 2-byte "0\n" write to disable the ROM.
>> >
>> > Sorry for dithering on this.
>> >
>> > Bjorn
>>
>> I was not under the impression that this particular device needed data
>> written to it(?).  If it is the case that arbitrary data needs to be
>> written, your argument makes sense, and I can put together a patch for
>> the documentation instead.
>
> I'm not sure I understand your question, but here's a stab at
> answering it:
>
> The sysfs "rom" file has a read method (pci_read_rom()) and a write
> method (pci_write_rom()).  User-space software can open the file and
> do a read() system call to read the contents of the ROM from the
> device, so pci_read_rom() is directly connected to the PCI hardware
> device.
>
> The write method doesn't touch the hardware device at all; it's only a
> software switch, and the data written to the sysfs "rom" file is never
> written to the PCI hardware device at all.  It doesn't need any
> particular data written to it.  The data can be whatever we decide it
> should be, and we can interpret that data in any way we like since
> it's purely a software construct.
>
> However, there is user-space software that assumes certain behavior
> (enabling/disabling read access to the ROM).  We don't want to break
> that software.  Using kstrtobool() would change the interpretation of
> some writes.  We *could* assume that user-space software always writes
> "1" to enable reads of the ROM and "0" to disable reads, but it's
> impossible to be certain of that.
>
> So the safest thing in terms of not breaking the ABI is to leave the
> code unchanged but update the documentation.  Does that make sense?
>
> Bjorn

Perfectly clear.  Thank you for the explanation.  That confirms that I
was thinking about it correctly, but your desire to leave it unchanged
had me thinking I didn't understand how this all worked.

I, however, was not thinking about the broader context where such a
change is likely to break existing behaviour.  Thank you for keeping
that in mind.

The off-topic question would then be: how would one go about changing it
so that it's using a more consolidated string library, if that's at all
desired.  Would that be something where we have both code paths, the old
input handler and the `kstrtobool()` code, reporting warnings when they
differ in interpretation of user input?  I'm not suggesting it, but I'm
trying to understand how something like that _could_ change.

Thank you,
-Kenny

Re: [PATCH] pci: use kstrtobool over ad hoc string parsing

[Bjorn Helgaas]

On Mon, Mar 05, 2018 at 08:15:04AM -0700, Kenny Ballou wrote:
> 
> On 2018年03月05日 14:28 GMT, Bjorn Helgaas wrote:
> > On Fri, Mar 02, 2018 at 04:47:16PM -0700, Kenny Ballou wrote:
> >>
> >> On 2018年02月28日 03:32 GMT, Bjorn Helgaas wrote:
> >> > On Mon, Feb 12, 2018 at 04:04:57PM -0700, Kenny Ballou wrote:
> >> >> Convert ROM read access enable/disable string parsing to use the
> >> >> `kstrtobool` function.
> >> >>
> >> >> This fixes Bugzilla Bug 111301 -- Sysfs PCI rom file functionality does
> >> >> not match documentation.
> >> >>
> >> >> bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111301
> >> >>
> >> >> Reported-by: googlegot@yahoo.com
> >> >> Signed-off-by: Kenny Ballou <kballou@devnulllabs.io>
> >> >> ---
> >> >>  drivers/pci/pci-sysfs.c | 8 +++++---
> >> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> >> index eb6bee8724cc..3cde1f25e786 100644
> >> >> --- a/drivers/pci/pci-sysfs.c
> >> >> +++ b/drivers/pci/pci-sysfs.c
> >> >> @@ -1424,10 +1424,12 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
> >> >>  {
> >> >>  	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> >> >>
> >> >> -	if ((off ==  0) && (*buf == '0') && (count == 2))
> >> >> -		pdev->rom_attr_enabled = 0;
> >> >> -	else
> >> >> +	bool res = false;
> >> >> +
> >> >> +	if (kstrtobool(buf, &res) == 0 && res)
> >> >>  		pdev->rom_attr_enabled = 1;
> >> >> +	else
> >> >> +		pdev->rom_attr_enabled = 0;
> >> >>
> >> >>  	return count;
> >> >>  }
> >> >
> >> > I know I proposed kstrtobool().  But looking closer, I don't think
> >> > it's the right answer because:
> >> >
> >> >   - kstrtobool() assumes a NULL-terminated string, and sysfs does not
> >> >     guarantee that.  This is a binary file and we can write arbitrary
> >> >     data to it.  kstrtobool_from_user() makes sure the string is
> >> >     NULL-terminated, but it does a copy_from_user() that we don't want
> >> >     in the sysfs case.
> >> >
> >> >   - The current behavior is that only 2-byte writes starting with '0'
> >> >     disable the ROM, and all other writes enable it.
> >> >
> >> >     Using kstrtobool() would enable the ROM only for writes starting
> >> >     with y/Y/1/on/oN/On/ON, and all other writes would disable it.
> >> >
> >> >     That changes the behavior for most writes, e.g., writing "2"
> >> >     currently enables, but would now disable.  This feels
> >> >     unnecessarily risky because we don't know what programs are
> >> >     writing to enable the ROM.  The doc says to write "1", but the
> >> >     code comment says "anything except 0".
> >> >
> >> > We *could* change the code to accept a single-byte '0' write, but I
> >> > think the simplest solution would be to change the documentation to
> >> > explicitly require a 2-byte "0\n" write to disable the ROM.
> >> >
> >> > Sorry for dithering on this.
> >> >
> >> > Bjorn
> >>
> >> I was not under the impression that this particular device needed data
> >> written to it(?).  If it is the case that arbitrary data needs to be
> >> written, your argument makes sense, and I can put together a patch for
> >> the documentation instead.
> >
> > I'm not sure I understand your question, but here's a stab at
> > answering it:
> >
> > The sysfs "rom" file has a read method (pci_read_rom()) and a write
> > method (pci_write_rom()).  User-space software can open the file and
> > do a read() system call to read the contents of the ROM from the
> > device, so pci_read_rom() is directly connected to the PCI hardware
> > device.
> >
> > The write method doesn't touch the hardware device at all; it's only a
> > software switch, and the data written to the sysfs "rom" file is never
> > written to the PCI hardware device at all.  It doesn't need any
> > particular data written to it.  The data can be whatever we decide it
> > should be, and we can interpret that data in any way we like since
> > it's purely a software construct.
> >
> > However, there is user-space software that assumes certain behavior
> > (enabling/disabling read access to the ROM).  We don't want to break
> > that software.  Using kstrtobool() would change the interpretation of
> > some writes.  We *could* assume that user-space software always writes
> > "1" to enable reads of the ROM and "0" to disable reads, but it's
> > impossible to be certain of that.
> >
> > So the safest thing in terms of not breaking the ABI is to leave the
> > code unchanged but update the documentation.  Does that make sense?
> >
> > Bjorn
> 
> Perfectly clear.  Thank you for the explanation.  That confirms that I
> was thinking about it correctly, but your desire to leave it unchanged
> had me thinking I didn't understand how this all worked.
> 
> I, however, was not thinking about the broader context where such a
> change is likely to break existing behaviour.  Thank you for keeping
> that in mind.
> 
> The off-topic question would then be: how would one go about changing it
> so that it's using a more consolidated string library, if that's at all
> desired.  Would that be something where we have both code paths, the old
> input handler and the `kstrtobool()` code, reporting warnings when they
> differ in interpretation of user input?  I'm not suggesting it, but I'm
> trying to understand how something like that _could_ change.

It's conceivable we could do something like emit warnings about a
change in interpretation, and then after a few years, make the change
permanent.  In this particular case, I don't think it's worth the
trouble.

If making the change fixed a security issue, we would just do it
immediately and accept the fact that some user applications might
break.  But I think the existing code is safe.

Bjorn