linux-kernel - Re: [PATCH] PCI: dwc: designware: don't sleep in atomic context

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: <20171020181917.GL6332@bhelgaas-glaptop.roam.corp.google.com>
Date:   Fri, 20 Oct 2017 13:19:17 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Pankaj Dubey <pankaj.dubey@...sung.com>
Cc:     David Laight <David.Laight@...LAB.COM>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Joao Pinto <Joao.Pinto@...opsys.com>
Subject: Re: [PATCH] PCI: dwc: designware: don't sleep in atomic context

On Fri, Oct 13, 2017 at 09:10:38AM +0530, Pankaj Dubey wrote:
> 
> 
> On 10/12/2017 04:09 PM, David Laight wrote:
> >From: Pankaj Dubey
> >>Sent: 12 October 2017 08:55
> >>In pcie-designware.c many places we are calling "usleep_range" which
> >>are in atomic context. This patch fixes these potential BUGs and
> >>replaces "usleep_range" with mdelay calls.
> >>
> >>Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
> >>---
> >>  drivers/pci/dwc/pcie-designware.c | 8 ++++----
> >>  drivers/pci/dwc/pcie-designware.h | 3 +--
> >>  2 files changed, 5 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
> >>index 88abddd..35d19b9 100644
> >>--- a/drivers/pci/dwc/pcie-designware.c
> >>+++ b/drivers/pci/dwc/pcie-designware.c
> >>@@ -138,7 +138,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> >>  		if (val & PCIE_ATU_ENABLE)
> >>  			return;
> >>
> >>-		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
> >>+		mdelay(LINK_WAIT_IATU_MIN);
> >>  	}
> >Spinning for 9ms (possibly 10 times) isn't really a good idea.
> 
> Yes. It may not be a good idea, however in our experiment it never
> hit maximum retry count.  I just converted usleep_range to mdelay
> keeping min time limitation as it is, though I am not sure, how do
> we arrived on these numbers in original code, may be Joao Pinto from
> Synopsys have some idea, I will try to do few experiment and try to
> find out what is sufficient minimum time in our hardware for these
> mdelay.

Just based on the preceding comment, it looks like the wait is
essential because subsequent config and I/O accesses won't work
correctly until the ATU enable takes effect.

If we timeout here, I suspect it's because something is seriously
wrong in the hardware, so I doubt there's any point in trying to
minimize the timeout period.  If something is that broken, it doesn't
matter whether we wait 9ms or 900ms.

Maybe the message should be more strident or maybe we should even
return failure so the caller can do something, e.g., fail an access,
instead of just printing an error and continuing on.

I'm also looking for an ack from Joao and/or Jingoo.

Bjorn