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: <Yc4Bxu8f9S5w3VsM@lunn.ch>
Date:   Thu, 30 Dec 2021 20:00:22 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Dimitris Michailidis <d.michailidis@...gible.com>
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 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.

	Andrew