diff mbox series

tty: serial: samsung_tty: remove set but not used variables

Message ID 1619170740-63717-1-git-send-email-tiantao6@hisilicon.com
State Superseded
Headers show
Series tty: serial: samsung_tty: remove set but not used variables | expand

Commit Message

Tian Tao April 23, 2021, 9:39 a.m. UTC
The value of 'ret' is not used, so just delete it.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/tty/serial/samsung_tty.c | 1 -
 1 file changed, 1 deletion(-)

Comments

tiantao (H) April 23, 2021, 9:54 a.m. UTC | #1
在 2021/4/23 17:47, Greg KH 写道:
> On Fri, Apr 23, 2021 at 05:39:00PM +0800, Tian Tao wrote:
>> The value of 'ret' is not used, so just delete it.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>>   drivers/tty/serial/samsung_tty.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index d9e4b67..d269d75 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -2220,7 +2220,6 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>>   			default:
>>   				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
>>   						prop);
>> -				ret = -EINVAL;
> That looks odd, shouldn't you do something with this instead of ignoring
> it???

How about this ?

diff --git a/drivers/tty/serial/samsung_tty.c 
b/drivers/tty/serial/samsung_tty.c
index d9e4b67..9fbc611 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2220,8 +2220,7 @@ static int s3c24xx_serial_probe(struct 
platform_device *pdev)
                         default:
                                 dev_warn(&pdev->dev, "unsupported 
reg-io-width (%d)\n",
                                                 prop);
-                               ret = -EINVAL;
-                               break;
+                               return -EINVAL;

> .
>
Greg KH April 23, 2021, 10:14 a.m. UTC | #2
On Fri, Apr 23, 2021 at 05:54:16PM +0800, tiantao (H) wrote:
> 
> 在 2021/4/23 17:47, Greg KH 写道:
> > On Fri, Apr 23, 2021 at 05:39:00PM +0800, Tian Tao wrote:
> > > The value of 'ret' is not used, so just delete it.
> > > 
> > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > > ---
> > >   drivers/tty/serial/samsung_tty.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > index d9e4b67..d269d75 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -2220,7 +2220,6 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> > >   			default:
> > >   				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> > >   						prop);
> > > -				ret = -EINVAL;
> > That looks odd, shouldn't you do something with this instead of ignoring
> > it???
> 
> How about this ?
> 
> diff --git a/drivers/tty/serial/samsung_tty.c
> b/drivers/tty/serial/samsung_tty.c
> index d9e4b67..9fbc611 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2220,8 +2220,7 @@ static int s3c24xx_serial_probe(struct platform_device
> *pdev)
>                         default:
>                                 dev_warn(&pdev->dev, "unsupported
> reg-io-width (%d)\n",
>                                                 prop);
> -                               ret = -EINVAL;
> -                               break;
> +                               return -EINVAL;
> 

You tell me, does the patch work for you?

Is this really a "hard error" and did you now just break devices that
used to work properly?  Are you correctly unwinding any previously
allocated state when you return here?

Please do some research on this, and ideally, lots of testing, before
submitting it as a real solution.

thanks,

greg k-h
Krzysztof Kozlowski April 26, 2021, 6:45 a.m. UTC | #3
On 23/04/2021 12:14, Greg KH wrote:
> On Fri, Apr 23, 2021 at 05:54:16PM +0800, tiantao (H) wrote:
>>
>> 在 2021/4/23 17:47, Greg KH 写道:
>>> On Fri, Apr 23, 2021 at 05:39:00PM +0800, Tian Tao wrote:
>>>> The value of 'ret' is not used, so just delete it.

Tian Tao, please use scripts/get_maintainers.pl to get the list of
people needed for Cc.

