linux-kernel - Re: [net-next v3 2/6] net: marvell: prestera: Add PCI interface 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]

Date:   Mon, 27 Jul 2020 11:04:56 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Andrew Lunn <andrew@...n.ch>,
        Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
        Serhiy Boiko <serhiy.boiko@...ision.eu>,
        Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
        Taras Chornyi <taras.chornyi@...ision.eu>,
        Andrii Savka <andrii.savka@...ision.eu>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mickey Rachamim <mickeyr@...vell.com>
Subject: Re: [net-next v3 2/6] net: marvell: prestera: Add PCI interface support

On Mon, Jul 27, 2020 at 1:55 AM Vadym Kochan <vadym.kochan@...ision.eu> wrote:
> On Sun, Jul 26, 2020 at 01:32:19PM +0300, Andy Shevchenko wrote:
> > On Sat, Jul 25, 2020 at 6:10 PM Vadym Kochan <vadym.kochan@...ision.eu> wrote:

...

For the non-commented I assume you are agree with. Correct?

...

> > > +config PRESTERA_PCI
> > > +       tristate "PCI interface driver for Marvell Prestera Switch ASICs family"
> > > +       depends on PCI && HAS_IOMEM && PRESTERA
> >
> > > +       default m
> >
> > Even if I have CONFIG_PRESTERA=y, why as a user I must have this as a module?
> > If it's a crucial feature, shouldn't it be rather
> >   default CONFIG_PRESTERA
> > ?
>
> The firmware image should be located on rootfs, and in case the rootfs
> should be mounted later the pci driver can't pick this up when
> statically compiled so I left it as 'm' by default.

We have for a long time to catch firmware blobs from initrd (initramfs).
default m is very unusual.

...

> > > +#define PRESTERA_FW_PATH \
> > > +       "mrvl/prestera/mvsw_prestera_fw-v" \
> > > +       __stringify(PRESTERA_SUPP_FW_MAJ_VER) \
> > > +       "." __stringify(PRESTERA_SUPP_FW_MIN_VER) ".img"
> >
> > Wouldn't it be better to see this in the C code?
>
> I have no strong opinion on this, but looks like macro is enough for
> this statically defined versioning.

The problem is that you have to bounce your editor to C code then to
macro then to another macro...
(in case you are looking for the code responsible for that)
In many drivers I saw either it's one static line (without those
__stringify(), etc) or done in C code dynamically near to
request_firmware() call.

Maybe you may replace __stringify by explicit characters / strings and
comment how the name was constructed?

#define FW_NAME "patch/to/it/fileX.Y.img"

...

> > > +static void prestera_pci_copy_to(u8 __iomem *dst, u8 *src, size_t len)
> > > +{
> > > +       u32 __iomem *dst32 = (u32 __iomem *)dst;
> > > +       u32 *src32 = (u32 *)src;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < (len / 4); dst32++, src32++, i++)
> > > +               writel_relaxed(*src32, dst32);
> > > +}
> > > +
> > > +static void prestera_pci_copy_from(u8 *dst, u8 __iomem *src, size_t len)
> > > +{
> > > +       u32 __iomem *src32 = (u32 __iomem *)src;
> > > +       u32 *dst32 = (u32 *)dst;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < (len / 4); dst32++, src32++, i++)
> > > +               *dst32 = readl_relaxed(src32);
> > > +}
> >
> > NIH of memcpy_fromio() / memcpy_toio() ?
> >
> I am not sure if there will be no issue with < 4 bytes transactions over
> PCI bus. I need to check it.

I didn't get it. You always do 4 byte chunks, so, supply aligned
length to memcpy and you will have the same.

...

> > > +static int prestera_fw_rev_check(struct prestera_fw *fw)
> > > +{
> > > +       struct prestera_fw_rev *rev = &fw->dev.fw_rev;
> > > +       u16 maj_supp = PRESTERA_SUPP_FW_MAJ_VER;
> > > +       u16 min_supp = PRESTERA_SUPP_FW_MIN_VER;
> > > +
> >
> > > +       if (rev->maj == maj_supp && rev->min >= min_supp)
> > > +               return 0;
> >
> > Why not traditional pattern
> >
> > if (err) {
> >  ...
> > }
>
> At least for me it looks simpler when to check which version is
> correct.

OK.

> > ...
> > return 0;
> >
> > ?
> >
> > > +       dev_err(fw->dev.dev, "Driver supports FW version only '%u.%u.x'",
> > > +               PRESTERA_SUPP_FW_MAJ_VER, PRESTERA_SUPP_FW_MIN_VER);
> > > +
> > > +       return -EINVAL;
> > > +}

...

> Thanks Andy for the comments, especially for pcim_ helpers.

You are welcome!

-- 
With Best Regards,
Andy Shevchenko