* [PATCH 0/2] improve arche_platform_wd_irq() function
@ 2023-03-30 14:11 Khadija Kamran
2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Khadija Kamran @ 2023-03-30 14:11 UTC (permalink / raw)
To: outreachy
Cc: hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging,
linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-03-30 14:11 [PATCH 0/2] improve arche_platform_wd_irq() function Khadija Kamran @ 2023-03-30 14:11 ` Khadija Kamran 2023-04-03 0:37 ` Ira Weiny 2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran 2023-04-03 0:36 ` [PATCH 0/2] improve arche_platform_wd_irq() function Ira Weiny 2 siblings, 1 reply; 18+ messages in thread From: Khadija Kamran @ 2023-03-30 14:11 UTC (permalink / raw) To: outreachy Cc: hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield arche_platform_wd_irq() function has two exit paths. To make the code more readable, use only one exit path. Suggested-by: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> --- drivers/staging/greybus/arche-platform.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c index fcbd5f71eff2..a64c1af091b0 100644 --- a/drivers/staging/greybus/arche-platform.c +++ b/drivers/staging/greybus/arche-platform.c @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid) static irqreturn_t arche_platform_wd_irq(int irq, void *devid) { struct arche_platform_drvdata *arche_pdata = devid; + irqreturn_t rc = IRQ_HANDLED; unsigned long flags; spin_lock_irqsave(&arche_pdata->wake_lock, flags); @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) WD_STATE_COLDBOOT_START) { arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG); - spin_unlock_irqrestore(&arche_pdata->wake_lock, - flags); - return IRQ_WAKE_THREAD; + rc = IRQ_WAKE_THREAD; } } } @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); - return IRQ_HANDLED; + return rc; } /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran @ 2023-04-03 0:37 ` Ira Weiny 2023-04-03 1:21 ` Khadija Kamran 0 siblings, 1 reply; 18+ messages in thread From: Ira Weiny @ 2023-04-03 0:37 UTC (permalink / raw) To: Khadija Kamran, outreachy Cc: hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-04-03 0:37 ` Ira Weiny @ 2023-04-03 1:21 ` Khadija Kamran 2023-04-03 3:07 ` Alison Schofield 2023-04-03 14:59 ` Ira Weiny 0 siblings, 2 replies; 18+ messages in thread From: Khadija Kamran @ 2023-04-03 1:21 UTC (permalink / raw) To: Ira Weiny Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote: > Khadija Kamran wrote: > > arche_platform_wd_irq() function has two exit paths. To make the code > > more readable, use only one exit path. > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Okay, noted. Also, would it be okay to send a patch revision with the changes or should I wait for the feedback on Dan's comment. Here is a link to it: https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/ Thank you! Regards, Khadija > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> > > --- > > drivers/staging/greybus/arche-platform.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > > index fcbd5f71eff2..a64c1af091b0 100644 > > --- a/drivers/staging/greybus/arche-platform.c > > +++ b/drivers/staging/greybus/arche-platform.c > > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid) > > static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > { > > struct arche_platform_drvdata *arche_pdata = devid; > > + irqreturn_t rc = IRQ_HANDLED; > > unsigned long flags; > > > > spin_lock_irqsave(&arche_pdata->wake_lock, flags); > > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > WD_STATE_COLDBOOT_START) { > > arche_platform_set_wake_detect_state(arche_pdata, > > WD_STATE_COLDBOOT_TRIG); > > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > > - flags); > > - return IRQ_WAKE_THREAD; > > + rc = IRQ_WAKE_THREAD; > > } > > } > > } > > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > > > - return IRQ_HANDLED; > > + return rc; > > } > > > > /* > > -- > > 2.34.1 > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-04-03 1:21 ` Khadija Kamran @ 2023-04-03 3:07 ` Alison Schofield 2023-04-03 3:15 ` Alison Schofield 2023-04-03 14:59 ` Ira Weiny 1 sibling, 1 reply; 18+ messages in thread From: Alison Schofield @ 2023-04-03 3:07 UTC (permalink / raw) To: Khadija Kamran Cc: Ira Weiny, outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel On Mon, Apr 03, 2023 at 06:21:53AM +0500, Khadija Kamran wrote: > On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote: > > Khadija Kamran wrote: > > > arche_platform_wd_irq() function has two exit paths. To make the code > > > more readable, use only one exit path. > > > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > Okay, noted. > > Also, would it be okay to send a patch revision with the changes or > should I wait for the feedback on Dan's comment. Here is a link to it: > https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/ Khadija, It's customary to wait for folks to respond to your follow ups, and address all the current feedback before sending out a new revision. Ira asked a question about using positive instead of negative logic. I probably steered you down the negative logic path, perhaps it can be flipped to a more preferable positive logic. Alison > > Thank you! > Regards, > Khadija > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> > > > --- > > > drivers/staging/greybus/arche-platform.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > > > index fcbd5f71eff2..a64c1af091b0 100644 > > > --- a/drivers/staging/greybus/arche-platform.c > > > +++ b/drivers/staging/greybus/arche-platform.c > > > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid) > > > static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > { > > > struct arche_platform_drvdata *arche_pdata = devid; > > > + irqreturn_t rc = IRQ_HANDLED; > > > unsigned long flags; > > > > > > spin_lock_irqsave(&arche_pdata->wake_lock, flags); > > > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > WD_STATE_COLDBOOT_START) { > > > arche_platform_set_wake_detect_state(arche_pdata, > > > WD_STATE_COLDBOOT_TRIG); > > > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > > > - flags); > > > - return IRQ_WAKE_THREAD; > > > + rc = IRQ_WAKE_THREAD; > > > } > > > } > > > } > > > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > > > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > > > > > - return IRQ_HANDLED; > > > + return rc; > > > } > > > > > > /* > > > -- > > > 2.34.1 > > > > > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-04-03 3:07 ` Alison Schofield @ 2023-04-03 3:15 ` Alison Schofield 0 siblings, 0 replies; 18+ messages in thread From: Alison Schofield @ 2023-04-03 3:15 UTC (permalink / raw) To: Khadija Kamran Cc: Ira Weiny, outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel On Sun, Apr 02, 2023 at 08:07:27PM -0700, Alison Schofield wrote: > On Mon, Apr 03, 2023 at 06:21:53AM +0500, Khadija Kamran wrote: > > On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote: > > > Khadija Kamran wrote: > > > > arche_platform_wd_irq() function has two exit paths. To make the code > > > > more readable, use only one exit path. > > > > > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > > > > Okay, noted. > > > > Also, would it be okay to send a patch revision with the changes or > > should I wait for the feedback on Dan's comment. Here is a link to it: > > https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/ > > Khadija, > > It's customary to wait for folks to respond to your follow ups, and > address all the current feedback before sending out a new revision. > > Ira asked a question about using positive instead of negative logic. > I probably steered you down the negative logic path, perhaps it can > be flipped to a more preferable positive logic. After I hit send, I worried that wasn't written clearly. All the current feedback includes, Dan's, Ira',s and the compile issue. (not only Ira's) > > Alison > > > > > > > Thank you! > > Regards, > > Khadija > > > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> > > > > --- > > > > drivers/staging/greybus/arche-platform.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > > > > index fcbd5f71eff2..a64c1af091b0 100644 > > > > --- a/drivers/staging/greybus/arche-platform.c > > > > +++ b/drivers/staging/greybus/arche-platform.c > > > > @@ -153,6 +153,7 @@ static irqreturn_t arche_platform_wd_irq_thread(int irq, void *devid) > > > > static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > { > > > > struct arche_platform_drvdata *arche_pdata = devid; > > > > + irqreturn_t rc = IRQ_HANDLED; > > > > unsigned long flags; > > > > > > > > spin_lock_irqsave(&arche_pdata->wake_lock, flags); > > > > @@ -180,9 +181,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > WD_STATE_COLDBOOT_START) { > > > > arche_platform_set_wake_detect_state(arche_pdata, > > > > WD_STATE_COLDBOOT_TRIG); > > > > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > > > > - flags); > > > > - return IRQ_WAKE_THREAD; > > > > + rc = IRQ_WAKE_THREAD; > > > > } > > > > } > > > > } > > > > @@ -204,7 +203,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > > > > > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > > > > > > > - return IRQ_HANDLED; > > > > + return rc; > > > > } > > > > > > > > /* > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() 2023-04-03 1:21 ` Khadija Kamran 2023-04-03 3:07 ` Alison Schofield @ 2023-04-03 14:59 ` Ira Weiny 1 sibling, 0 replies; 18+ messages in thread From: Ira Weiny @ 2023-04-03 14:59 UTC (permalink / raw) To: Khadija Kamran, Ira Weiny Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield Khadija Kamran wrote: > On Sun, Apr 02, 2023 at 05:37:48PM -0700, Ira Weiny wrote: > > Khadija Kamran wrote: > > > arche_platform_wd_irq() function has two exit paths. To make the code > > > more readable, use only one exit path. > > > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > Okay, noted. > > Also, would it be okay to send a patch revision with the changes or > should I wait for the feedback on Dan's comment. Here is a link to it: > https://lore.kernel.org/outreachy/6ce8aa34-e600-4d6a-adad-ead8255342e5@kili.mountain/ In this case you are going to need to make a v2 of the _series_. In the next revision you send both patches again as V2. (If you are using b4[*] it will help to track the series versions.) I am saying this patch looks good but there are still issues with patch 2/2. If there are no other comments on 1/2 and you don't make any changes then you include it with my review tag as v2. When I do this I make a note of picking up tags like so: --- Changes from V1: Add review tags If you get other comments and make changes then my review is no longer valid because you have changed the patch. In that case make those changes and make a note. Then I can see why my review was dropped and, more importantly, I know why I should look at the patch again. Then in patch 2/2 make changes as needed. Ira [*] https://b4.docs.kernel.org/en/latest/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-03-30 14:11 [PATCH 0/2] improve arche_platform_wd_irq() function Khadija Kamran 2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran @ 2023-03-30 14:11 ` Khadija Kamran 2023-03-30 15:23 ` Dan Carpenter 2023-03-30 16:58 ` kernel test robot 2023-04-03 0:36 ` [PATCH 0/2] improve arche_platform_wd_irq() function Ira Weiny 2 siblings, 2 replies; 18+ messages in thread From: Khadija Kamran @ 2023-03-30 14:11 UTC (permalink / raw) To: outreachy Cc: hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield Linux kernel coding-style suggests to fix your program if it needs more than 3 levels of indentation. Due to indentation, line length also exceeds 100 columns, resulting in issues reported by checkpatch. Refactor the arche_platform_wd_irq() function and reduce the indentation with the help of goto statement. Suggested-by: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> --- drivers/staging/greybus/arche-platform.c | 79 ++++++++++++------------ 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c index a64c1af091b0..dde30c8da1a1 100644 --- a/drivers/staging/greybus/arche-platform.c +++ b/drivers/staging/greybus/arche-platform.c @@ -158,49 +158,52 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) spin_lock_irqsave(&arche_pdata->wake_lock, flags); - if (gpiod_get_value(arche_pdata->wake_detect)) { - /* wake/detect rising */ + if (!gpiod_get_value(arche_pdata->wake_detect)) + goto falling; + /* wake/detect rising */ + + /* + * If wake/detect line goes high after low, within less than + * 30msec, then standby boot sequence is initiated, which is not + * supported/implemented as of now. So ignore it. + */ + if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT) + goto out; + + if (time_before(jiffies, + arche_pdata->wake_detect_start + + msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) { + arche_platform_set_wake_detect_state(arche_pdata, + WD_STATE_IDLE); + got out; + } + + /* Check we are not in middle of irq thread already */ + if (arche_pdata->wake_detect_state != + WD_STATE_COLDBOOT_START) { + arche_platform_set_wake_detect_state(arche_pdata, + WD_STATE_COLDBOOT_TRIG); + rc = IRQ_WAKE_THREAD; + goto out; + } + +falling: + /* wake/detect falling */ + if (arche_pdata->wake_detect_state == WD_STATE_IDLE) { + arche_pdata->wake_detect_start = jiffies; /* - * If wake/detect line goes high after low, within less than - * 30msec, then standby boot sequence is initiated, which is not - * supported/implemented as of now. So ignore it. + * In the beginning, when wake/detect goes low + * (first time), we assume it is meant for coldboot + * and set the flag. If wake/detect line stays low + * beyond 30msec, then it is coldboot else fallback + * to standby boot. */ - if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) { - if (time_before(jiffies, - arche_pdata->wake_detect_start + - msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) { - arche_platform_set_wake_detect_state(arche_pdata, - WD_STATE_IDLE); - } else { - /* - * Check we are not in middle of irq thread - * already - */ - if (arche_pdata->wake_detect_state != - WD_STATE_COLDBOOT_START) { - arche_platform_set_wake_detect_state(arche_pdata, - WD_STATE_COLDBOOT_TRIG); - rc = IRQ_WAKE_THREAD; - } - } - } - } else { - /* wake/detect falling */ - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) { - arche_pdata->wake_detect_start = jiffies; - /* - * In the beginning, when wake/detect goes low - * (first time), we assume it is meant for coldboot - * and set the flag. If wake/detect line stays low - * beyond 30msec, then it is coldboot else fallback - * to standby boot. - */ - arche_platform_set_wake_detect_state(arche_pdata, - WD_STATE_BOOT_INIT); - } + arche_platform_set_wake_detect_state(arche_pdata, + WD_STATE_BOOT_INIT); } +out: spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); return rc; -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran @ 2023-03-30 15:23 ` Dan Carpenter 2023-04-01 17:41 ` Khadija Kamran 2023-04-03 2:18 ` Ira Weiny 2023-03-30 16:58 ` kernel test robot 1 sibling, 2 replies; 18+ messages in thread From: Dan Carpenter @ 2023-03-30 15:23 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-03-30 15:23 ` Dan Carpenter @ 2023-04-01 17:41 ` Khadija Kamran 2023-04-03 4:29 ` Dan Carpenter 2023-04-03 2:18 ` Ira Weiny 1 sibling, 1 reply; 18+ messages in thread From: Khadija Kamran @ 2023-04-01 17:41 UTC (permalink / raw) To: Dan Carpenter Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-04-01 17:41 ` Khadija Kamran @ 2023-04-03 4:29 ` Dan Carpenter 2023-04-03 15:23 ` Dan Carpenter 2023-04-27 6:27 ` Khadija Kamran 0 siblings, 2 replies; 18+ messages in thread From: Dan Carpenter @ 2023-04-03 4:29 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-04-03 4:29 ` Dan Carpenter @ 2023-04-03 15:23 ` Dan Carpenter 2023-04-27 6:27 ` Khadija Kamran 1 sibling, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2023-04-03 15:23 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 { ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-04-03 4:29 ` Dan Carpenter 2023-04-03 15:23 ` Dan Carpenter @ 2023-04-27 6:27 ` Khadija Kamran 2023-05-03 22:16 ` Alison Schofield 1 sibling, 1 reply; 18+ messages in thread From: Khadija Kamran @ 2023-04-27 6:27 UTC (permalink / raw) To: Dan Carpenter Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-04-27 6:27 ` Khadija Kamran @ 2023-05-03 22:16 ` Alison Schofield 0 siblings, 0 replies; 18+ messages in thread From: Alison Schofield @ 2023-05-03 22:16 UTC (permalink / raw) To: Khadija Kamran Cc: Dan Carpenter, outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-03-30 15:23 ` Dan Carpenter 2023-04-01 17:41 ` Khadija Kamran @ 2023-04-03 2:18 ` Ira Weiny 1 sibling, 0 replies; 18+ messages in thread From: Ira Weiny @ 2023-04-03 2:18 UTC (permalink / raw) To: Dan Carpenter, Khadija Kamran Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield 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 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() 2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran 2023-03-30 15:23 ` Dan Carpenter @ 2023-03-30 16:58 ` kernel test robot 1 sibling, 0 replies; 18+ messages in thread From: kernel test robot @ 2023-03-30 16:58 UTC (permalink / raw) To: Khadija Kamran, outreachy Cc: oe-kbuild-all, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel, Alison Schofield Hi Khadija, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on staging/staging-next staging/staging-linus linus/master v6.3-rc4 next-20230330] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-greybus-add-a-single-exit-path-to-arche_platform_wd_irq/20230330-222140 patch link: https://lore.kernel.org/r/96d04a4ff3d4a46293355f5afae3a8ece65f2c5b.1680185025.git.kamrankhadijadj%40gmail.com patch subject: [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() config: microblaze-randconfig-r016-20230329 (https://download.01.org/0day-ci/archive/20230331/202303310037.mGo4pYNd-lkp@intel.com/config) compiler: microblaze-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/fd0907bb290e9a4f8b33d8c56ca14a49e3177de9 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Khadija-Kamran/staging-greybus-add-a-single-exit-path-to-arche_platform_wd_irq/20230330-222140 git checkout fd0907bb290e9a4f8b33d8c56ca14a49e3177de9 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/staging/greybus/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303310037.mGo4pYNd-lkp@intel.com/ All error/warnings (new ones prefixed by >>): drivers/staging/greybus/arche-platform.c: In function 'arche_platform_wd_irq': >> drivers/staging/greybus/arche-platform.c:179:17: error: unknown type name 'got' 179 | got out; | ^~~ >> drivers/staging/greybus/arche-platform.c:179:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] drivers/staging/greybus/arche-platform.c:179:21: warning: unused variable 'out' [-Wunused-variable] 179 | got out; | ^~~ drivers/staging/greybus/arche-platform.c: At top level: drivers/staging/greybus/arche-platform.c:626:34: warning: 'arche_combined_id' defined but not used [-Wunused-const-variable=] 626 | static const struct of_device_id arche_combined_id[] = { | ^~~~~~~~~~~~~~~~~ vim +/got +179 drivers/staging/greybus/arche-platform.c 152 153 static irqreturn_t arche_platform_wd_irq(int irq, void *devid) 154 { 155 struct arche_platform_drvdata *arche_pdata = devid; 156 irqreturn_t rc = IRQ_HANDLED; 157 unsigned long flags; 158 159 spin_lock_irqsave(&arche_pdata->wake_lock, flags); 160 161 if (!gpiod_get_value(arche_pdata->wake_detect)) 162 goto falling; 163 164 /* wake/detect rising */ 165 166 /* 167 * If wake/detect line goes high after low, within less than 168 * 30msec, then standby boot sequence is initiated, which is not 169 * supported/implemented as of now. So ignore it. 170 */ 171 if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT) 172 goto out; 173 174 if (time_before(jiffies, 175 arche_pdata->wake_detect_start + 176 msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) { 177 arche_platform_set_wake_detect_state(arche_pdata, 178 WD_STATE_IDLE); > 179 got out; 180 } 181 182 /* Check we are not in middle of irq thread already */ 183 if (arche_pdata->wake_detect_state != 184 WD_STATE_COLDBOOT_START) { 185 arche_platform_set_wake_detect_state(arche_pdata, 186 WD_STATE_COLDBOOT_TRIG); 187 rc = IRQ_WAKE_THREAD; 188 goto out; 189 } 190 191 falling: 192 /* wake/detect falling */ 193 if (arche_pdata->wake_detect_state == WD_STATE_IDLE) { 194 arche_pdata->wake_detect_start = jiffies; 195 /* 196 * In the beginning, when wake/detect goes low 197 * (first time), we assume it is meant for coldboot 198 * and set the flag. If wake/detect line stays low 199 * beyond 30msec, then it is coldboot else fallback 200 * to standby boot. 201 */ 202 arche_platform_set_wake_detect_state(arche_pdata, 203 WD_STATE_BOOT_INIT); 204 } 205 206 out: 207 spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); 208 209 return rc; 210 } 211 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] improve arche_platform_wd_irq() function 2023-03-30 14:11 [PATCH 0/2] improve arche_platform_wd_irq() function Khadija Kamran 2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran 2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran @ 2023-04-03 0:36 ` Ira Weiny 2023-04-03 1:18 ` Khadija Kamran 2 siblings, 1 reply; 18+ messages in thread From: Ira Weiny @ 2023-04-03 0:36 UTC (permalink / raw) To: Khadija Kamran, outreachy Cc: hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] improve arche_platform_wd_irq() function 2023-04-03 0:36 ` [PATCH 0/2] improve arche_platform_wd_irq() function Ira Weiny @ 2023-04-03 1:18 ` Khadija Kamran 0 siblings, 0 replies; 18+ messages in thread From: Khadija Kamran @ 2023-04-03 1:18 UTC (permalink / raw) To: Ira Weiny Cc: outreachy, hvaibhav.linux, johan, elder, gregkh, greybus-dev, linux-staging, linux-kernel 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 > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-05-03 22:16 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-30 14:11 [PATCH 0/2] improve arche_platform_wd_irq() function Khadija Kamran 2023-03-30 14:11 ` [PATCH 1/2] staging: greybus: add a single exit path to arche_platform_wd_irq() Khadija Kamran 2023-04-03 0:37 ` Ira Weiny 2023-04-03 1:21 ` Khadija Kamran 2023-04-03 3:07 ` Alison Schofield 2023-04-03 3:15 ` Alison Schofield 2023-04-03 14:59 ` Ira Weiny 2023-03-30 14:11 ` [PATCH 2/2] staging: greybus: refactor arche_platform_wd_irq() Khadija Kamran 2023-03-30 15:23 ` Dan Carpenter 2023-04-01 17:41 ` Khadija Kamran 2023-04-03 4:29 ` Dan Carpenter 2023-04-03 15:23 ` Dan Carpenter 2023-04-27 6:27 ` Khadija Kamran 2023-05-03 22:16 ` Alison Schofield 2023-04-03 2:18 ` Ira Weiny 2023-03-30 16:58 ` kernel test robot 2023-04-03 0:36 ` [PATCH 0/2] improve arche_platform_wd_irq() function Ira Weiny 2023-04-03 1:18 ` Khadija Kamran
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).