| Message ID | 20200522155005.46099-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8fede89f853a7bfc671f9d8f069ccfe4190061f3 |
| Headers | show |
| Series | spi: Make spi_delay_exec() warn if called from atomic context | expand |
On Fri, May 22, 2020 at 04:50:05PM +0100, Mark Brown wrote: > If the delay used is long enough the spi_delay_exec() will use a sleeping > function to implement it. Add a might_sleep() here to help avoid callers > using this from an atomic context and running into problems at runtime on > other systems. Thanks! Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Suggested-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/spi/spi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index e02f434affca..c69d23379e08 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1160,6 +1160,8 @@ int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer) > { > int delay; > > + might_sleep(); > + > if (!_delay) > return -EINVAL; > > -- > 2.20.1 >
On Fri, May 22, 2020 at 07:02:33PM +0300, Andy Shevchenko wrote: > On Fri, May 22, 2020 at 04:50:05PM +0100, Mark Brown wrote: > > If the delay used is long enough the spi_delay_exec() will use a sleeping > > function to implement it. Add a might_sleep() here to help avoid callers > > using this from an atomic context and running into problems at runtime on > > other systems. > > Thanks! > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> We may improve it later, though, to be smarter and spill a warning only when it uses non-atomic delays. For now this is good enough.
On Fri, May 22, 2020 at 07:04:17PM +0300, Andy Shevchenko wrote: > On Fri, May 22, 2020 at 07:02:33PM +0300, Andy Shevchenko wrote: > > On Fri, May 22, 2020 at 04:50:05PM +0100, Mark Brown wrote: > > > If the delay used is long enough the spi_delay_exec() will use a sleeping > > > function to implement it. Add a might_sleep() here to help avoid callers > > > using this from an atomic context and running into problems at runtime on > > > other systems. > > > > Thanks! > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > We may improve it later, though, to be smarter and spill a warning only when it > uses non-atomic delays. For now this is good enough. IMO this wouldn't be better than the current solution. might_sleep() is called "might" to warn that the called method may get to sleep, not shall, not will. As I see it it's better to warn about the consequences straight away, but not at the point when the sleeping method is actually called in the atomic context. -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, May 22, 2020 at 07:10:01PM +0300, Serge Semin wrote: > On Fri, May 22, 2020 at 07:04:17PM +0300, Andy Shevchenko wrote: > > We may improve it later, though, to be smarter and spill a warning only when it > > uses non-atomic delays. For now this is good enough. > IMO this wouldn't be better than the current solution. might_sleep() is called > "might" to warn that the called method may get to sleep, not shall, not will. As > I see it it's better to warn about the consequences straight away, but not at the > point when the sleeping method is actually called in the atomic context. Yes, this is the whole point of might_sleep() - it's to ensure that we always get a warning about code that might sleep, rather than only getting a warning if we happen to go down a path that does actually sleep. This means we're less likely to run into bugs at runtime in unusual use cases.
On Fri, 22 May 2020 16:50:05 +0100, Mark Brown wrote: > If the delay used is long enough the spi_delay_exec() will use a sleeping > function to implement it. Add a might_sleep() here to help avoid callers > using this from an atomic context and running into problems at runtime on > other systems. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: Make spi_delay_exec() warn if called from atomic context commit: 8fede89f853a7bfc671f9d8f069ccfe4190061f3 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index e02f434affca..c69d23379e08 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1160,6 +1160,8 @@ int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer) { int delay; + might_sleep(); + if (!_delay) return -EINVAL;
If the delay used is long enough the spi_delay_exec() will use a sleeping function to implement it. Add a might_sleep() here to help avoid callers using this from an atomic context and running into problems at runtime on other systems. Suggested-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/spi/spi.c | 2 ++ 1 file changed, 2 insertions(+)