outreachy.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
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()
Date: Mon, 3 Apr 2023 07:29:36 +0300	[thread overview]
Message-ID: <9688d93f-5ece-4799-898d-98515a98f9be@kili.mountain> (raw)
In-Reply-To: <ZChs5jB7DMCUnVzr@khadija-virtual-machine>

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

  reply	other threads:[~2023-04-03  4:29 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-30 14:11 [PATCH 0/2] improve arche_platform_wd_irq() function Khadija Kamran
2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran
2023-04-03  0:37   ` Ira Weiny
2023-04-03  1:21     ` Khadija Kamran
2023-04-03  3:07       ` Alison Schofield
2023-04-03  3:15         ` Alison Schofield
2023-04-03 14:59       ` Ira Weiny
2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran
2023-03-30 15:23   ` Dan Carpenter
2023-04-01 17:41     ` Khadija Kamran
2023-04-03  4:29       ` Dan Carpenter [this message]
2023-04-03 15:23         ` Dan Carpenter
2023-04-27  6:27         ` Khadija Kamran
2023-05-03 22:16           ` Alison Schofield
2023-04-03  2:18     ` Ira Weiny
2023-03-30 16:58   ` kernel test robot
2023-04-03  0:36 ` [PATCH 0/2] improve arche_platform_wd_irq() function Ira Weiny
2023-04-03  1:18   ` Khadija Kamran

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9688d93f-5ece-4799-898d-98515a98f9be@kili.mountain \
    --to=error27@gmail.com \
    --cc=alison.schofield@intel.com \
    --cc=elder@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=greybus-dev@lists.linaro.org \
    --cc=hvaibhav.linux@gmail.com \
    --cc=johan@kernel.org \
    --cc=kamrankhadijadj@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=outreachy@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).