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]

Date:   Thu, 30 Dec 2021 12:13:02 -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 11:00 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Thu, Dec 30, 2021 at 10:24:10AM -0800, Dimitris Michailidis wrote:
> > 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.
>
> Yes.
>
>
> > > 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.
>
> I knew it would be hard to directly use iopoll, which is why i only
> said 'consider'. The problem is, this implementation has the same bug
> nearly everybody makes when writing their own implementation of what
> iopoll does, which is why i always point people at iopoll.
>
> msleep(100) guarantees that it will not return within 100ms. That is
> all. Consider what happens when msleep(100) actually sleeps for
> 1000.

Thanks, I see your point. Let me see which fix is easier.

>
>         Andrew