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

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Apr 27, 2023 at 11:27:51AM +0500, Khadija Kamran wrote:
> 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@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.
> > 
> > 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. 

Hi Khadija,

Thanks for picking this up again. I suggest posting an updated version
and let folks take a look at it again. It's a bit stale in my mind now,
but I'm happy to iterate on it with you further.

Thanks,
Alison

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