[PATCH 0/2] improve arche_platform_wd_irq() function

[PATCH 0/2] improve arche_platform_wd_irq() function

[Khadija Kamran]

Improve arche_platform_wd_irq() function to minimize indentation and fix
checkpatch issues.

Khadija Kamran (2):
  staging: greybus: add a single exit path to arche_platform_wd_irq()
  staging: greybus: refactor arche_platform_wd_irq()

 drivers/staging/greybus/arche-platform.c | 84 ++++++++++++------------
 1 file changed, 43 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Khadija Kamran]

arche_platform_wd_irq() function has two exit paths. To make the code
more readable, use only one exit path.

Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---
 drivers/staging/greybus/arche-platform.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..a64c1af091b0 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid)
 static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 {
 	struct arche_platform_drvdata *arche_pdata = devid;
+	irqreturn_t rc = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
@@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 						WD_STATE_COLDBOOT_START) {
 					arche_platform_set_wake_detect_state(arche_pdata,
 									     WD_STATE_COLDBOOT_TRIG);
-					spin_unlock_irqrestore(&arche_pdata->wake_lock,
-							       flags);
-					return IRQ_WAKE_THREAD;
+					rc = IRQ_WAKE_THREAD;
 				}
 			}
 		}
@@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 
 	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
 
-	return IRQ_HANDLED;
+	return rc;
 }
 
 /*
-- 
2.34.1

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

[Khadija Kamran]

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;
+
+	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) {
+		arche_platform_set_wake_detect_state(arche_pdata,
+						     WD_STATE_COLDBOOT_TRIG);
+		rc = IRQ_WAKE_THREAD;
+		goto out;
+	}
+
+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

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

[Dan Carpenter]

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
> 

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

[kernel test robot]

Hi Khadija,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on staging/staging-next staging/staging-linus linus/master v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-greybus-add-a-single-exit-path-to-arche_platform_wd_irq/20230330-222140
patch link:    https://lore.kernel.org/r/96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.1680185025.git.kamrankhadijadj%40gmail.com
patch subject: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq()
config: microblaze-randconfig-r016-20230329 (https://download.01.org/0day-ci/archive/20230331/202303310037.mGo4pYNd-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/fd0907bb290e9a4f8b33d8c56ca14a49e3177de9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Khadija-Kamran/staging-greybus-add-a-single-exit-path-to-arche_platform_wd_irq/20230330-222140
        git checkout fd0907bb290e9a4f8b33d8c56ca14a49e3177de9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/staging/greybus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303310037.mGo4pYNd-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/staging/greybus/arche-platform.c: In function 'arche_platform_wd_irq':
>> drivers/staging/greybus/arche-platform.c:179:17: error: unknown type name 'got'
     179 |                 got out;
         |                 ^~~
>> drivers/staging/greybus/arche-platform.c:179:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   drivers/staging/greybus/arche-platform.c:179:21: warning: unused variable 'out' [-Wunused-variable]
     179 |                 got out;
         |                     ^~~
   drivers/staging/greybus/arche-platform.c: At top level:
   drivers/staging/greybus/arche-platform.c:626:34: warning: 'arche_combined_id' defined but not used [-Wunused-const-variable=]
     626 | static const struct of_device_id arche_combined_id[] = {
         |                                  ^~~~~~~~~~~~~~~~~


vim +/got +179 drivers/staging/greybus/arche-platform.c

   152	
   153	static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
   154	{
   155		struct arche_platform_drvdata *arche_pdata = devid;
   156		irqreturn_t rc = IRQ_HANDLED;
   157		unsigned long flags;
   158	
   159		spin_lock_irqsave(&arche_pdata->wake_lock, flags);
   160	
   161		if (!gpiod_get_value(arche_pdata->wake_detect))
   162			goto falling;
   163	
   164		/* wake/detect rising */
   165	
   166		/*
   167		 * If wake/detect line goes high after low, within less than
   168		 * 30msec, then standby boot sequence is initiated, which is not
   169		 * supported/implemented as of now. So ignore it.
   170		 */
   171		if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)
   172			goto out;
   173	
   174		if (time_before(jiffies,
   175				arche_pdata->wake_detect_start +
   176				msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
   177			arche_platform_set_wake_detect_state(arche_pdata,
   178							     WD_STATE_IDLE);
 > 179			got out;
   180		}
   181	
   182		/* Check we are not in middle of irq thread already */
   183		if (arche_pdata->wake_detect_state !=
   184				WD_STATE_COLDBOOT_START) {
   185			arche_platform_set_wake_detect_state(arche_pdata,
   186							     WD_STATE_COLDBOOT_TRIG);
   187			rc = IRQ_WAKE_THREAD;
   188			goto out;
   189		}
   190	
   191	falling:
   192		/* wake/detect falling */
   193		if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
   194			arche_pdata->wake_detect_start = jiffies;
   195			/*
   196			 * In the beginning, when wake/detect goes low
   197			 * (first time), we assume it is meant for coldboot
   198			 * and set the flag. If wake/detect line stays low
   199			 * beyond 30msec, then it is coldboot else fallback
   200			 * to standby boot.
   201			 */
   202			arche_platform_set_wake_detect_state(arche_pdata,
   203							     WD_STATE_BOOT_INIT);
   204		}
   205	
   206	out:
   207		spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
   208	
   209		return rc;
   210	}
   211	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Khadija Kamran]

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

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

