| Message ID | 20230323095311.1266655-1-mx_xiang@hust.edu.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc3: host: remove dead code in dwc3_host_get_irq() | expand |
On Thu, Mar 23, 2023 at 05:53:10PM +0800, Mingxuan Xiang wrote: > platform_get_irq() no longer returns 0, so there is no > need to check whether the return value is 0. > > Signed-off-by: Mingxuan Xiang <mx_xiang@hust.edu.cn> > --- > v1->v2: remove redundant goto > drivers/usb/dwc3/host.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index f6f13e7f1ba1..ca1e8294e835 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -54,12 +54,8 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > irq = platform_get_irq(dwc3_pdev, 0); > if (irq > 0) { > dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); > - goto out; > } This patch is against kernel standards because we do not use {} curly braces for single line indents. I prefered the v1 patch. It silenced the static checker warning and deleted the dead code without getting into unrelated cleanups. I do not like deleting the goto because now the last if statement is different and I regard "making the last thing different" as an anti-pattern. It's better to be consistent. I also prefer to keep the error path and the success path as separate as possible. This function is weird because we are trying a bunch of different functions until one succeeds. Normally it is the reverse. Everything is expected to succeed and we give up as soon as we encounter a failure. So normally I would expect that the failure path would be indented an extra tab and I tell everyone to do failure handling not success handling but this function is the reverse. I also do not like do nothing out labels. It is more readable to return directly. Some people think that using an out label will encourage discipline and force people to think about error handling. There is no evidence to support this. I see plenty of ommited clean up in functions which have out labels. On the other hand, there is a lot of evidence that do nothing out labels introduce Forgot To Set the Error Code bugs. People sometimes think that error codes are not important but returning success instead of failure almost always leads to a kernel crash and for verify_input() functions forgetting to set the error code has obvious security implications. So anyway, I would probably re-write this function in a different way, but it's not related to the dead code. Next time, if someone asks you to make unrelated cleanups don't get tricked into a huge discussion about style. Just say that it seems unrelated and that it should be in a separate patch. On the other hand, I don't really care... I guess send a v3 of this patch but delete the { } as well. I still prefer v1 but since I don't care then let's do whatever is expedient. regards, dan carpenter
On Thu, Mar 23, 2023 at 02:00:35PM +0300, Dan Carpenter wrote: > irq = platform_get_irq(dwc3_pdev, 0); > if (irq > 0) { > dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); > - goto out; > + return irq; > } > > - if (!irq) > - irq = -EINVAL; > - > -out: > - return irq; > + return -ENODEV; Oh wait. We actually need to propagate the error code here because of -EPROBE_DEFER so my patch introduces a bug. regards, dan carpenter
On 23.03.23 12:13, Dan Carpenter wrote: >>>> v1->v2: remove redundant goto >>>> drivers/usb/dwc3/host.c | 4 ---- >>>> 1 file changed, 4 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>> index f6f13e7f1ba1..ca1e8294e835 100644 >>>> --- a/drivers/usb/dwc3/host.c >>>> +++ b/drivers/usb/dwc3/host.c >>>> @@ -54,12 +54,8 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) >>>> irq = platform_get_irq(dwc3_pdev, 0); >>>> if (irq > 0) { >>>> dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); >>>> - goto out; >>>> } >>> >>> Now drop {} please. :-) >> >> Well, no, please drop the whole patch. >> If platform_get_irq() returns -EPROBE_DEFER you now give that >> as a return value. >> >> This tiny bit of optimization is not worth changing semantics. > > The v2 patch doesn't change the semantics. Mine did though... Now I may be dense, but let's look at the current code: irq = platform_get_irq(dwc3_pdev, 0); assuming irq = -EPROBE_DEFER if (irq > 0) { not taken dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); goto out; } if (!irq) irq != 0 irq = -EINVAL; out: return irq; returning -EINVAL Patched version: irq = platform_get_irq(dwc3_pdev, 0); assuming irq = -EPROBE_DEFER if (irq > 0) { dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); } out: return irq; returning -EPROBE_DEFER Your version: irq = platform_get_irq(dwc3_pdev, 0); assuming irq = -EPROBE_DEFER if (irq > 0) { not taken dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); + return irq; } + return -ENODEV; Yet another error return. Now, I admit I am by now sufficiently confused to know which version is correct, but they are all three different in what they return. Regards Oliver
On 23.03.23 15:06, Dan Carpenter wrote: >> irq != 0 > > You've reversed this if statement in your head. It says that if > platform_get_irq() returns zero, then return -EINVAL. > Argh. You are right. Sorry Oliver
On 2023/3/23 23:40, Greg Kroah-Hartman wrote: > On Thu, Mar 23, 2023 at 05:53:10PM +0800, Mingxuan Xiang wrote: >> platform_get_irq() no longer returns 0, so there is no >> need to check whether the return value is 0. > How did you find this issue? > > How was it tested? Hi Greg, We found this issue by Smatch. Our team is trying fix some true bugs found by Smatch, with the help of Dan. Since this is a dead code removal, we only do compilation testing. > > thanks, > > greg k-h
On Fri, Mar 24, 2023 at 09:46:40AM +0800, Dongliang Mu wrote: > > On 2023/3/23 23:40, Greg Kroah-Hartman wrote: > > On Thu, Mar 23, 2023 at 05:53:10PM +0800, Mingxuan Xiang wrote: > > > platform_get_irq() no longer returns 0, so there is no > > > need to check whether the return value is 0. > > How did you find this issue? > > > > How was it tested? > > Hi Greg, > > We found this issue by Smatch. Our team is trying fix some true bugs found > by Smatch, with the help of Dan. LOL. What did I do to get thrown under the bus like this? > > Since this is a dead code removal, we only do compilation testing. > Just v3 and mention in the commit message that the issue was found by Smatch and the warning message that Smatch prints. Put under the --- cut off line that it has only been compile tested. Also we needed to send a v3 anyway to remove the {} curly braces. regards, dan carpenter
On Fri, Mar 24, 2023 at 1:17 PM Dan Carpenter <error27@gmail.com> wrote: > > On Fri, Mar 24, 2023 at 09:46:40AM +0800, Dongliang Mu wrote: > > > > On 2023/3/23 23:40, Greg Kroah-Hartman wrote: > > > On Thu, Mar 23, 2023 at 05:53:10PM +0800, Mingxuan Xiang wrote: > > > > platform_get_irq() no longer returns 0, so there is no > > > > need to check whether the return value is 0. > > > How did you find this issue? > > > > > > How was it tested? > > > > Hi Greg, > > > > We found this issue by Smatch. Our team is trying fix some true bugs found > > by Smatch, with the help of Dan. > > LOL. What did I do to get thrown under the bus like this? Oh, my apologies :). We are a team. We together would like to fix kernel bugs and do some contribution. > > > > > Since this is a dead code removal, we only do compilation testing. > > > > Just v3 and mention in the commit message that the issue was found by > Smatch and the warning message that Smatch prints. Put under the --- > cut off line that it has only been compile tested. > > Also we needed to send a v3 anyway to remove the {} curly braces. Yes, I have asked Mingxuan to craft this v3 patch. > > regards, > dan carpenter > > -- > You received this message because you are subscribed to the Google Groups "HUST OS Kernel Contribution" group. > To unsubscribe from this group and stop receiving emails from it, send an email to hust-os-kernel-patches+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/hust-os-kernel-patches/adcd6c67-cedf-4831-9a9d-53c3ee2ebb88%40kili.mountain.
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index f6f13e7f1ba1..ca1e8294e835 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -54,12 +54,8 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) irq = platform_get_irq(dwc3_pdev, 0); if (irq > 0) { dwc3_host_fill_xhci_irq_res(dwc, irq, NULL); - goto out; } - if (!irq) - irq = -EINVAL; - out: return irq; }
platform_get_irq() no longer returns 0, so there is no need to check whether the return value is 0. Signed-off-by: Mingxuan Xiang <mx_xiang@hust.edu.cn> --- v1->v2: remove redundant goto drivers/usb/dwc3/host.c | 4 ---- 1 file changed, 4 deletions(-)