| 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 |
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???
On 23. 04. 21, 11:39, 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; > break; Added by: commit 57253ccd5831e7e5720c433437775c3e6b7d0c72 Author: Hyunki Koo <hyunki00.koo@samsung.com> Date: Wed May 6 17:02:40 2020 +0900 serial: samsung: 32-bit access for TX/RX hold registers Hyunki, what was the intention with "ret" here? > } > } >
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
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? :)
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 --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; } }
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(-)