[PATCH] usb: dwc3: host: remove dead code in dwc3_host_get_irq()

[Mingxuan Xiang]

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_x...@hust.edu.cn>
---
drivers/usb/dwc3/host.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f6f13e7f1ba1..aa19c27344e6 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -57,9 +57,6 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
goto out;
}

- if (!irq)
- irq = -EINVAL;
-
out:
return irq;
}
--
2.39.2

[Dongliang Mu]

On 3/23/23 11:45, 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_x...@hust.edu.cn>

This patch looks fine, you can send it to the kernel mailing list.
Please remember to cc our own mailing list.

Refer to the following webpage to automatically extract maintainers and
mailing lists.

[1]
https://mudongliang.github.io/2021/06/21/git-send-email-with-cc-cmd-and-to-cmd.html

[Mingxuan Xiang]

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_x...@hust.edu.cn>

---
drivers/usb/dwc3/host.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f6f13e7f1ba1..aa19c27344e6 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -57,9 +57,6 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
goto out;
}

- if (!irq)
- irq = -EINVAL;
-
out:
return irq;
}

--
2.39.2

[Sergei Shtylyov]

On 3/23/23 8:42 AM, 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_x...@hust.edu.cn>
> ---
> drivers/usb/dwc3/host.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f6f13e7f1ba1..aa19c27344e6 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -57,9 +57,6 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
> goto out;

Shouldn't *goto* be dropped now?

> }
>
> - if (!irq)
> - irq = -EINVAL;
> -
> out:
> return irq;
> }

MBR, Sergey

[Mingxuan Xiang]

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_x...@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;

}

- if (!irq)
- irq = -EINVAL;
-
out:
return irq;
}

--
2.39.2

[Sergei Shtylyov]

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 <mx_x...@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;
> }

Now drop {} please. :-)

>
> - if (!irq)
> - irq = -EINVAL;
> -
> out:
> return irq;
> }

MBR, Sergey

[Dan Carpenter]

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_x...@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

[Oliver Neukum]

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

[Dan Carpenter]

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)

[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

[Dan Carpenter]

The v2 patch doesn't change the semantics. Mine did though...

regards,
dan carpenter

[Oliver Neukum]

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

[Dan Carpenter]

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

[Oliver Neukum]

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

[Greg Kroah-Hartman]

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

[Dongliang Mu]

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

[Dan Carpenter]

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

[Dan Carpenter]

Also you could mention in the commit message that platform_get_irq()
has a comment about returns:

* Return: non-zero IRQ number on success, negative error number on failure.

regards,
dan carpenter

[Dongliang Mu]

On Fri, Mar 24, 2023 at 1:17 PM Dan Carpenter <err...@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-pa...@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.

[Mingxuan Xiang]

According to the description of platform_get_irq()

* Return: non-zero IRQ number on success,
negative error number on failure.

and the code, platform_get_irq() will return -EINVAL
instead of IRQ0.

So platform_get_irq() no longer returns 0, there is no

need to check whether the return value is 0.

Found by Smatch:
drivers/usb/dwc3/host.c:60 dwc3_host_get_irq()
warn: platform_get_irq() does not return zero

Signed-off-by: Mingxuan Xiang <mx_x...@hust.edu.cn>
---

v2->v3: remove curly braces

v1->v2: remove redundant goto

Only compile tested.
---
drivers/usb/dwc3/host.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f6f13e7f1ba1..61f57fe5bb78 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -52,13 +52,8 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
goto out;

irq = platform_get_irq(dwc3_pdev, 0);
- if (irq > 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

[Dan Carpenter]

On Fri, Mar 24, 2023 at 02:09:34PM +0800, Mingxuan Xiang wrote:
> According to the description of platform_get_irq()
> * Return: non-zero IRQ number on success,
> negative error number on failure.
> and the code, platform_get_irq() will return -EINVAL
> instead of IRQ0.
>
> So platform_get_irq() no longer returns 0, there is no
> need to check whether the return value is 0.
>
> Found by Smatch:
> drivers/usb/dwc3/host.c:60 dwc3_host_get_irq()
> warn: platform_get_irq() does not return zero
>
> Signed-off-by: Mingxuan Xiang <mx_x...@hust.edu.cn>
> ---
> v2->v3: remove curly braces
> v1->v2: remove redundant goto
> Only compile tested.

Reviewed-by: Dan Carpenter <err...@gmail.com>

regards,
dan carpenter

[Thinh Nguyen]

Acked-by: Thinh Nguyen <Thinh....@synopsys.com>

Thanks,
Thinh