Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 13:52:41 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Mingxuan Xiang <mx_xiang@hust.edu.cn>,
        Sergey Shtylyov <s.shtylyov@omp.ru>
Cc:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        hust-os-kernel-patches@googlegroups.com,
        Dongliang Mu <dzm91@hust.edu.cn>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] usb: dwc3: host: remove dead code in
 dwc3_host_get_irq()
Message-ID: <eedbfdca-0eb1-4b01-976b-4ddba516cfad@kili.mountain>
References: <20230323095311.1266655-1-mx_xiang@hust.edu.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230323095311.1266655-1-mx_xiang@hust.edu.cn>
Precedence: bulk

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