netdev - Re: [PATCH net-next 2/8] net/fungible: Add service module for Fungible drivers

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: <CAOkoqZnoOgGDGcnDeOQxjZ_eYh8eyFHK_E+w7E6QHWAvaembKw@mail.gmail.com>
Date:   Thu, 30 Dec 2021 10:24:10 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/8] net/fungible: Add service module for
 Fungible drivers

On Thu, Dec 30, 2021 at 9:28 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +/* Wait for the CSTS.RDY bit to match @enabled. */
> > +static int fun_wait_ready(struct fun_dev *fdev, bool enabled)
> > +{
> > +     unsigned int cap_to = NVME_CAP_TIMEOUT(fdev->cap_reg);
> > +     unsigned long timeout = ((cap_to + 1) * HZ / 2) + jiffies;
> > +     u32 bit = enabled ? NVME_CSTS_RDY : 0;
>
> Reverse Christmas tree, since this is a network driver.

The longer line in the middle depends on the previous line, I'd need to
remove the initializers to sort these by length.

> Please also consider using include/linux/iopoll.h. The signal handling
> might make that not possible, but signal handling in driver code is in
> itself very unusual.

This initialization is based on NVMe, hence the use of NVMe registers,
and this function is based on nvme_wait_ready(). The check sequence
including signal handling comes from there.

iopoll is possible with the signal check removed, though I see I'd need a
shorter delay than the 100ms used here and it doesn't check for reads of
all 1s, which happen occasionally. My preference though would be to keep
this close to the NVMe version. Let me know.

> > +
> > +     do {
> > +             u32 csts = readl(fdev->bar + NVME_REG_CSTS);
> > +
> > +             if (csts == ~0) {
> > +                     dev_err(fdev->dev, "CSTS register read %#x\n", csts);
> > +                     return -EIO;
> > +             }
> > +
> > +             if ((csts & NVME_CSTS_RDY) == bit)
> > +                     return 0;
> > +
> > +             msleep(100);
> > +             if (fatal_signal_pending(current))
> > +                     return -EINTR;
> > +     } while (time_is_after_eq_jiffies(timeout));
> > +
> > +     dev_err(fdev->dev,
> > +             "Timed out waiting for device to indicate RDY %u; aborting %s\n",
> > +             enabled, enabled ? "initialization" : "reset");
> > +     return -ETIMEDOUT;
> > +}
>
>   Andrew