>>>>
>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>>>> ---
>>>>   drivers/tty/serial/samsung_tty.c | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>>> index d9e4b67..d269d75 100644
>>>> --- a/drivers/tty/serial/samsung_tty.c
>>>> +++ b/drivers/tty/serial/samsung_tty.c
>>>> @@ -2220,7 +2220,6 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>>>>   			default:
>>>>   				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
>>>>   						prop);
>>>> -				ret = -EINVAL;
>>> That looks odd, shouldn't you do something with this instead of ignoring
>>> it???
>>
>> How about this ?
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c
>> b/drivers/tty/serial/samsung_tty.c
>> index d9e4b67..9fbc611 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -2220,8 +2220,7 @@ static int s3c24xx_serial_probe(struct platform_device
>> *pdev)
>>                         default:
>>                                 dev_warn(&pdev->dev, "unsupported
>> reg-io-width (%d)\n",
>>                                                 prop);
>> -                               ret = -EINVAL;
>> -                               break;
>> +                               return -EINVAL;
>>
> 
> You tell me, does the patch work for you?
> 
> Is this really a "hard error" and did you now just break devices that
> used to work properly?  Are you correctly unwinding any previously
> allocated state when you return here?
> 
> Please do some research on this, and ideally, lots of testing, before
> submitting it as a real solution.

It's a patch coming from automated tool (e.g. Coverity), so I doubt
there is any testing here. However the "return -EINVAL" looks correct here:
1. No particular unwinding is needed here,
2. It's an optional property (not used by existing DTS, only
non-upstreamed by Samsung) thus treating it as hard-error is fine.
Probably better to exit than convert it to some default value.


Best regards,
Krzysztof
Greg KH April 26, 2021, 6:56 a.m. UTC | #4
On Mon, Apr 26, 2021 at 08:45:44AM +0200, Krzysztof Kozlowski wrote:
> On 23/04/2021 12:14, Greg KH wrote:

> > On Fri, Apr 23, 2021 at 05:54:16PM +0800, tiantao (H) wrote:

> >>

> >> 在 2021/4/23 17:47, Greg KH 写道:

> >>> On Fri, Apr 23, 2021 at 05:39:00PM +0800, Tian Tao wrote:

> >>>> The value of 'ret' is not used, so just delete it.

> 

> Tian Tao, please use scripts/get_maintainers.pl to get the list of

> people needed for Cc.

> 

> >>>>

> >>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

> >>>> ---

> >>>>   drivers/tty/serial/samsung_tty.c | 1 -

> >>>>   1 file changed, 1 deletion(-)

> >>>>

> >>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c

> >>>> index d9e4b67..d269d75 100644

> >>>> --- a/drivers/tty/serial/samsung_tty.c

> >>>> +++ b/drivers/tty/serial/samsung_tty.c

> >>>> @@ -2220,7 +2220,6 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)

> >>>>   			default:

> >>>>   				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",

> >>>>   						prop);

> >>>> -				ret = -EINVAL;

> >>> That looks odd, shouldn't you do something with this instead of ignoring

> >>> it???

> >>

> >> How about this ?

> >>

> >> diff --git a/drivers/tty/serial/samsung_tty.c

> >> b/drivers/tty/serial/samsung_tty.c

> >> index d9e4b67..9fbc611 100644

> >> --- a/drivers/tty/serial/samsung_tty.c

> >> +++ b/drivers/tty/serial/samsung_tty.c

> >> @@ -2220,8 +2220,7 @@ static int s3c24xx_serial_probe(struct platform_device

> >> *pdev)

> >>                         default:

> >>                                 dev_warn(&pdev->dev, "unsupported

> >> reg-io-width (%d)\n",

> >>                                                 prop);

> >> -                               ret = -EINVAL;

> >> -                               break;

> >> +                               return -EINVAL;

> >>

> > 

> > You tell me, does the patch work for you?

> > 

> > Is this really a "hard error" and did you now just break devices that

> > used to work properly?  Are you correctly unwinding any previously

> > allocated state when you return here?

> > 

> > Please do some research on this, and ideally, lots of testing, before

