[net-next,6/6] amd-xgbe: Resolve checkpatch warning about sscanf usage

Checkpatch issued a warning preferring to use kstrto<type> when
using a single variable sscanf.  Change the sscanf invocation to
a kstrtouint call.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
> Checkpatch issued a warning preferring to use kstrto<type> when
> using a single variable sscanf.  Change the sscanf invocation to
> a kstrtouint call.
[]
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
[]
> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
>  		return len;
>  
>  	workarea[len] = '\0';
> -	if (sscanf(workarea, "%x", &scan_value) == 1)
> -		*value = scan_value;
> -	else
> -		return -EIO;
> +	ret = kstrtouint(workarea, 0, value);

Don't you need to use 16 for the base here?

> +	if (ret)
> +		return ret;

Are there any issues with any of the various callers
getting a different error return?

-EINVAL/-ERANGE vs -EIO ?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/24/2014 05:00 PM, Joe Perches wrote:
> On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
>> Checkpatch issued a warning preferring to use kstrto<type> when
>> using a single variable sscanf.  Change the sscanf invocation to
>> a kstrtouint call.
> []
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
> []
>> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
>>   		return len;
>>
>>   	workarea[len] = '\0';
>> -	if (sscanf(workarea, "%x", &scan_value) == 1)
>> -		*value = scan_value;
>> -	else
>> -		return -EIO;
>> +	ret = kstrtouint(workarea, 0, value);
>
> Don't you need to use 16 for the base here?

Using 0 allows for greater flexibility in the input format.

>
>> +	if (ret)
>> +		return ret;
>
> Are there any issues with any of the various callers
> getting a different error return?
>
> -EINVAL/-ERANGE vs -EIO ?
>

There shouldn't be, but I can always return -EIO to be
consistent with how it was previously.

Tom
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2014-06-24 at 17:44 -0500, Tom Lendacky wrote:
> On 06/24/2014 05:00 PM, Joe Perches wrote:
> > On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
> >> Checkpatch issued a warning preferring to use kstrto<type> when
> >> using a single variable sscanf.  Change the sscanf invocation to
> >> a kstrtouint call.
> > []
> >> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
> > []
> >> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
> >>   		return len;
> >>
> >>   	workarea[len] = '\0';
> >> -	if (sscanf(workarea, "%x", &scan_value) == 1)
> >> -		*value = scan_value;
> >> -	else
> >> -		return -EIO;
> >> +	ret = kstrtouint(workarea, 0, value);
> >
> > Don't you need to use 16 for the base here?

> Using 0 allows for greater flexibility in the input format.

True, but there could be a change in behavior like reading a
previously hex value like 10 is now a decimal 10 not decimal 16.

> > Are there any issues with any of the various callers
> > getting a different error return?
> >
> > -EINVAL/-ERANGE vs -EIO ?
> There shouldn't be, but I can always return -EIO to be
> consistent with how it was previously.

Up to you Tom.  I just wanted you to think about it.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/24/2014 05:53 PM, Joe Perches wrote:
> On Tue, 2014-06-24 at 17:44 -0500, Tom Lendacky wrote:
>> On 06/24/2014 05:00 PM, Joe Perches wrote:
>>> On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
>>>> Checkpatch issued a warning preferring to use kstrto<type> when
>>>> using a single variable sscanf.  Change the sscanf invocation to
>>>> a kstrtouint call.
>>> []
>>>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
>>> []
>>>> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
>>>>    		return len;
>>>>
>>>>    	workarea[len] = '\0';
>>>> -	if (sscanf(workarea, "%x", &scan_value) == 1)
>>>> -		*value = scan_value;
>>>> -	else
>>>> -		return -EIO;
>>>> +	ret = kstrtouint(workarea, 0, value);
>>>
>>> Don't you need to use 16 for the base here?
>
>> Using 0 allows for greater flexibility in the input format.
>
> True, but there could be a change in behavior like reading a
> previously hex value like 10 is now a decimal 10 not decimal 16.
>
>>> Are there any issues with any of the various callers
>>> getting a different error return?
>>>
>>> -EINVAL/-ERANGE vs -EIO ?
>> There shouldn't be, but I can always return -EIO to be
>> consistent with how it was previously.
>
> Up to you Tom.  I just wanted you to think about it.
>

