* [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() @ 2021-06-01 10:03 Tong Tiangen 2021-06-03 11:45 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Tong Tiangen @ 2021-06-01 10:03 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Tong Tiangen 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() 2021-06-01 10:03 [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() Tong Tiangen @ 2021-06-03 11:45 ` Greg Kroah-Hartman 2021-06-04 2:09 ` tongtiangen 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2021-06-03 11:45 UTC (permalink / raw) To: Tong Tiangen; +Cc: linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() 2021-06-03 11:45 ` Greg Kroah-Hartman @ 2021-06-04 2:09 ` tongtiangen 2021-06-04 8:22 ` Rui Miguel Silva 2021-06-04 15:12 ` Rui Miguel Silva 0 siblings, 2 replies; 5+ messages in thread From: tongtiangen @ 2021-06-04 2:09 UTC (permalink / raw) To: Greg Kroah-Hartman, rui.silva; +Cc: linux-usb, linux-kernel 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 . > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() 2021-06-04 2:09 ` tongtiangen @ 2021-06-04 8:22 ` Rui Miguel Silva 2021-06-04 15:12 ` Rui Miguel Silva 1 sibling, 0 replies; 5+ messages in thread From: Rui Miguel Silva @ 2021-06-04 8:22 UTC (permalink / raw) To: tongtiangen, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() 2021-06-04 2:09 ` tongtiangen 2021-06-04 8:22 ` Rui Miguel Silva @ 2021-06-04 15:12 ` Rui Miguel Silva 1 sibling, 0 replies; 5+ messages in thread From: Rui Miguel Silva @ 2021-06-04 15:12 UTC (permalink / raw) To: tongtiangen, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel 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 > . > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-04 15:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-01 10:03 [PATCH -next] usb: isp1760: Fix meaningless check in isp1763_run() Tong Tiangen 2021-06-03 11:45 ` Greg Kroah-Hartman 2021-06-04 2:09 ` tongtiangen 2021-06-04 8:22 ` Rui Miguel Silva 2021-06-04 15:12 ` Rui Miguel Silva
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).