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 <[email protected]>
---
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;
}
- if (!irq)
- irq = -EINVAL;
-
out:
return irq;
}
--
2.39.2
On 3/23/23 12:53 PM, 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 <[email protected]>
> ---
> 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. :-)
>
> - if (!irq)
> - irq = -EINVAL;
> -
> out:
> return irq;
> }
MBR, Sergey
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 <[email protected]>
> ---
> 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 23.03.23 11:29, Sergei Shtylyov wrote:
> On 3/23/23 12:53 PM, 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 <[email protected]>
>> ---
>> 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.
Regards
Oliver
Here is how I would write that function, but I can't justify sending it
because it's just a matter of personal taste.
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f6f13e7f1ba1..fcc831ac3236 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -36,32 +36,28 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
irq = platform_get_irq_byname_optional(dwc3_pdev, "host");
if (irq > 0) {
dwc3_host_fill_xhci_irq_res(dwc, irq, "host");
- goto out;
+ return irq;
}
if (irq == -EPROBE_DEFER)
- goto out;
+ return -EPROBE_DEFER;
irq = platform_get_irq_byname_optional(dwc3_pdev, "dwc_usb3");
if (irq > 0) {
dwc3_host_fill_xhci_irq_res(dwc, irq, "dwc_usb3");
- goto out;
+ return irq;
}
if (irq == -EPROBE_DEFER)
- goto out;
+ return -EPROBE_DEFER;
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;
}
int dwc3_host_init(struct dwc3 *dwc)
On Thu, Mar 23, 2023 at 11:54:24AM +0100, 'Oliver Neukum' via HUST OS Kernel Contribution wrote:
>
>
> On 23.03.23 11:29, Sergei Shtylyov wrote:
> > On 3/23/23 12:53 PM, 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 <[email protected]>
> > > ---
> > > 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...
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 Thu, Mar 23, 2023 at 02:48:56PM +0100, Oliver Neukum wrote:
> 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
You've reversed this if statement in your head. It says that if
platform_get_irq() returns zero, then return -EINVAL.
The problem that Mingxuan is trying to address is that checking for zero
is dead code and sometimes represents a bug when people check for zero
instead of negatives.
> irq = -EINVAL;
>
> out:
> return irq;
>
> returning -EINVAL
We do *want* it to return -EPROBE_DEFER as the current code does.
regards,
dan carpenter
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 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?
thanks,
greg k-h
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 <[email protected]> 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 [email protected].
> 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.