> > submitting it as a real solution.

> 

> It's a patch coming from automated tool (e.g. Coverity), so I doubt

> there is any testing here. However the "return -EINVAL" looks correct here:

> 1. No particular unwinding is needed here,

> 2. It's an optional property (not used by existing DTS, only

> non-upstreamed by Samsung) thus treating it as hard-error is fine.

> Probably better to exit than convert it to some default value.


So is that a "Reviwed-by:" or not?  :)
Krzysztof Kozlowski April 26, 2021, 6:57 a.m. UTC | #5
On 26/04/2021 08:56, Greg KH wrote:
> On Mon, Apr 26, 2021 at 08:45:44AM +0200, Krzysztof Kozlowski wrote:

>> On 23/04/2021 12:14, Greg KH wrote:

>>> On Fri, Apr 23, 2021 at 05:54:16PM +0800, tiantao (H) wrote:

>>>>

>>>> 在 2021/4/23 17:47, Greg KH 写道:

>>>>> On Fri, Apr 23, 2021 at 05:39:00PM +0800, Tian Tao wrote:

>>>>>> The value of 'ret' is not used, so just delete it.

>>

>> Tian Tao, please use scripts/get_maintainers.pl to get the list of

>> people needed for Cc.

>>

>>>>>>

>>>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

>>>>>> ---

>>>>>>   drivers/tty/serial/samsung_tty.c | 1 -

>>>>>>   1 file changed, 1 deletion(-)

>>>>>>

>>>>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c

>>>>>> index d9e4b67..d269d75 100644

>>>>>> --- a/drivers/tty/serial/samsung_tty.c

>>>>>> +++ b/drivers/tty/serial/samsung_tty.c

>>>>>> @@ -2220,7 +2220,6 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)

>>>>>>   			default:

>>>>>>   				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",

>>>>>>   						prop);

>>>>>> -				ret = -EINVAL;

>>>>> That looks odd, shouldn't you do something with this instead of ignoring

>>>>> it???

>>>>

>>>> How about this ?

>>>>

>>>> diff --git a/drivers/tty/serial/samsung_tty.c

>>>> b/drivers/tty/serial/samsung_tty.c

>>>> index d9e4b67..9fbc611 100644

>>>> --- a/drivers/tty/serial/samsung_tty.c

>>>> +++ b/drivers/tty/serial/samsung_tty.c

>>>> @@ -2220,8 +2220,7 @@ static int s3c24xx_serial_probe(struct platform_device

>>>> *pdev)

>>>>                         default:

>>>>                                 dev_warn(&pdev->dev, "unsupported

>>>> reg-io-width (%d)\n",

>>>>                                                 prop);

>>>> -                               ret = -EINVAL;

>>>> -                               break;

>>>> +                               return -EINVAL;

>>>>

>>>

>>> You tell me, does the patch work for you?

>>>

>>> Is this really a "hard error" and did you now just break devices that

>>> used to work properly?  Are you correctly unwinding any previously

>>> allocated state when you return here?

>>>

>>> Please do some research on this, and ideally, lots of testing, before

>>> submitting it as a real solution.

>>

>> It's a patch coming from automated tool (e.g. Coverity), so I doubt

>> there is any testing here. However the "return -EINVAL" looks correct here:

>> 1. No particular unwinding is needed here,

>> 2. It's an optional property (not used by existing DTS, only

>> non-upstreamed by Samsung) thus treating it as hard-error is fine.

>> Probably better to exit than convert it to some default value.

> 

> So is that a "Reviwed-by:" or not?  :)


First, Tian Tao needs to send a v2 of that patch with a "return" instead
of break. Then this will be a reviewed-by. :)

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d9e4b67..d269d75 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2220,7 +2220,6 @@  static int s3c24xx_serial_probe(struct platform_device *pdev)
 			default:
 				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
 						prop);
-				ret = -EINVAL;
 				break;
 			}
 		}