| Message ID | 20150722020306.GA8379@earth (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 22, 2015 at 04:03:07AM +0200, Sebastian Reichel wrote: > Hi, > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > This code has my head spinning. > > > > I found that the errors do not occur when the driver is built into the kernel. > > > > I also found that with the patch below the errors go away. > > > > Not sure if it is acceptible but see if it fixes things on your side. > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index cf8b91b..3164d13 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > if (!spi->max_speed_hz) > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > - spi_set_cs(spi, false); > > - > > if (spi->master->setup) > > status = spi->master->setup(spi); > > > > + spi_set_cs(spi, false); > > + > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n", > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > mh. maybe a runtime PM issue? mh? > > * external abort on non-linefetch: address cannot be accessed, > since the module's clocks are disabled > * built-in works, module not: built-in is probably a little bit > faster (module must not be loaded from filesystem), so that > the device has not yet been suspended > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > routine, which acquired runtime PM > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > prior pm_runtime_get_sync() > * With your workaround, the device has not yet returned to > suspend after the runtime PM acquisition in setup() > > So I suggest trying the following (compile tested only) patch: > It seems you are right. With this patch the SPI drivers no longer cause data aborts. I will wait for feedback from Pavel and Pali but it looks like we have a winner. > -- Sebastian > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > index 5867384..f7d9ffd 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > { > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > u32 l; > > /* The controller handles the inverted chip selects > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > enable = !enable; > > if (spi->controller_state) { > + pm_runtime_get_sync(mcspi->dev); > + Should the return code should be checked here as with the other instances of pm_runtime_get_sync? > l = mcspi_cached_chconf0(spi); > > if (enable) > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > l |= OMAP2_MCSPI_CHCONF_FORCE; > > mcspi_write_chconf0(spi, l); > + > + pm_runtime_mark_last_busy(mcspi->dev); > + pm_runtime_put_autosuspend(mcspi->dev); > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 22 July 2015 00:13:34 Michael Welling wrote: > On Wed, Jul 22, 2015 at 04:03:07AM +0200, Sebastian Reichel wrote: > > Hi, > > > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > > > This code has my head spinning. > > > > > > I found that the errors do not occur when the driver is built into the kernel. > > > > > > I also found that with the patch below the errors go away. > > > > > > Not sure if it is acceptible but see if it fixes things on your side. > > > > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index cf8b91b..3164d13 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > > if (!spi->max_speed_hz) > > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > > > - spi_set_cs(spi, false); > > > - > > > if (spi->master->setup) > > > status = spi->master->setup(spi); > > > > > > + spi_set_cs(spi, false); > > > + > > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n", > > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > > > mh. maybe a runtime PM issue? > > mh? > > > > > * external abort on non-linefetch: address cannot be accessed, > > since the module's clocks are disabled > > * built-in works, module not: built-in is probably a little bit > > faster (module must not be loaded from filesystem), so that > > the device has not yet been suspended > > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > > routine, which acquired runtime PM > > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > > prior pm_runtime_get_sync() > > * With your workaround, the device has not yet returned to > > suspend after the runtime PM acquisition in setup() > > > > So I suggest trying the following (compile tested only) patch: > > > > It seems you are right. > > With this patch the SPI drivers no longer cause data aborts. > > I will wait for feedback from Pavel and Pali but it looks like > we have a winner. > I'm busy right now for testing something on my N900... > > -- Sebastian > > > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > > index 5867384..f7d9ffd 100644 > > --- a/drivers/spi/spi-omap2-mcspi.c > > +++ b/drivers/spi/spi-omap2-mcspi.c > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > { > > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > > u32 l; > > > > /* The controller handles the inverted chip selects > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > enable = !enable; > > > > if (spi->controller_state) { > > + pm_runtime_get_sync(mcspi->dev); > > + > > Should the return code should be checked here as with the other > instances of pm_runtime_get_sync? > Yes, you *must* check return value of pm_runtime_get_sync! Otherwise we can get another oops/kernel panic. See similar commits: f7b2b5dd6a62a47e871627c71ed01aa3482154d9 604c31039dae4653f33003d08c91ef58b70b5e63 Anyway, what do you think about adding gcc macro for pm_runtime_get_sync function which report compiler warning when return value is not checked? > > l = mcspi_cached_chconf0(spi); > > > > if (enable) > > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > l |= OMAP2_MCSPI_CHCONF_FORCE; > > > > mcspi_write_chconf0(spi, l); > > + > > + pm_runtime_mark_last_busy(mcspi->dev); > > + pm_runtime_put_autosuspend(mcspi->dev); > > } > > } > > > >
On Wed 2015-07-22 04:03:07, Sebastian Reichel wrote: > Hi, > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > This code has my head spinning. > > > > I found that the errors do not occur when the driver is built into the kernel. > > > > I also found that with the patch below the errors go away. > > > > Not sure if it is acceptible but see if it fixes things on your side. > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index cf8b91b..3164d13 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > if (!spi->max_speed_hz) > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > - spi_set_cs(spi, false); > > - > > if (spi->master->setup) > > status = spi->master->setup(spi); > > > > + spi_set_cs(spi, false); > > + > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n", > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > mh. maybe a runtime PM issue? > > * external abort on non-linefetch: address cannot be accessed, > since the module's clocks are disabled > * built-in works, module not: built-in is probably a little bit > faster (module must not be loaded from filesystem), so that > the device has not yet been suspended > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > routine, which acquired runtime PM > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > prior pm_runtime_get_sync() > * With your workaround, the device has not yet returned to > suspend after the runtime PM acquisition in setup() > > So I suggest trying the following (compile tested only) patch: Solves problem for me. I had to apply it by patch -l. Tested-by: Pavel Machek <pavel@ucw.cz> Pavel > -- Sebastian > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > index 5867384..f7d9ffd 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > { > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > u32 l; > > /* The controller handles the inverted chip selects > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > enable = !enable; > > if (spi->controller_state) { > + pm_runtime_get_sync(mcspi->dev); > + > l = mcspi_cached_chconf0(spi); > > if (enable) > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > l |= OMAP2_MCSPI_CHCONF_FORCE; > > mcspi_write_chconf0(spi, l); > + > + pm_runtime_mark_last_busy(mcspi->dev); > + pm_runtime_put_autosuspend(mcspi->dev); > } > } >
Hi, On Wed, Jul 22, 2015 at 09:27:32AM +0200, Pali Rohár wrote: > > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > > > index 5867384..f7d9ffd 100644 > > > --- a/drivers/spi/spi-omap2-mcspi.c > > > +++ b/drivers/spi/spi-omap2-mcspi.c > > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > > > > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > > { > > > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > > > u32 l; > > > > > > /* The controller handles the inverted chip selects > > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > > enable = !enable; > > > > > > if (spi->controller_state) { > > > + pm_runtime_get_sync(mcspi->dev); > > > + > > > > Should the return code should be checked here as with the other > > instances of pm_runtime_get_sync? Yes. If it fails, the line fetch error would be triggered again. Probably the set_cs callback should also be changed, so that it can return an error code. This is not something for 4.2-rc, though. > Anyway, what do you think about adding gcc macro for pm_runtime_get_sync > function which report compiler warning when return value is not checked? Adding __must_check to pm_runtime_get_sync() should be discussed separately with the relevant people in Cc. A quick grep in drivers/ reveals a huge amount of code not handling pm_runtime_get_sync's return code. -- Sebastian
On Wednesday 22 July 2015 15:33:18 Sebastian Reichel wrote: > Hi, > > On Wed, Jul 22, 2015 at 09:27:32AM +0200, Pali Rohár wrote: > > > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > > > > index 5867384..f7d9ffd 100644 > > > > --- a/drivers/spi/spi-omap2-mcspi.c > > > > +++ b/drivers/spi/spi-omap2-mcspi.c > > > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > > > > > > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > > > { > > > > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > > > > u32 l; > > > > > > > > /* The controller handles the inverted chip selects > > > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > > > enable = !enable; > > > > > > > > if (spi->controller_state) { > > > > + pm_runtime_get_sync(mcspi->dev); > > > > + > > > > > > Should the return code should be checked here as with the other > > > instances of pm_runtime_get_sync? > > Yes. If it fails, the line fetch error would be triggered again. > > Probably the set_cs callback should also be changed, so that it can > return an error code. This is not something for 4.2-rc, though. > > > Anyway, what do you think about adding gcc macro for pm_runtime_get_sync > > function which report compiler warning when return value is not checked? > > Adding __must_check to pm_runtime_get_sync() should be discussed > separately with the relevant people in Cc. A quick grep in drivers/ > reveals a huge amount of code not handling pm_runtime_get_sync's return > code. > > -- Sebastian Two times, missing check for pm_runtime_get_sync() cased kernel crash on Nokia N900. See that crypto commits. I think all drivers which do not check should be fixed. Otherwise we can see another mysterious problems. Sebastian, can you CC relevant people? Or maybe start new thread?
On Wednesday 22 July 2015 04:03:07 Sebastian Reichel wrote: > Hi, > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > This code has my head spinning. > > > > I found that the errors do not occur when the driver is built into > > the kernel. > > > > I also found that with the patch below the errors go away. > > > > Not sure if it is acceptible but see if it fixes things on your > > side. > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index cf8b91b..3164d13 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > > > if (!spi->max_speed_hz) > > > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > - spi_set_cs(spi, false); > > - > > > > if (spi->master->setup) > > > > status = spi->master->setup(spi); > > > > + spi_set_cs(spi, false); > > + > > > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max > > --> %d\n", > > > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > mh. maybe a runtime PM issue? > > * external abort on non-linefetch: address cannot be accessed, > since the module's clocks are disabled > * built-in works, module not: built-in is probably a little bit > faster (module must not be loaded from filesystem), so that > the device has not yet been suspended > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > routine, which acquired runtime PM > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > prior pm_runtime_get_sync() > * With your workaround, the device has not yet returned to > suspend after the runtime PM acquisition in setup() > > So I suggest trying the following (compile tested only) patch: > > -- Sebastian > > diff --git a/drivers/spi/spi-omap2-mcspi.c > b/drivers/spi/spi-omap2-mcspi.c index 5867384..f7d9ffd 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct > spi_device *spi, int enable) > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > { > + struct omap2_mcspi *mcspi = > spi_master_get_devdata(spi->master); u32 l; > > /* The controller handles the inverted chip selects > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device > *spi, bool enable) enable = !enable; > > if (spi->controller_state) { > + pm_runtime_get_sync(mcspi->dev); > + > l = mcspi_cached_chconf0(spi); > > if (enable) > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device > *spi, bool enable) l |= OMAP2_MCSPI_CHCONF_FORCE; > > mcspi_write_chconf0(spi, l); > + > + pm_runtime_mark_last_busy(mcspi->dev); > + pm_runtime_put_autosuspend(mcspi->dev); > } > } This patch fix spi bus for me. Tested-by: Pali Rohár <pali.rohar@gmail.com>
On Saturday 25 July 2015 10:55:50 Pali Rohár wrote: > On Wednesday 22 July 2015 04:03:07 Sebastian Reichel wrote: > > Hi, > > > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > > > This code has my head spinning. > > > > > > I found that the errors do not occur when the driver is built into > > > the kernel. > > > > > > I also found that with the patch below the errors go away. > > > > > > Not sure if it is acceptible but see if it fixes things on your > > > side. > > > > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index cf8b91b..3164d13 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > > > > > if (!spi->max_speed_hz) > > > > > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > > > - spi_set_cs(spi, false); > > > - > > > > > > if (spi->master->setup) > > > > > > status = spi->master->setup(spi); > > > > > > + spi_set_cs(spi, false); > > > + > > > > > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max > > > --> %d\n", > > > > > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > > > mh. maybe a runtime PM issue? > > > > * external abort on non-linefetch: address cannot be accessed, > > since the module's clocks are disabled > > * built-in works, module not: built-in is probably a little bit > > faster (module must not be loaded from filesystem), so that > > the device has not yet been suspended > > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > > routine, which acquired runtime PM > > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > > prior pm_runtime_get_sync() > > * With your workaround, the device has not yet returned to > > suspend after the runtime PM acquisition in setup() > > > > So I suggest trying the following (compile tested only) patch: > > > > -- Sebastian > > > > diff --git a/drivers/spi/spi-omap2-mcspi.c > > b/drivers/spi/spi-omap2-mcspi.c index 5867384..f7d9ffd 100644 > > --- a/drivers/spi/spi-omap2-mcspi.c > > +++ b/drivers/spi/spi-omap2-mcspi.c > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct > > spi_device *spi, int enable) > > > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > { > > + struct omap2_mcspi *mcspi = > > spi_master_get_devdata(spi->master); u32 l; > > > > /* The controller handles the inverted chip selects > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device > > *spi, bool enable) enable = !enable; > > > > if (spi->controller_state) { > > + pm_runtime_get_sync(mcspi->dev); > > + > > l = mcspi_cached_chconf0(spi); > > > > if (enable) > > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device > > *spi, bool enable) l |= OMAP2_MCSPI_CHCONF_FORCE; > > > > mcspi_write_chconf0(spi, l); > > + > > + pm_runtime_mark_last_busy(mcspi->dev); > > + pm_runtime_put_autosuspend(mcspi->dev); > > } > > } > > This patch fix spi bus for me. > > Tested-by: Pali Rohár <pali.rohar@gmail.com> > Sebastian or Michael: Can you add check for pm_runtime_get_sync() function and send patch for including in mainline kernel?
Hi Pali, On Tue, Jul 28, 2015 at 10:39:32AM +0200, Pali Rohár wrote: > Sebastian or Michael: Can you add check for pm_runtime_get_sync() > function and send patch for including in mainline kernel? This actually already happened before your Tested-By. I accidently did not CC you, sorry for that: https://lkml.org/lkml/2015/7/22/581 https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?h=for-linus&id=5f74db105b1c0980c9863e7a7d1bc0525e0316e8 -- Sebastian
On Tuesday 28 July 2015 14:26:13 Sebastian Reichel wrote: > Hi Pali, > > On Tue, Jul 28, 2015 at 10:39:32AM +0200, Pali Rohár wrote: > > Sebastian or Michael: Can you add check for pm_runtime_get_sync() > > function and send patch for including in mainline kernel? > > This actually already happened before your Tested-By. > I accidently did not CC you, sorry for that: > > https://lkml.org/lkml/2015/7/22/581 > https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?h=for-linus&id=5f74db105b1c0980c9863e7a7d1bc0525e0316e8 > > -- Sebastian Ok, so it will be fixed in 4.2. Thanks!
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index 5867384..f7d9ffd 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) { + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); u32 l; /* The controller handles the inverted chip selects @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) enable = !enable; if (spi->controller_state) { + pm_runtime_get_sync(mcspi->dev); + l = mcspi_cached_chconf0(spi); if (enable) @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) l |= OMAP2_MCSPI_CHCONF_FORCE; mcspi_write_chconf0(spi, l); + + pm_runtime_mark_last_busy(mcspi->dev); + pm_runtime_put_autosuspend(mcspi->dev); } }