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: <20171106193021.GE31930@bhelgaas-glaptop.roam.corp.google.com>
Date:   Mon, 6 Nov 2017 13:30:21 -0600
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 20, 2017 at 01:19:17PM -0500, Bjorn Helgaas wrote:
> 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.

Dropping for lack of ack.