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@xxxxxxxxx>
> > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@xxxxxxxxx>
> > > > > ---
> > > > > 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
>
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|