Message-ID: <CAKXUXMx+v_X2QGw4gqByWyGiyyJ+Cdecw_K+Wc=Edk4XrAM3zw@mail.gmail.com>
Date:   Mon, 13 Mar 2023 05:30:30 +0100
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Dan Carpenter <error27@...il.com>
Cc:     Khadija Kamran <kamrankhadijadj@...il.com>,
        outreachy@...ts.linux.dev,
        Vaibhav Hiremath <hvaibhav.linux@...il.com>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: greybus: fix exceeds line length

On Fri, Mar 10, 2023 at 8:40 PM Dan Carpenter <error27@...il.com> wrote:
>
> On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> > Length of line 182 exceeds 100 columns in file
> > drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> > line.
> >
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@...il.com>
> > ---
> >  drivers/staging/greybus/arche-platform.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index fcbd5f71eff2..0f0fbc263f8a 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -179,7 +179,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >                               if (arche_pdata->wake_detect_state !=
> >                                               WD_STATE_COLDBOOT_START) {
> >                                       arche_platform_set_wake_detect_state(arche_pdata,
> > -                                                                          WD_STATE_COLDBOOT_TRIG);
> > +                                             WD_STATE_COLDBOOT_TRIG);
>
> The original line was done deliberately so that it lines up.  If we
> apply your patch and re-run checkpatch -f on the file then it has a new
> warning:
>
> CHECK: Alignment should match open parenthesis
> #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> +                                       arche_platform_set_wake_detect_state(arche_pdata,
> +                                               WD_STATE_COLDBOOT_TRIG);
>
> Always try to think about the bigger picture.  Why did the original
> author do it that way?  The change makes checkpatch happy, but does it
> make the code more readable?  Is there a more important readability
> improvement to be done here?
>
> For example, you could re-arrange the if statements like this and pull
> everything in a few tabs.  Don't necessarily do that.  Just think about
> doing it.  I write quite a few cleanup patches that I don't send because
> the next day I just decide it's not worth it.
>
> When I look at this file, the style is not bad at all.  But at the start
> of the file there is #if IS_ENABLED(CONFIG_USB_HSIC_USB3613).  What is
> that?  The CONFIG doesn't exist and the header doesn't exits.  Probably
> it can be deleted.
>
> But that raises a new question.  Lukas Bulwahn is always looking for
> CONFIG_ entries which don't exist.  I would have expected him to find
> this already.
>

True, and my (private) random linux notes of the checkkconfigsymbols
effort on this config states:

USB_HSIC_USB3613: Reported and maintenance of dead code is fine for
staging maintainer

So, I did report it, and a quick search on lore.kernel.org
(https://lore.kernel.org/all/?q=USB_HSIC_USB3613) will give us some
more insight and refresh Greg's and my memory:

I reported the issue here:
https://lore.kernel.org/all/CAKXUXMym0M1UNuNGUVpFr2yUwOwjkZ_sQpCD0jC8YB+hs=j-bA@mail.gmail.com/

And Greg responded:

"... It's a good example driver for those wanting to create a greybus
host controller driver so it's nice to have in the tree..."

So, even though the code is dead, it is a nice example of some driver
code. So, we keep it.


> Anyway, we can write our own scripts to make a list of stuff inside
> IS_ENABLED():
>
> git grep IS_ENABLED | \
>         perl -ne 'if (/IS_ENABLED\((.+?)\)/){ print "$1\n"}' | \
>         sort -u | tee CONFIG_list
>
> Then we can go through the CONFIG_list file and see which other stuff
> doesn't exist.
>
> for i in $(grep ^CONFIG CONFIG_list  | cut -d '_' -f 2-) ; do \
>         grep -q -w "config $i$" $(find -name Kconfig) || echo $i ; \
> done | tee CONFIG_not_found
>
> I have never done this before so I don't know what you'll find.  But
> everywhere you look if you just look closer then it raises questions
> which raise more questions.  So it's interesting to explore.  Anyway,
> look closely at each line in the file and follow the rabbit holes until
> you find something interesting to work on.
>

@Dan, Thanks for pointing out my clean-up activity here!

Yes, I agree with Dan. That is certainly an interesting task to
explore. If you search the mailing list, you will find another bash
script of similar length that Joe Perches used a few years back. I
personally use the script ./scripts/checkkconfigsymbols.py. They may
differ a bit on what they report, but I fear at this point, most of
its reports are false positives. I have looked at all of them, I am
checking the new ones introduced, and sending out patches to clean up
stuff. There are a few on my todo list, like cleaning up the
definition of CONFIG_CAAM_QI. If you want to assist, please let me
know. I can certainly give you a few things that are at least worth a
deeper investigation and maybe also some clean up.

Lukas

> regards,
> dan carpenter
>
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..2d9e0c41b5e3 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -165,43 +165,39 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>                  * 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) {
> -                       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);
> -                                       spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -                                                              flags);
> -                                       return IRQ_WAKE_THREAD;
> -                               }
> -                       }
> -               }
> -       } else {
> -               /* wake/detect falling */
> -               if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> -                       arche_pdata->wake_detect_start = jiffies;
> +               if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)
> +                       goto out_unlock;
> +
> +               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 if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {
>                         /*
> -                        * 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.
> +                        * Check we are not in middle of irq thread
> +                        * already
>                          */
>                         arche_platform_set_wake_detect_state(arche_pdata,
> -                                                            WD_STATE_BOOT_INIT);
> +                                                            WD_STATE_COLDBOOT_TRIG);
> +                       spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> +                       return IRQ_WAKE_THREAD;
>                 }
> +       } else if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> +               /* wake/detect falling */
> +               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);
>         }
>
> +out_unlock:
>         spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>
>         return IRQ_HANDLED;

Powered by blists - more mailing lists