netdev - Re: [PATCH v2 net-next 3/7] octeontx2-vf: Virtual function driver support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20200314191258.GB3046@unreal>
Date:   Sat, 14 Mar 2020 21:12:58 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Sunil Kovvuri <sunil.kovvuri@...il.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tomasz Duszynski <tduszynski@...vell.com>,
        Subbaraya Sundeep <sbhatta@...vell.com>,
        Geetha sowjanya <gakula@...vell.com>,
        Sunil Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH v2 net-next 3/7] octeontx2-vf: Virtual function driver
 support

On Sat, Mar 14, 2020 at 09:10:28PM +0530, Sunil Kovvuri wrote:
> On Fri, Mar 13, 2020 at 11:41 PM Leon Romanovsky <leon@...nel.org> wrote:
>  > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > > new file mode 100644
> > > index 0000000..cf366dc
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > > @@ -0,0 +1,659 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Marvell OcteonTx2 RVU Virtual Function ethernet driver
> > > + *
> > > + * Copyright (C) 2020 Marvell International Ltd.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> >
> > Please don't add license text, the SPDX line is enough.
> >
>
> Can you please point me to where this is written.

The whole idea of SPDX tags is to provide clear and proper license for the code.
See what it means to place SPDX tags and there can be found LICENSE text.
https://elixir.bootlin.com/linux/latest/source/Documentation/process/license-rules.rst
https://elixir.bootlin.com/linux/latest/source/Documentation/process/howto.rst#L59


> It would be great if these are made rules and written somewhere so
> that everyone can go through and follow.
> I see that there are so many patches being submitted with copyright text.
> So this is very confusing.

It is a mistake to place LICENSE text in addition to SPDX in new files.

>
> > > +
> > > +static int otx2vf_process_mbox_msg_up(struct otx2_nic *vf,
> > > +                                   struct mbox_msghdr *req)
> > > +{
> > > +     /* Check if valid, if not reply with a invalid msg */
> > > +     if (req->sig != OTX2_MBOX_REQ_SIG) {
> > > +             otx2_reply_invalid_msg(&vf->mbox.mbox_up, 0, 0, req->id);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     switch (req->id) {
> > > +#define M(_name, _id, _fn_name, _req_type, _rsp_type)                        \
> > > +     case _id: {                                                     \
> > > +             struct _rsp_type *rsp;                                  \
> > > +             int err;                                                \
> > > +                                                                     \
> > > +             rsp = (struct _rsp_type *)otx2_mbox_alloc_msg(          \
> > > +                     &vf->mbox.mbox_up, 0,                           \
> > > +                     sizeof(struct _rsp_type));                      \
> > > +             if (!rsp)                                               \
> > > +                     return -ENOMEM;                                 \
> > > +                                                                     \
> > > +             rsp->hdr.id = _id;                                      \
> > > +             rsp->hdr.sig = OTX2_MBOX_RSP_SIG;                       \
> > > +             rsp->hdr.pcifunc = 0;                                   \
> > > +             rsp->hdr.rc = 0;                                        \
> > > +                                                                     \
> > > +             err = otx2_mbox_up_handler_ ## _fn_name(                \
> > > +                     vf, (struct _req_type *)req, rsp);              \
> > > +             return err;                                             \
> > > +     }
> > > +MBOX_UP_CGX_MESSAGES
> > > +#undef M
> >
> > "return ..." inside macro which is called by another macro is highly
> > discouraged by the Linux kernel coding style.
> >
>
> There are many mailbox messages to handle and adding each one of them
> to switch case would be a
> lot of duplicate code. Hence we choose to with these macros.

The coding style section 12.1 talks exactly about that pattern.
https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst#L752
Somehow all other drivers succeeded to write their code without such
macros, I'm confident that you will success too. Please try your best.

Thanks

>
> Thanks,
> Sunil.