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 <[email protected]>
Signed-off-by: David Mueller <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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.
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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
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
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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
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.
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
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
From: David Mueller <[email protected]>
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().
This is currently called from atomic context through
pci_config_{read,write).
For more detail please refer bugzilla.kernel.org, Bug
100031
Signed-off-by: David Mueller <[email protected]>
Signed-off-by: Sanjeev Sharma <[email protected]>
Changes in v2:
-order of signoff has been change
-Author of patch has been change
-A mdelay(1000) is different timescale than
a usleep(1000).change it to correct scale i.e mdelay(1)
-updated comment
---
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..c03527f 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(1);
}
/*
* From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
--
1.7.11.7