| Message ID | 20210601100311.70200-1-tongtiangen@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next] usb: isp1760: Fix meaningless check in isp1763_run() | expand |
On Tue, Jun 01, 2021 at 06:03:11PM +0800, Tong Tiangen wrote: > There's a meaningless check in isp1763_run. According to the > similar implement in isp1760_run, the proper check should remove > retval = 0; > > Fixes: 60d789f3bfbb ("usb: isp1760: add support for isp1763") > Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> > --- > drivers/usb/isp1760/isp1760-hcd.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c > index 016a54ea76f4..27168b4a4ef2 100644 > --- a/drivers/usb/isp1760/isp1760-hcd.c > +++ b/drivers/usb/isp1760/isp1760-hcd.c > @@ -1648,7 +1648,6 @@ static int isp1763_run(struct usb_hcd *hcd) > down_write(&ehci_cf_port_reset_rwsem); > retval = isp1760_hcd_set_and_wait(hcd, FLAG_CF, 250 * 1000); > up_write(&ehci_cf_port_reset_rwsem); > - retval = 0; > if (retval) > return retval; > > -- > 2.18.0.huawei.25 > Did you test this change to verify that the driver still works properly? You are now checking something that never was checked before... thanks, greg k-h
On 2021/6/3 19:45, Greg Kroah-Hartman wrote: > On Tue, Jun 01, 2021 at 06:03:11PM +0800, Tong Tiangen wrote: >> There's a meaningless check in isp1763_run. According to the >> similar implement in isp1760_run, the proper check should remove >> retval = 0; >> >> Fixes: 60d789f3bfbb ("usb: isp1760: add support for isp1763") >> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> >> --- >> drivers/usb/isp1760/isp1760-hcd.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c >> index 016a54ea76f4..27168b4a4ef2 100644 >> --- a/drivers/usb/isp1760/isp1760-hcd.c >> +++ b/drivers/usb/isp1760/isp1760-hcd.c >> @@ -1648,7 +1648,6 @@ static int isp1763_run(struct usb_hcd *hcd) >> down_write(&ehci_cf_port_reset_rwsem); >> retval = isp1760_hcd_set_and_wait(hcd, FLAG_CF, 250 * 1000); >> up_write(&ehci_cf_port_reset_rwsem); >> - retval = 0; >> if (retval) >> return retval; >> >> -- >> 2.18.0.huawei.25 >> > Did you test this change to verify that the driver still works properly? > You are now checking something that never was checked before... > > thanks, > > greg k-h > . Sorry, this fix was not send to Rui. From the point of view of code logic, there should be a problem here. I don't have the actual hardware to verify whether it works properly. Rui may know if the patch affects the original workflow. thanks . >
Hi Tiangen, On Fri Jun 4, 2021 at 3:09 AM WEST, tongtiangen wrote: > On 2021/6/3 19:45, Greg Kroah-Hartman wrote: > > On Tue, Jun 01, 2021 at 06:03:11PM +0800, Tong Tiangen wrote: > >> There's a meaningless check in isp1763_run. According to the > >> similar implement in isp1760_run, the proper check should remove > >> retval = 0; > >> > >> Fixes: 60d789f3bfbb ("usb: isp1760: add support for isp1763") > >> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> > >> --- > >> drivers/usb/isp1760/isp1760-hcd.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c > >> index 016a54ea76f4..27168b4a4ef2 100644 > >> --- a/drivers/usb/isp1760/isp1760-hcd.c > >> +++ b/drivers/usb/isp1760/isp1760-hcd.c > >> @@ -1648,7 +1648,6 @@ static int isp1763_run(struct usb_hcd *hcd) > >> down_write(&ehci_cf_port_reset_rwsem); > >> retval = isp1760_hcd_set_and_wait(hcd, FLAG_CF, 250 * 1000); > >> up_write(&ehci_cf_port_reset_rwsem); > >> - retval = 0; > >> if (retval) > >> return retval; > >> > >> -- > >> 2.18.0.huawei.25 > >> > > Did you test this change to verify that the driver still works properly? > > You are now checking something that never was checked before... Thanks Greg for asking the right question here :) > > > > thanks, > > > > greg k-h > > . > Sorry, this fix was not send to Rui. Yeah, I did not get the original, no. I will need to add entries in MAINTAINERS for this driver to be easier for others. > From the point of view of code logic, there should be a problem here. It looks like. Only if we fail to setup the Configure Flag, something that it's not really expected to fail. > I don't have the actual hardware to verify whether it works > properly. Rui may know if the patch affects the original workflow. Thanks for the patch and I will test this and get back to you. ------ Cheers, Rui
Hi, Managed to test this and looks good. Everything is working as expected. On Fri Jun 4, 2021 at 3:09 AM WEST, tongtiangen wrote: > On 2021/6/3 19:45, Greg Kroah-Hartman wrote: > > On Tue, Jun 01, 2021 at 06:03:11PM +0800, Tong Tiangen wrote: > >> There's a meaningless check in isp1763_run. According to the > >> similar implement in isp1760_run, the proper check should remove > >> retval = 0; however I think a more descriptive changelog instead of comparing to similar function would be better, maybe something around: "Remove attribution to retval before check, which make it completely meaningless, and does't check what it was supposed: the return value of the timed function to set up configuration flag." > >> > >> Fixes: 60d789f3bfbb ("usb: isp1760: add support for isp1763") > >> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> With changelog changed you can add: Tested-by: Rui Miguel Silva <rui.silva@linaro.org> Reviewed-by: Rui Miguel Silva <rui.silva@linaro.org> ------ Cheers, Rui > >> --- > >> drivers/usb/isp1760/isp1760-hcd.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c > >> index 016a54ea76f4..27168b4a4ef2 100644 > >> --- a/drivers/usb/isp1760/isp1760-hcd.c > >> +++ b/drivers/usb/isp1760/isp1760-hcd.c > >> @@ -1648,7 +1648,6 @@ static int isp1763_run(struct usb_hcd *hcd) > >> down_write(&ehci_cf_port_reset_rwsem); > >> retval = isp1760_hcd_set_and_wait(hcd, FLAG_CF, 250 * 1000); > >> up_write(&ehci_cf_port_reset_rwsem); > >> - retval = 0; > >> if (retval) > >> return retval; > >> > >> -- > >> 2.18.0.huawei.25 > >> > > Did you test this change to verify that the driver still works properly? > > You are now checking something that never was checked before... > > > > thanks, > > > > greg k-h > > . > Sorry, this fix was not send to Rui. > From the point of view of code logic, there should be a problem here. I > don't have the actual hardware to verify whether it works properly. Rui > may know if the patch affects the original workflow. > > thanks > . > >
diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c index 016a54ea76f4..27168b4a4ef2 100644 --- a/drivers/usb/isp1760/isp1760-hcd.c +++ b/drivers/usb/isp1760/isp1760-hcd.c @@ -1648,7 +1648,6 @@ static int isp1763_run(struct usb_hcd *hcd) down_write(&ehci_cf_port_reset_rwsem); retval = isp1760_hcd_set_and_wait(hcd, FLAG_CF, 250 * 1000); up_write(&ehci_cf_port_reset_rwsem); - retval = 0; if (retval) return retval;
There's a meaningless check in isp1763_run. According to the similar implement in isp1760_run, the proper check should remove retval = 0; Fixes: 60d789f3bfbb ("usb: isp1760: add support for isp1763") Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> --- drivers/usb/isp1760/isp1760-hcd.c | 1 - 1 file changed, 1 deletion(-)