Understood.  This is a brand new driver that is still being developed
so I have some latitude initially on what and how I make changes. I
understand that later I'll have to maintain behaviors, interfaces, etc.

Thanks,
Tom

> cheers, Joe
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Joe Perches <joe@perches.com>
Date: Tue, 24 Jun 2014 15:53:30 -0700

> On Tue, 2014-06-24 at 17:44 -0500, Tom Lendacky wrote:
>> On 06/24/2014 05:00 PM, Joe Perches wrote:
>> > On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
>> >> Checkpatch issued a warning preferring to use kstrto<type> when
>> >> using a single variable sscanf.  Change the sscanf invocation to
>> >> a kstrtouint call.
>> > []
>> >> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
>> > []
>> >> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
>> >>   		return len;
>> >>
>> >>   	workarea[len] = '\0';
>> >> -	if (sscanf(workarea, "%x", &scan_value) == 1)
>> >> -		*value = scan_value;
>> >> -	else
>> >> -		return -EIO;
>> >> +	ret = kstrtouint(workarea, 0, value);
>> >
>> > Don't you need to use 16 for the base here?
> 
>> Using 0 allows for greater flexibility in the input format.
> 
> True, but there could be a change in behavior like reading a
> previously hex value like 10 is now a decimal 10 not decimal 16.

Tom, under other circumstance you can't change the format.
v3.16 is going to be released with the existing %x formatting
expecting hexadecimal numbers.

And you're targetting this change to decimal format in net-next.

The only thing that really allows you to do this is that this is
debugfs, and it's a reason I really hate debugfs, people do
arbitrary stuff so that if the debugfs elements turn out to be
useful for someone the driver author can arbitarily break things
on them however they want.

It's a cop-out for things people don't want to be bound to avoid ABI
changes, and to me that's garbage.  If you expose it to the user
design it well to the point where you're willing to live with it's
interface forever, or don't expose it to the user at all.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/26/2014 07:12 PM, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Tue, 24 Jun 2014 15:53:30 -0700
>
>> On Tue, 2014-06-24 at 17:44 -0500, Tom Lendacky wrote:
>>> On 06/24/2014 05:00 PM, Joe Perches wrote:
>>>> On Tue, 2014-06-24 at 16:19 -0500, Tom Lendacky wrote:
>>>>> Checkpatch issued a warning preferring to use kstrto<type> when
>>>>> using a single variable sscanf.  Change the sscanf invocation to
>>>>> a kstrtouint call.
>>>> []
>>>>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
>>>> []
>>>>> @@ -165,10 +165,9 @@ static ssize_t xgbe_common_write(const char __user *buffer, size_t count,
>>>>>    		return len;
>>>>>
>>>>>    	workarea[len] = '\0';
>>>>> -	if (sscanf(workarea, "%x", &scan_value) == 1)
>>>>> -		*value = scan_value;
>>>>> -	else
>>>>> -		return -EIO;
>>>>> +	ret = kstrtouint(workarea, 0, value);
>>>>
>>>> Don't you need to use 16 for the base here?
>>
>>> Using 0 allows for greater flexibility in the input format.
>>
>> True, but there could be a change in behavior like reading a
>> previously hex value like 10 is now a decimal 10 not decimal 16.
>
> Tom, under other circumstance you can't change the format.
> v3.16 is going to be released with the existing %x formatting
> expecting hexadecimal numbers.
>
> And you're targetting this change to decimal format in net-next.
>
> The only thing that really allows you to do this is that this is
> debugfs, and it's a reason I really hate debugfs, people do
> arbitrary stuff so that if the debugfs elements turn out to be
> useful for someone the driver author can arbitarily break things
> on them however they want.
>
> It's a cop-out for things people don't want to be bound to avoid ABI
> changes, and to me that's garbage.  If you expose it to the user
> design it well to the point where you're willing to live with it's
> interface forever, or don't expose it to the user at all.
>

After Joe's comments I started looking around to see what is expected
of debugfs.  I had always been under the impression that you shouldn't
expect things to remain compatible under debugfs.  But after reading a
number of things it appears that tools get created based on your
interface and then break when it's changed.

I'll submit a follow-on patch that addresses Joe's and your concerns and
put the interface back to just hexadecimal input with -EIO return on
error to maintain compatibility with what is in 3.16

Thanks,
Tom
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html