| 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 |
在 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; > . >
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
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(-)