[PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Mark Brown]

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(+)

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

Re: [PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Andy Shevchenko]

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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Serge Semin]

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
> 
> 

Re: [PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Make spi_delay_exec() warn if called from atomic context

[Mark Brown]

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