linux-kernel - Re: [PATCH] drm/nouveau/fifo:Fix Nineteen occurrences of the gk104.c error: ERROR: : trailing statements should be on next line

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] [day] [month] [year] [list]

Message-ID: <CACO55tu08XTx=TZQK=RyCGRdFVDQN9m+X+y3F3V0H7nuwyycRw@mail.gmail.com>
Date:   Sat, 15 Jul 2023 11:23:01 +0200
From:   Karol Herbst <kherbst@...hat.com>
To:     Lyude Paul <lyude@...hat.com>
Cc:     huzhi001@...suo.com, bskeggs@...hat.com, airlied@...il.com,
        daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        nouveau@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/nouveau/fifo:Fix Nineteen occurrences of the gk104.c
 error: ERROR: : trailing statements should be on next line

On Sat, Jul 15, 2023 at 1:07 AM Lyude Paul <lyude@...hat.com> wrote:
>
> NAK - checkpatch.pl is a (strongish) guideline, but not a rule. In the cases
> corrected in the patch series here, we format the switch cases on single lines
> as it dramatically improves the readability of what is otherwise just a /long/
> list of slightly different static mappings. I don't believe we're the only
> part of the kernel to do this either.
>

I wished there was a place to document something like "patches whose
only reason is 'checkpatch.pl' said so" will be rejected.

I think following some checkpatch rules are sane, but then the
argument should be "makes code more clear" or "converts risky coding
practises to less risky ones". Or do we have such a place? Maybe we
should patch checkpatch.pl instead to throw a warning

> On Fri, 2023-07-14 at 14:58 +0800, huzhi001@...suo.com wrote:
> > Signed-off-by: ZhiHu <huzhi001@...suo.com>
> > ---
> >   .../gpu/drm/nouveau/nvkm/engine/fifo/gk104.c  | 40 ++++++++++++++-----
> >   1 file changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > index d8a4d773a58c..b99e0a7c96bb 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > @@ -137,15 +137,29 @@ gk104_ectx_bind(struct nvkm_engn *engn, struct
> > nvkm_cctx *cctx, struct nvkm_chan
> >       u64 addr = 0ULL;
> >
> >       switch (engn->engine->subdev.type) {
> > -    case NVKM_ENGINE_SW    : return;
> > -    case NVKM_ENGINE_GR    : ptr0 = 0x0210; break;
> > -    case NVKM_ENGINE_SEC   : ptr0 = 0x0220; break;
> > -    case NVKM_ENGINE_MSPDEC: ptr0 = 0x0250; break;
> > -    case NVKM_ENGINE_MSPPP : ptr0 = 0x0260; break;
> > -    case NVKM_ENGINE_MSVLD : ptr0 = 0x0270; break;
> > -    case NVKM_ENGINE_VIC   : ptr0 = 0x0280; break;
> > -    case NVKM_ENGINE_MSENC : ptr0 = 0x0290; break;
> > -    case NVKM_ENGINE_NVDEC :
> > +    case NVKM_ENGINE_SW:
> > +        return;
> > +    case NVKM_ENGINE_GR:
> > +        ptr0 = 0x0210;
> > +        break;
> > +    case NVKM_ENGINE_SEC:
> > +        ptr0 = 0x0220;
> > +        break;
> > +    case NVKM_ENGINE_MSPDEC:
> > +        ptr0 = 0x0250;
> > +        break;
> > +    case NVKM_ENGINE_MSPPP:
> > +        ptr0 = 0x0260;
> > +        break;
> > +    case NVKM_ENGINE_MSVLD:
> > +        ptr0 = 0x0270;
> > +        break;
> > +    case NVKM_ENGINE_VIC:
> > +        ptr0 = 0x0280; break;
> > +    case NVKM_ENGINE_MSENC:
> > +        ptr0 = 0x0290;
> > +        break;
> > +    case NVKM_ENGINE_NVDEC:
> >           ptr1 = 0x0270;
> >           ptr0 = 0x0210;
> >           break;
> > @@ -435,8 +449,12 @@ gk104_runl_commit(struct nvkm_runl *runl, struct
> > nvkm_memory *memory, u32 start,
> >       int target;
> >
> >       switch (nvkm_memory_target(memory)) {
> > -    case NVKM_MEM_TARGET_VRAM: target = 0; break;
> > -    case NVKM_MEM_TARGET_NCOH: target = 3; break;
> > +    case NVKM_MEM_TARGET_VRAM:
> > +        target = 0;
> > +        break;
> > +    case NVKM_MEM_TARGET_NCOH:
> > +        target = 3;
> > +        break;
>
> This one isn't very long, but I'd still say it's definitely a lot easier to
> read in the compact form. If anything, the only change I would make here is
> formatting the default: case to be on a single line as well
>
> >       default:
> >           WARN_ON(1);
> >           return;
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
>