Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq()

[Khadija Kamran]

On Mon, Apr 03, 2023 at 07:29:36AM +0300, Dan Carpenter wrote:
> 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@xxxxxxxxx>
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@xxxxxxxxx>
> > > > ---
> > > >  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.
> 
> 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?


Hey Dan!

Sorry about the delay in the reply. I want to continue working on the
completion of this thread. 

Thank you for the above steps, they clearly explain the problem you
addressed in the new code. Can't this problem be resolved by "goto out;"
right above the 'falling:' label?

I'm copy pasting your mail here,

On Mon, Apr 03, 2023 at 07:29:36AM +0300, Dan Carpenter wrote:
> > > 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?

> It turned out the answer was yes, it can be removed.

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

> The locking is correct.  Take a look at it.

> We are holding the &arche_pdata->wake_lock in arche_platform_wd_irq()
> and every place that calls arche_platform_set_wake_detect_state() takes
> that lock first.  So it can't change in the background.

> Delete the check like so.

If we delete the check then the indentation problem would be
automatically addressed. Also, there would be a single exit path to the
function. Should I send a patch or is there anything else that should be
addressed. 

Thank you!

Regards,
Khadija

> 
> 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