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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
