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, 30 Mar 2023 18:23:19 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Khadija Kamran <kamrankhadijadj@gmail.com>
Cc:     outreachy@lists.linux.dev, hvaibhav.linux@gmail.com,
        johan@kernel.org, elder@kernel.org, gregkh@linuxfoundation.org,
        greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org,
        Alison Schofield <alison.schofield@intel.com>
Subject: Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq()
Message-ID: <6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain>
References: <cover.1680185025.git.kamrankhadijadj@gmail.com>
 <96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.1680185025.git.kamrankhadijadj@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.1680185025.git.kamrankhadijadj@gmail.com>
Precedence: bulk

On Thu, Mar 30, 2023 at 07:11:25PM +0500, Khadija Kamran wrote:
> Linux kernel coding-style suggests to fix your program if it needs more
> than 3 levels of indentation. Due to indentation, line length also
> exceeds 100 columns, resulting in issues reported by checkpatch.
> 
> Refactor the arche_platform_wd_irq() function and reduce the indentation
> with the help of goto statement.
> 
> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 79 ++++++++++++------------
>  1 file changed, 41 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index a64c1af091b0..dde30c8da1a1 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -158,49 +158,52 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  
>  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
>  
> -	if (gpiod_get_value(arche_pdata->wake_detect)) {
> -		/* wake/detect rising */
> +	if (!gpiod_get_value(arche_pdata->wake_detect))
> +		goto falling;
>  
> +	/* wake/detect rising */
> +
> +	/*
> +	 * If wake/detect line goes high after low, within less than
> +	 * 30msec, then standby boot sequence is initiated, which is not
> +	 * supported/implemented as of now. So ignore it.
> +	 */
> +	if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)
> +		goto out;

This checks that we are in WD_STATE_BOOT_INIT state.

> +
> +	if (time_before(jiffies,
> +			arche_pdata->wake_detect_start +
> +			msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> +		arche_platform_set_wake_detect_state(arche_pdata,
> +						     WD_STATE_IDLE);
> +		got out;
> +	}
> +
> +	/* Check we are not in middle of irq thread already */
> +	if (arche_pdata->wake_detect_state !=
> +			WD_STATE_COLDBOOT_START) {

This checks that we are not in WD_STATE_COLDBOOT_START state.  How are
we going to be in COLDBOOT if we are in INIT?  Is this changing in the
background?  Can this check be removed?  This might be took tricky to
answer but it's important that we understand this before we continue.


> +		arche_platform_set_wake_detect_state(arche_pdata,
> +						     WD_STATE_COLDBOOT_TRIG);
> +		rc = IRQ_WAKE_THREAD;
> +		goto out;
> +	}

Let's assume the above check cannot be removed.

In the original code if gpiod_get_value(arche_pdata->wake_detect)
returned true and arche_pdata->wake_detect_state == WD_STATE_IDLE then
it just returned without doing anything, but now we fall through to the
falling: label below.

So this patch seems like it introduces a bug, but actually it might just
have a dead code problem.

regards,
dan carpenter

> +
> +falling:
> +	/* wake/detect falling */
> +	if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> +		arche_pdata->wake_detect_start = jiffies;
>  		/*
> -		 * If wake/detect line goes high after low, within less than
> -		 * 30msec, then standby boot sequence is initiated, which is not
> -		 * supported/implemented as of now. So ignore it.
> +		 * In the beginning, when wake/detect goes low
> +		 * (first time), we assume it is meant for coldboot
> +		 * and set the flag. If wake/detect line stays low
> +		 * beyond 30msec, then it is coldboot else fallback
> +		 * to standby boot.
>  		 */
> -		if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> -			if (time_before(jiffies,
> -					arche_pdata->wake_detect_start +
> -					msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> -				arche_platform_set_wake_detect_state(arche_pdata,
> -								     WD_STATE_IDLE);
> -			} else {
> -				/*
> -				 * Check we are not in middle of irq thread
> -				 * already
> -				 */
> -				if (arche_pdata->wake_detect_state !=
> -						WD_STATE_COLDBOOT_START) {
> -					arche_platform_set_wake_detect_state(arche_pdata,
> -									     WD_STATE_COLDBOOT_TRIG);
> -					rc = IRQ_WAKE_THREAD;
> -				}
> -			}
> -		}
> -	} else {
> -		/* wake/detect falling */
> -		if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> -			arche_pdata->wake_detect_start = jiffies;
> -			/*
> -			 * In the beginning, when wake/detect goes low
> -			 * (first time), we assume it is meant for coldboot
> -			 * and set the flag. If wake/detect line stays low
> -			 * beyond 30msec, then it is coldboot else fallback
> -			 * to standby boot.
> -			 */
> -			arche_platform_set_wake_detect_state(arche_pdata,
> -							     WD_STATE_BOOT_INIT);
> -		}
> +		arche_platform_set_wake_detect_state(arche_pdata,
> +						     WD_STATE_BOOT_INIT);
>  	}
>  
> +out:
>  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  
>  	return rc;
> -- 
> 2.34.1
>