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:   Mon, 3 Apr 2023 07:29:36 +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: <9688d93f-5ece-4799-898d-98515a98f9be@kili.mountain>
References: <cover.1680185025.git.kamrankhadijadj@gmail.com>
 <96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.1680185025.git.kamrankhadijadj@gmail.com>
 <6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain>
 <ZChs5jB7DMCUnVzr@khadija-virtual-machine>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZChs5jB7DMCUnVzr@khadija-virtual-machine>
Precedence: bulk

On Sat, Apr 01, 2023 at 10:41:58PM +0500, Khadija Kamran wrote:
> On Thu, Mar 30, 2023 at 06:23:19PM +0300, Dan Carpenter wrote:
> > 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.
> 
> Hey Dan,
> 
> Just to clear my undrstanding in the new code, if
> gpiod_get_value(arche_pdata->wake_detect) returned true, we go to the
> rising section. In the rising section if arche_pdata->wake_detect_state
> == WD_STATE_IDLE then the condition,
> if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT) becomes true
> so we goto out label. I do not understand how we go to falling label.
> 
> Regards,
> Khadija

Let me show you in the original code:

STEP 1:	if (gpiod_get_value(arche_pdata->wake_detect)) {

Assume this condition is true.

STEP 2:		if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {

Assume this condition is true.

STEP 3:		if (time_before(jiffies, ...)

Assume that time is up.

STEP 4:			if (arche_pdata->wake_detect_state !=
					WD_STATE_COLDBOOT_START) {

Assume that it is equal.  We are done.  return IRQ_HANDLED;

Now in the new code:

STEP 1:	if (!gpiod_get_value(arche_pdata->wake_detect))
		goto falling;

Assume we don't goto falling.

STEP 2:	if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)

Assume that it == WD_STATE_BOOT_INIT.

STEP 3:	if (time_before(jiffies, ...)

Assume that time is up.

STEP 4:	if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {

Assume that it is equal.  Before, in the old code it would return
return IRQ_HANDLED; at this point.  But now it does:

STEP 5:	if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {

Which seems like it's a contradictory condition with STEP 4 so it's
probably impossible.  But on the other hand, we have already checked
contradictory conditions between STEP 2 and STEP 4 so who knows what's
going on?

This function is very hard to understand.

Also if stuff is changing in the background and there is no way the
locking is correct.

regards,
dan carpenter