| Message ID | 1447066080-5859-1-git-send-email-sanjeev_sharma@mentor.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Am Montag, den 09.11.2015, 16:18 +0530 schrieb Sanjeev Sharma: > If additional PCIe switch get connected between the > host and the NIC,the kernel crashes with "BUG: > scheduling while atomic". To handle this we need to > call mdelay() instead of usleep_range(). > > For more detail please refer bugzilla.kernel.org, Bug > 100031 > > Signed-off-by: Sanjeev Sharma <sanjeev_sharma@mentor.com> This is wrong. You are not the author of this patch and this should be reflected both in the From: line as well as in the order of signoffs. > Signed-off-by: David Mueller <dave.mueller@gmx.ch> > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 233a196..9769b13 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(1000); A mdelay(1000) is a whole different timescale than a usleep(1000). If this patch works for you with mdelay(1) or maybe mdelay(2) I would be fine with it. Regards, Lucas > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
On Tuesday 10 November 2015 09:41:18 Lucas Stach wrote: > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > index 233a196..9769b13 100644 > > --- a/drivers/pci/host/pci-imx6.c > > +++ b/drivers/pci/host/pci-imx6.c > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > * Wait a little bit, then re-check if the link finished > > * the training. > > */ > > - usleep_range(1000, 2000); > > + mdelay(1000); > > A mdelay(1000) is a whole different timescale than a usleep(1000). If > this patch works for you with mdelay(1) or maybe mdelay(2) I would be > fine with it. mdelay(1) is still a really long time to block the CPU for, on potentially every config space access. Everybody else just returns the link status here, which seems to be the better alternative. If you need to delay the startup, better have a msleep(1) loop in the initial probe function where you are allowed to sleep. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, den 10.11.2015, 10:28 +0100 schrieb Arnd Bergmann: > On Tuesday 10 November 2015 09:41:18 Lucas Stach wrote: > > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > > index 233a196..9769b13 100644 > > > --- a/drivers/pci/host/pci-imx6.c > > > +++ b/drivers/pci/host/pci-imx6.c > > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > > * Wait a little bit, then re-check if the link finished > > > * the training. > > > */ > > > - usleep_range(1000, 2000); > > > + mdelay(1000); > > > > A mdelay(1000) is a whole different timescale than a usleep(1000). If > > this patch works for you with mdelay(1) or maybe mdelay(2) I would be > > fine with it. > > mdelay(1) is still a really long time to block the CPU for, on potentially > every config space access. > > Everybody else just returns the link status here, which seems to be > the better alternative. If you need to delay the startup, better have > a msleep(1) loop in the initial probe function where you are allowed to > sleep. > Yes, it's somewhere on my TODO list to rework the link-up handling here, but as there are quite a few timing and ordering implications in that code, this needs a good thought and a good deal of testing. So I'm inclined to ACK the current patch to get rid of the obvious bug and sort things out properly in a follow on patchset. Regards, Lucas
On Tuesday 10 November 2015 10:35:10 Lucas Stach wrote: > Am Dienstag, den 10.11.2015, 10:28 +0100 schrieb Arnd Bergmann: > > On Tuesday 10 November 2015 09:41:18 Lucas Stach wrote: > > > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > > > index 233a196..9769b13 100644 > > > > --- a/drivers/pci/host/pci-imx6.c > > > > +++ b/drivers/pci/host/pci-imx6.c > > > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > > > * Wait a little bit, then re-check if the link finished > > > > * the training. > > > > */ > > > > - usleep_range(1000, 2000); > > > > + mdelay(1000); > > > > > > A mdelay(1000) is a whole different timescale than a usleep(1000). If > > > this patch works for you with mdelay(1) or maybe mdelay(2) I would be > > > fine with it. > > > > mdelay(1) is still a really long time to block the CPU for, on potentially > > every config space access. > > > > Everybody else just returns the link status here, which seems to be > > the better alternative. If you need to delay the startup, better have > > a msleep(1) loop in the initial probe function where you are allowed to > > sleep. > > > Yes, it's somewhere on my TODO list to rework the link-up handling here, > but as there are quite a few timing and ordering implications in that > code, this needs a good thought and a good deal of testing. So I'm > inclined to ACK the current patch to get rid of the obvious bug and sort > things out properly in a follow on patchset. Maybe use that patch with some modifications then: * add a comment to explain that this is currently called from possibly atomic context through pci_config_{read,write} and that the link state handling never belonged here. * instead of looping five times for up to 2ms each, loop 100 times around a udelay(20) to hopefully be done earlier. I was going to suggest using time_before(timeout, jiffies) as the condition to wait for, but that doesn't work if called with interrupts disabled. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 November 2015 10:35:10 Lucas Stach wrote: > Am Dienstag, den 10.11.2015, 10:28 +0100 schrieb Arnd Bergmann: > > On Tuesday 10 November 2015 09:41:18 Lucas Stach wrote: > > > > diff --git a/drivers/pci/host/pci-imx6.c > > > > b/drivers/pci/host/pci-imx6.c index 233a196..9769b13 100644 > > > > --- a/drivers/pci/host/pci-imx6.c > > > > +++ b/drivers/pci/host/pci-imx6.c > > > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > > > * Wait a little bit, then re-check if the link finished > > > > * the training. > > > > */ > > > > - usleep_range(1000, 2000); > > > > + mdelay(1000); > > > > > > A mdelay(1000) is a whole different timescale than a usleep(1000). > > > If this patch works for you with mdelay(1) or maybe mdelay(2) I > > > would be fine with it. > > > > mdelay(1) is still a really long time to block the CPU for, on > > potentially every config space access. > > > > Everybody else just returns the link status here, which seems to be > > the better alternative. If you need to delay the startup, better > > have a msleep(1) loop in the initial probe function where you are > > allowed to sleep. > > > Yes, it's somewhere on my TODO list to rework the link-up handling > here, but as there are quite a few timing and ordering implications in > that code, this needs a good thought and a good deal of testing. So > I'm inclined to ACK the current patch to get rid of the obvious bug > and sort things out properly in a follow on patchset. Maybe use that patch with some modifications then: * add a comment to explain that this is currently called from possibly atomic context through pci_config_{read,write} and that the link state handling never belonged here. * instead of looping five times for up to 2ms each, loop 100 times around a udelay(20) to hopefully be done earlier. I was going to suggest using time_before(timeout, jiffies) as the condition to wait for, but that doesn't work if called with interrupts disabled. Arnd Shall I go ahead by changing only current patch to mdelay(1). I will also Incorporate comment #1 given by Arnd above. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 16.11.2015, 09:36 +0000 schrieb Sharma, Sanjeev: > On Tuesday 10 November 2015 10:35:10 Lucas Stach wrote: > > Am Dienstag, den 10.11.2015, 10:28 +0100 schrieb Arnd Bergmann: > > > On Tuesday 10 November 2015 09:41:18 Lucas Stach wrote: > > > > > diff --git a/drivers/pci/host/pci-imx6.c > > > > > b/drivers/pci/host/pci-imx6.c index 233a196..9769b13 100644 > > > > > --- a/drivers/pci/host/pci-imx6.c > > > > > +++ b/drivers/pci/host/pci-imx6.c > > > > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > > > > * Wait a little bit, then re-check if the link finished > > > > > * the training. > > > > > */ > > > > > - usleep_range(1000, 2000); > > > > > + mdelay(1000); > > > > > > > > A mdelay(1000) is a whole different timescale than a usleep(1000). > > > > If this patch works for you with mdelay(1) or maybe mdelay(2) I > > > > would be fine with it. > > > > > > mdelay(1) is still a really long time to block the CPU for, on > > > potentially every config space access. > > > > > > Everybody else just returns the link status here, which seems to be > > > the better alternative. If you need to delay the startup, better > > > have a msleep(1) loop in the initial probe function where you are > > > allowed to sleep. > > > > > Yes, it's somewhere on my TODO list to rework the link-up handling > > here, but as there are quite a few timing and ordering implications in > > that code, this needs a good thought and a good deal of testing. So > > I'm inclined to ACK the current patch to get rid of the obvious bug > > and sort things out properly in a follow on patchset. > > Maybe use that patch with some modifications then: > > * add a comment to explain that this is currently called from possibly > atomic context through pci_config_{read,write} and that the link > state handling never belonged here. > > * instead of looping five times for up to 2ms each, loop 100 times > around a udelay(20) to hopefully be done earlier. I was going to > suggest using time_before(timeout, jiffies) as the condition to > wait for, but that doesn't work if called with interrupts disabled. > > Arnd > Shall I go ahead by changing only current patch to mdelay(1). I will also > Incorporate comment #1 given by Arnd above. Yes, please go ahead. Smaller delay loops make sense, but please ensure that the total timeouts stay the same. Regards, Lucas
Hi Sanjeev, On Mon, Nov 09, 2015 at 04:18:00PM +0530, Sanjeev Sharma wrote: > If additional PCIe switch get connected between the > host and the NIC,the kernel crashes with "BUG: > scheduling while atomic". To handle this we need to > call mdelay() instead of usleep_range(). > > For more detail please refer bugzilla.kernel.org, Bug > 100031 > > Signed-off-by: Sanjeev Sharma <sanjeev_sharma@mentor.com> > Signed-off-by: David Mueller <dave.mueller@gmx.ch> I'm dropping this for now because we've been kicking around the same solution (with tweaks to the mdelay amount) since June, but no progress on the *real* issue, which is that imx6_pcie_link_up() should never wait; it should simply return the link status. I'm pretty sure the amount of time I've spent looking into this would have been enough to make some progress on that underlying issue. Bjorn > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 233a196..9769b13 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(1000); > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. > -- > 1.7.11.7 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message----- From: Bjorn Helgaas [mailto:helgaas@kernel.org] Sent: Thursday, January 07, 2016 3:35 AM To: Sharma, Sanjeev Cc: Richard.Zhu@freescale.com; l.stach@pengutronix.de; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; David Mueller Subject: Re: [PATCH] PCI: imx6:don't sleep in atomic context Hi Sanjeev, On Mon, Nov 09, 2015 at 04:18:00PM +0530, Sanjeev Sharma wrote: > If additional PCIe switch get connected between the host and the > NIC,the kernel crashes with "BUG: > scheduling while atomic". To handle this we need to call mdelay() > instead of usleep_range(). > > For more detail please refer bugzilla.kernel.org, Bug > 100031 > > Signed-off-by: Sanjeev Sharma <sanjeev_sharma@mentor.com> > Signed-off-by: David Mueller <dave.mueller@gmx.ch> I'm dropping this for now because we've been kicking around the same solution (with tweaks to the mdelay amount) since June, but no progress on the *real* issue, which is that imx6_pcie_link_up() should never wait; it should simply return the link status. I'm pretty sure the amount of time I've spent looking into this would have been enough to make some progress on that underlying issue. Bjorn Ok ! please share the change you are planning to implement. Sanjeev Sharma > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 233a196..9769b13 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(1000); > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. > -- > 1.7.11.7 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in the body of a message to majordomo@vger.kernel.org More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 18, 2016 at 07:17:41AM +0000, Sharma, Sanjeev wrote: > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Thursday, January 07, 2016 3:35 AM > To: Sharma, Sanjeev > Cc: Richard.Zhu@freescale.com; l.stach@pengutronix.de; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; David Mueller > Subject: Re: [PATCH] PCI: imx6:don't sleep in atomic context > > Hi Sanjeev, > > On Mon, Nov 09, 2015 at 04:18:00PM +0530, Sanjeev Sharma wrote: > > If additional PCIe switch get connected between the host and the > > NIC,the kernel crashes with "BUG: > > scheduling while atomic". To handle this we need to call mdelay() > > instead of usleep_range(). > > > > For more detail please refer bugzilla.kernel.org, Bug > > 100031 > > > > Signed-off-by: Sanjeev Sharma <sanjeev_sharma@mentor.com> > > Signed-off-by: David Mueller <dave.mueller@gmx.ch> > > I'm dropping this for now because we've been kicking around the same solution (with tweaks to the mdelay amount) since June, but no progress on the *real* issue, which is that imx6_pcie_link_up() should never wait; it should simply return the link status. > > I'm pretty sure the amount of time I've spent looking into this would have been enough to make some progress on that underlying issue. > > Bjorn > > Ok ! please share the change you are planning to implement. I didn't mean I personally was going to do this. But I think Lucas stepped up and did it: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-imx6&id=4d107d3b5a686b5834e533a00b73bf7b1cf59df7 > > --- > > drivers/pci/host/pci-imx6.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > index 233a196..9769b13 100644 > > --- a/drivers/pci/host/pci-imx6.c > > +++ b/drivers/pci/host/pci-imx6.c > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > * Wait a little bit, then re-check if the link finished > > * the training. > > */ > > - usleep_range(1000, 2000); > > + mdelay(1000); > > } > > /* > > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. > > -- > > 1.7.11.7 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > > in the body of a message to majordomo@vger.kernel.org More majordomo > > info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message----- From: Bjorn Helgaas [mailto:helgaas@kernel.org] Sent: Thursday, February 18, 2016 8:39 PM To: Sharma, Sanjeev Cc: Richard.Zhu@freescale.com; l.stach@pengutronix.de; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; David Mueller Subject: Re: [PATCH] PCI: imx6:don't sleep in atomic context On Thu, Feb 18, 2016 at 07:17:41AM +0000, Sharma, Sanjeev wrote: > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Thursday, January 07, 2016 3:35 AM > To: Sharma, Sanjeev > Cc: Richard.Zhu@freescale.com; l.stach@pengutronix.de; > bhelgaas@google.com; linux-pci@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > David Mueller > Subject: Re: [PATCH] PCI: imx6:don't sleep in atomic context > > Hi Sanjeev, > > On Mon, Nov 09, 2015 at 04:18:00PM +0530, Sanjeev Sharma wrote: > > If additional PCIe switch get connected between the host and the > > NIC,the kernel crashes with "BUG: > > scheduling while atomic". To handle this we need to call mdelay() > > instead of usleep_range(). > > > > For more detail please refer bugzilla.kernel.org, Bug > > 100031 > > > > Signed-off-by: Sanjeev Sharma <sanjeev_sharma@mentor.com> > > Signed-off-by: David Mueller <dave.mueller@gmx.ch> > > I'm dropping this for now because we've been kicking around the same solution (with tweaks to the mdelay amount) since June, but no progress on the *real* issue, which is that imx6_pcie_link_up() should never wait; it should simply return the link status. > > I'm pretty sure the amount of time I've spent looking into this would have been enough to make some progress on that underlying issue. > > Bjorn > > Ok ! please share the change you are planning to implement. I didn't mean I personally was going to do this. But I think Lucas stepped up and did it: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-imx6&id=4d107d3b5a686b5834e533a00b73bf7b1cf59df7 Thanks Bjorn code look ok ! > > --- > > drivers/pci/host/pci-imx6.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/host/pci-imx6.c > > b/drivers/pci/host/pci-imx6.c index 233a196..9769b13 100644 > > --- a/drivers/pci/host/pci-imx6.c > > +++ b/drivers/pci/host/pci-imx6.c > > @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > > * Wait a little bit, then re-check if the link finished > > * the training. > > */ > > - usleep_range(1000, 2000); > > + mdelay(1000); > > } > > /* > > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. > > -- > > 1.7.11.7 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > > in the body of a message to majordomo@vger.kernel.org More majordomo > > info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in the body of a message to majordomo@vger.kernel.org More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index 233a196..9769b13 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -499,7 +499,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) * Wait a little bit, then re-check if the link finished * the training. */ - usleep_range(1000, 2000); + mdelay(1000); } /* * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.