Re: [PATCH 0/2] improve arche_platform_wd_irq() function

[Ira Weiny]

Khadija Kamran wrote:
> Improve arche_platform_wd_irq() function to minimize indentation and fix
> checkpatch issues.

Minor point but the cover should have

'staging: greybus: ...'

... for the subject.

The function name is nice but it is a pain to look in the code to know
what part of the kernel this series is for.

Ira

> 
> Khadija Kamran (2):
>   staging: greybus: add a single exit path to arche_platform_wd_irq()
>   staging: greybus: refactor arche_platform_wd_irq()
> 
>  drivers/staging/greybus/arche-platform.c | 84 ++++++++++++------------
>  1 file changed, 43 insertions(+), 41 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Ira Weiny]

Khadija Kamran wrote:
> arche_platform_wd_irq() function has two exit paths. To make the code
> more readable, use only one exit path.
> 
> Suggested-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..a64c1af091b0 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid)
>  static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  {
>  	struct arche_platform_drvdata *arche_pdata = devid;
> +	irqreturn_t rc = IRQ_HANDLED;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  						WD_STATE_COLDBOOT_START) {
>  					arche_platform_set_wake_detect_state(arche_pdata,
>  									     WD_STATE_COLDBOOT_TRIG);
> -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -							       flags);
> -					return IRQ_WAKE_THREAD;
> +					rc = IRQ_WAKE_THREAD;
>  				}
>  			}
>  		}
> @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  
>  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  
> -	return IRQ_HANDLED;
> +	return rc;
>  }
>  
>  /*
> -- 
> 2.34.1
> 
> 

Re: [PATCH 0/2] improve arche_platform_wd_irq() function

[Khadija Kamran]

On Sun, Apr 02, 2023 at 05:36:24PM -0700, Ira Weiny wrote:
> Khadija Kamran wrote:
> > Improve arche_platform_wd_irq() function to minimize indentation and fix
> > checkpatch issues.
> 
> Minor point but the cover should have
> 
> 'staging: greybus: ...'
> 
> ... for the subject.
>

Hey Ira,

Sorry, my bad! I missed it by mistake.

Regards,
Khadija

> The function name is nice but it is a pain to look in the code to know
> what part of the kernel this series is for.
> 
> Ira
> 
> > 
> > Khadija Kamran (2):
> >   staging: greybus: add a single exit path to arche_platform_wd_irq()
> >   staging: greybus: refactor arche_platform_wd_irq()
> > 
> >  drivers/staging/greybus/arche-platform.c | 84 ++++++++++++------------
> >  1 file changed, 43 insertions(+), 41 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 
> > 
> 
> 

Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Khadija Kamran]

On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote:
> Khadija Kamran wrote:
> > arche_platform_wd_irq() function has two exit paths. To make the code
> > more readable, use only one exit path.
> > 
> > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>

Okay, noted.

Also, would it be okay to send a patch revision with the changes or
should I wait for the feedback on Dan's comment. Here is a link to it:
https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/

Thank you!
Regards,
Khadija 

> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> >  drivers/staging/greybus/arche-platform.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index fcbd5f71eff2..a64c1af091b0 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid)
> >  static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  {
> >  	struct arche_platform_drvdata *arche_pdata = devid;
> > +	irqreturn_t rc = IRQ_HANDLED;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  						WD_STATE_COLDBOOT_START) {
> >  					arche_platform_set_wake_detect_state(arche_pdata,
> >  									     WD_STATE_COLDBOOT_TRIG);
> > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > -							       flags);
> > -					return IRQ_WAKE_THREAD;
> > +					rc = IRQ_WAKE_THREAD;
> >  				}
> >  			}
> >  		}
> > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  
> >  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> >  
> > -	return IRQ_HANDLED;
> > +	return rc;
> >  }
> >  
> >  /*
> > -- 
> > 2.34.1
> > 
> > 
> 
> 

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

[Ira Weiny]

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;

I don't like this negative logic here.  Can't this be handled with
positive logic?

> >  
> > +	/* 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.

Doesn't this check we are _not_ in 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);
> > +		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.

I don't believe we do.  But I think the proposed logic does make this
difficult to detect.

Ira

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

Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Alison Schofield]

On Mon, Apr 03, 2023 at 06:21:53AM +0500, Khadija Kamran wrote:
> On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote:
> > Khadija Kamran wrote:
> > > arche_platform_wd_irq() function has two exit paths. To make the code
> > > more readable, use only one exit path.
> > > 
> > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > 
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> >
> 
> Okay, noted.
> 
> Also, would it be okay to send a patch revision with the changes or
> should I wait for the feedback on Dan's comment. Here is a link to it:
> https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/

Khadija,

It's customary to wait for folks to respond to your follow ups, and
address  all the current feedback before sending out a new revision.

Ira asked a question about using positive instead of negative logic.
I probably steered you down the negative logic path, perhaps it can
be flipped to a more preferable positive logic. 

Alison



> 
> Thank you!
> Regards,
> Khadija 
> 
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > ---
> > >  drivers/staging/greybus/arche-platform.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > > index fcbd5f71eff2..a64c1af091b0 100644
> > > --- a/drivers/staging/greybus/arche-platform.c
> > > +++ b/drivers/staging/greybus/arche-platform.c
> > > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid)
> > >  static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >  {
> > >  	struct arche_platform_drvdata *arche_pdata = devid;
> > > +	irqreturn_t rc = IRQ_HANDLED;
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> > > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >  						WD_STATE_COLDBOOT_START) {
> > >  					arche_platform_set_wake_detect_state(arche_pdata,
> > >  									     WD_STATE_COLDBOOT_TRIG);
> > > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > > -							       flags);
> > > -					return IRQ_WAKE_THREAD;
> > > +					rc = IRQ_WAKE_THREAD;
> > >  				}
> > >  			}
> > >  		}
> > > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >  
> > >  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > >  
> > > -	return IRQ_HANDLED;
> > > +	return rc;
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > 
> > 

Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Alison Schofield]

On Sun, Apr 02, 2023 at 08:07:27PM -0700, Alison Schofield wrote:
> On Mon, Apr 03, 2023 at 06:21:53AM +0500, Khadija Kamran wrote:
> > On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote:
> > > Khadija Kamran wrote:
> > > > arche_platform_wd_irq() function has two exit paths. To make the code
> > > > more readable, use only one exit path.
> > > > 
> > > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > >
> > 
> > Okay, noted.
> > 
> > Also, would it be okay to send a patch revision with the changes or
> > should I wait for the feedback on Dan's comment. Here is a link to it:
> > https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/
> 
> Khadija,
> 
> It's customary to wait for folks to respond to your follow ups, and
> address  all the current feedback before sending out a new revision.
> 
> Ira asked a question about using positive instead of negative logic.
> I probably steered you down the negative logic path, perhaps it can
> be flipped to a more preferable positive logic. 

After I hit send, I worried that wasn't written clearly.
All the current feedback includes, Dan's, Ira',s and the compile issue.
(not only Ira's)

> 
> Alison
> 
> 
> 
> > 
> > Thank you!
> > Regards,
> > Khadija 
> > 
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > > ---
> > > >  drivers/staging/greybus/arche-platform.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > > > index fcbd5f71eff2..a64c1af091b0 100644
> > > > --- a/drivers/staging/greybus/arche-platform.c
> > > > +++ b/drivers/staging/greybus/arche-platform.c
> > > > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid)
> > > >  static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > > >  {
> > > >  	struct arche_platform_drvdata *arche_pdata = devid;
> > > > +	irqreturn_t rc = IRQ_HANDLED;
> > > >  	unsigned long flags;
> > > >  
> > > >  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> > > > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > > >  						WD_STATE_COLDBOOT_START) {
> > > >  					arche_platform_set_wake_detect_state(arche_pdata,
> > > >  									     WD_STATE_COLDBOOT_TRIG);
> > > > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > > > -							       flags);
> > > > -					return IRQ_WAKE_THREAD;
> > > > +					rc = IRQ_WAKE_THREAD;
> > > >  				}
> > > >  			}
> > > >  		}
> > > > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > > >  
> > > >  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > > >  
> > > > -	return IRQ_HANDLED;
> > > > +	return rc;
> > > >  }
> > > >  
> > > >  /*
> > > > -- 
> > > > 2.34.1
> > > > 
> > > > 
> > > 
> > > 
> 

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

[Dan Carpenter]

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

Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq()

[Ira Weiny]

Khadija Kamran wrote:
> On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote:
> > Khadija Kamran wrote:
> > > arche_platform_wd_irq() function has two exit paths. To make the code
> > > more readable, use only one exit path.
> > > 
> > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > 
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> >
> 
> Okay, noted.
> 
> Also, would it be okay to send a patch revision with the changes or
> should I wait for the feedback on Dan's comment. Here is a link to it:
> https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/

In this case you are going to need to make a v2 of the _series_.  In the
next revision you send both patches again as V2.  (If you are using b4[*]
it will help to track the series versions.)

I am saying this patch looks good but there are still issues with patch
2/2.  If there are no other comments on 1/2 and you don't make any changes
then you include it with my review tag as v2.

When I do this I make a note of picking up tags like so:

---
Changes from V1:
	Add review tags

If you get other comments and make changes then my review is no longer
valid because you have changed the patch.  In that case make those changes
and make a note.  Then I can see why my review was dropped and, more
importantly, I know why I should look at the patch again.

Then in patch 2/2 make changes as needed.

Ira

[*] https://b4.docs.kernel.org/en/latest/

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

[Dan Carpenter]

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.

regards,
dan carpenter

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..4cca45ee70b3 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -172,18 +172,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 				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);
-					spin_unlock_irqrestore(&arche_pdata->wake_lock,
-							       flags);
-					return IRQ_WAKE_THREAD;
-				}
+				arche_platform_set_wake_detect_state(arche_pdata,
+								     WD_STATE_COLDBOOT_TRIG);
+				spin_unlock_irqrestore(&arche_pdata->wake_lock,
+						       flags);
+				return IRQ_WAKE_THREAD;
 			}
 		}
 	} else {

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

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

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

[Alison Schofield]

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
>