linux-kernel - Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 29 Sep 2023 23:08:48 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Sean Young <sean@...s.org>
Cc:     Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>,
        linux-media@...r.kernel.org, Tony Lindgren <tony@...mide.com>,
        Russell King <linux@...linux.org.uk>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Timo Kokkonen <timo.t.kokkonen@....fi>,
        Pali Rohár <pali.rohar@...il.com>,
        "Sicelo A . Mhlongo" <absicsz@...il.com>,
        linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic
 pwm-ir-tx

Hello Sean,

On Fri, Sep 29, 2023 at 05:20:09PM +0100, Sean Young wrote:
> On Fri, Sep 29, 2023 at 11:49:52AM +0300, Ivaylo Dimitrov wrote:
> > On 26.09.23 г. 23:18 ч., Sean Young wrote:
> > > I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
> > > directly improves the situation even a bit, then that would be great.
> > 
> > The issue with hrtimers is that we cannot use them directly, as
> > pwm_apply_state() may sleep, but hrtimer function is called in atomic
> > context.
> 
> I've also been looking at this problem and came to same conclusion: the
> fact that pwm_apply_state() sleeps is a huge problem.
> 
> 1) The vast majority of pwm drivers don't sleep, or could even be converted
>    to spinlocks (e.g pwm-sifive.c could use spinlocks, as far as I can see).
> 
> 2) Sure, some pwm devices are on i2c busses, so the driver needs to sleep.
>    Those devices aren't great for what we're trying to do here, since the
>    sleeping may cause delays and affect the generated signal.
> 
> What would be ideal here is to have pwm-ir-tx work in atomic context if
> a non-sleeping pwm device is used, and another (non-optimal) code path
> for sleeping pwm drivers. We could even just refuse to run on sleeping pwm
> drivers.
> 
> Uwe what do you think of this idea? The pwm api could have a
> bool pwm_may_sleep(struct pwm *pwm) function,

It's certainly possible. The idea of introducing the might_sleep() was
to catch atomic users and if some appear to be able to evaluate if
something needs to be done. See commit 4ad91a227817 ("pwm: Make it
explicit that pwm_apply_state() might sleep").

It complicates things concerning my last bigger pwm series, see
https://lore.kernel.org/linux-pwm/20230808171931.944154-102-u.kleine-koenig@pengutronix.de/
which introduces a mutex_lock() in pwm_apply_state(). Hmm.

> and pwm_apply_state() does 
> not contain might_sleep() - only the driver-specific apply calls might_sleep().

I'd replace the might_sleep() by something like
might_sleep_if(pwm_may_sleep(pwm)); but that's an implementation detail.

> It would be nice if this could all be done at compile time through e.g. a
> device tree attribute.

I wouldn't have something like "linux,slow-pwm" or similar in the device
tree, and I'd expect the dt maintainers to shoot down something like
that, too. What is the problem with a pwm_can_sleep() function only?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)