linux-kernel - Re: [PATCH] pci: hotplug: ibmphp: Fix 'line over 80 characters' Warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jun 2019 11:41:03 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     benniciemanuel78@...il.com
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Joe Perches <joe@...ches.com>, Lukas Wunner <lukas@...ner.de>,
        Sebastian Ott <sebott@...ux.ibm.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] pci: hotplug: ibmphp: Fix 'line over 80 characters' Warning

On Sun, Jun 2, 2019 at 7:25 PM Emanuel Bennici
<benniciemanuel78@...il.com> wrote:
>
> Fix checkpatch.pl 'line over 80 characters' Warning in ibmphp_ebda.c and
> ibmphp_hpc.c

I'm not sure it's needed.
And actually some of the changes are the regressions in order of
readability ratio.

> +                               debug("dev->device = %x, "
> +                                     "dev->subsystem_device = %x\n",

Don't split string literals like this.

> -       if ((pslot == NULL)
> -           || ((pstatus == NULL) && (cmd != READ_ALLSTAT) && (cmd != READ_BUSSTATUS))) {
> +       if ((pslot == NULL) || ((pstatus == NULL) && (cmd != READ_ALLSTAT) &&
> +                               (cmd != READ_BUSSTATUS))) {

No, no, this breaks readability a lot.
In the former it's clearly shown the logic between conditionals, while
in the latter it's a mess.

> +                                       err("%s - Error ctrl_read failed\n",
> +                                               __func__);

Really, no advantage on this line even for 80 characters.

> +                       err("%s - Exit Error:invalid bus, rc[%d]\n",
> +                               __func__, rc);

Same here.

> -               debug("%s - ctlr id[%x] physical[%lx] logical[%lx] i2c[%x]\n", __func__,
> -               ctlr_ptr->ctlr_id, (ulong) (ctlr_ptr->u.wpeg_ctlr.wpegbbar), (ulong) wpg_bbar,
> -               ctlr_ptr->u.wpeg_ctlr.i2c_addr);
> +               debug("%s - ctlr id[%x] physical[%lx] logical[%lx] i2c[%x]\n",
> +                       __func__, ctlr_ptr->ctlr_id,
> +                       (ulong) (ctlr_ptr->u.wpeg_ctlr.wpegbbar),
> +                       (ulong) wpg_bbar, ctlr_ptr->u.wpeg_ctlr.i2c_addr);

Here better to fix explicit casts (perhaps it means wrong specifiers
being used).

> +               if ((poldslot->status & 0x20) &&
> +                   (SLOT_CONNECT(poldslot->status) == HPC_SLOT_CONNECTED)
> +                   && (SLOT_PRESENT(poldslot->status)))

Here you are not consistent in a way where to put logical operation.

> +                               memcpy((void *) &myslot, (void *) pslot,
> +                                      sizeof(struct slot));

Why spaces after ) here?

> +                               debug("%s - call process_changeinstatus for"
> +                                     "slot[%d]\n", __func__, i);

Do not split string literals like this.

-- 
With Best Regards,
Andy Shevchenko