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(-)
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; }
/*
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
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....
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
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....
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
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....
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
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....
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
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;
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
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
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
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 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
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
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
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-greybu... patch link: https://lore.kernel.org/r/96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.168018502... 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@i...) 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/fd0907bb290e9a4f8b33d8c56ca14a... 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
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
-
Alison Schofield -
Dan Carpenter -
Ira Weiny -
kernel test robot -
Khadija Kamran