Re: [PATCH] rts5208: fix a missing check of the status of sd_init_power

[Dan Carpenter]

On Tue, Dec 25, 2018 at 02:33:32AM -0600, Kangjie Lu wrote:
> sd_init_power() could fail. The fix inserts a check of its status. If it
> fails, returns STATUS_FAIL.
> 
> Signed-off-by: Kangjie Lu <kjlu@xxxxxxx>
> ---
>  drivers/staging/rts5208/sd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> index ff1a9aa152ce..8c3fd885a4f3 100644
> --- a/drivers/staging/rts5208/sd.c
> +++ b/drivers/staging/rts5208/sd.c
> @@ -2352,7 +2352,9 @@ static int reset_sd(struct rtsx_chip *chip)
>  				break;
>  			}
>  
> -			sd_init_power(chip);
> +			retval = sd_init_power(chip);
> +			if (retval != STATUS_SUCCESS)
> +				goto status_fail;

Are you able to test this?

I don't think you appreciate the risk of applying this patch.  We are
taking code which succeeds and making it fail.  That's a risky thing.
The benefit is just that it makes a static analysis tool happy?

It would be different if the benefit were that it improves security or
prevents a crash.  Or if you could test it.

This is the reset_sd() function.  It always feels a bit hacky to me to
even have a reset function.  It basically means you know you have messed
up but you don't know where or how so let's just try scrub everything
and go back to the start.  Maybe the driver is trying to work around a
firmware bug.  I often find bugs in reset functions which show they have
not been tested...  It's so tricky for me to do a cost benefit analysis
here, but I sort of think we should leave the code as-is.

It's frustrating because the static analysis tool is probably right, but
maybe the code only works because two bugs cancel each other out?  We
can't know without testing.

In a more ideal world we would do the static analysis on patches before
they are applied and refuse to apply them until the maintainer explains
why the function is not checked.

regards,
dan carpenter