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
arche_platform_wd_irq() function has two exit paths. To make the code
more readable, use only one exit path.
Suggested-by: Alison Schofield <[email protected]>
Signed-off-by: Khadija Kamran <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Khadija Kamran <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Khadija Kamran <[email protected]>
> ---
> 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
>
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> > Signed-off-by: Khadija Kamran <[email protected]>
> > ---
> > 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
> >
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 <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
> Signed-off-by: Khadija Kamran <[email protected]>
> ---
> 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
>
>
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
>
>
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
> >
> >
>
>
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 <[email protected]>
>
> Reviewed-by: Ira Weiny <[email protected]>
>
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/[email protected]/
Thank you!
Regards,
Khadija
> > Signed-off-by: Khadija Kamran <[email protected]>
> > ---
> > 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
> >
> >
>
>
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 <[email protected]>
> > Signed-off-by: Khadija Kamran <[email protected]>
> > ---
> > 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
> >
>
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 <[email protected]>
> >
> > Reviewed-by: Ira Weiny <[email protected]>
> >
>
> 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/[email protected]/
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 <[email protected]>
> > > ---
> > > 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
> > >
> > >
> >
> >
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 <[email protected]>
> > >
> > > Reviewed-by: Ira Weiny <[email protected]>
> > >
> >
> > 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/[email protected]/
>
> 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 <[email protected]>
> > > > ---
> > > > 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
> > > >
> > > >
> > >
> > >
>
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 <[email protected]>
> > > Signed-off-by: Khadija Kamran <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> >
> > Reviewed-by: Ira Weiny <[email protected]>
> >
>
> 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/[email protected]/
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/
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 {
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 <[email protected]>
> > > > Signed-off-by: Khadija Kamran <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
> > > > > Signed-off-by: Khadija Kamran <[email protected]>
> > > > > ---
> > > > > 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
>