linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Oleksij Rempel <o.rempel@pengutronix.de>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	David Jander <david@protonic.nl>,
	Robin van der Gracht <robin@protonic.nl>,
	linux-iio@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Re: [PATCH v1 2/2] iio: adc: tsc2046: fix sleeping in atomic context warning and a deadlock after iio_trigger_poll() call
Date: Mon, 5 Jul 2021 12:58:08 +0200	[thread overview]
Message-ID: <20210705105808.GA7671@pengutronix.de> (raw)
In-Reply-To: <20210705035440.35iualr6kkg22n56@pengutronix.de>

On Mon, Jul 05, 2021 at 05:54:40AM +0200, Oleksij Rempel wrote:
> Hi Jonathan,
> 
> On Sun, Jul 04, 2021 at 06:57:10PM +0100, Jonathan Cameron wrote:
> > On Fri, 25 Jun 2021 08:59:22 +0200
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > 
> > > If iio_trigger_poll() is called after IRQ was disabled, we will call
> > > reenable_trigger() directly from hard IRQ or hrtimer context instead of
> > > IRQ thread. In this case we will run in to multiple issue as sleeping in atomic
> > > context and a deadlock.
> > 
> > Hmm. This sounds like a problem that might bite us in other circumstances.
> > 
> > So do I have the basic issue right in thinking we have a race between
> > calling iio_trigger_poll() and having no devices still using that trigger?
> > Thus we end up with all of trig->subirqs not being enabled.
> > 
> > There was a previous discussion that the calls to iio_trigger_notify_done() in
> > iio_trigger_poll() are only meant to decrement the counter, as the assumption
> > was that the calls via threads would always happen later.  Unfortunately this
> > is all clearly a little bit racy and I suspect not many of the reenable() callbacks
> > are safe if they are called in interrupt context.
> > 
> > Perhaps an alternative would be to schedule the reenable() if we hit it from
> > that path thus ensuring it doesn't happen in a place where we can't sleep?
> > 
> > Would something like that solve your problem?
> 
> Yes :)

But the initial design of the driver wasn't that good, there were two
variables to decide what to do and now there is a proper state machine.
I see this as a cleanup, that also fixes the problem with the
re-enable().

> > I'd do it by having a new function
> > 
> > iio_trigger_notify_done_schedule() that uses a work struct to call
> > trig->ops->reenable(trig) from a context that can sleep.

Said that, the driver doesn't need the re-enable from sleeping context
anymore. If you provide an initial patch I can test that.

> > It's a rare corner case so I don't really care that in theory we might have
> > a device that was safe to reenable the trigger without sleeping.  That makes
> > it easier to just have one path for this which allows sleeping.

Sure, having always the same (i.e. sleeping context) makes it easier for
the driver, especially if the non sleeping re-enable is only called
during shutdown of the trigger-consumer if an interrupt comes in.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

      reply	other threads:[~2021-07-05 10:58 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25  6:59 [PATCH v1 1/2] iio: adc: tsc2046: fix scan interval warning Oleksij Rempel
2021-06-25  6:59 ` [PATCH v1 2/2] iio: adc: tsc2046: fix sleeping in atomic context warning and a deadlock after iio_trigger_poll() call Oleksij Rempel
2021-07-04 17:57   ` Jonathan Cameron
2021-07-05  3:54     ` Oleksij Rempel
2021-07-05 10:58       ` Oleksij Rempel [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210705105808.GA7671@pengutronix.de \
    --to=o.rempel@pengutronix.de \
    --cc=david@protonic.nl \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=jic23@kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    --cc=robin@protonic.nl \
    --subject='Re: [PATCH v1 2/2] iio: adc: tsc2046: fix sleeping in atomic context warning and a deadlock after iio_trigger_poll() call' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).