linux-pwm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Trent Piepho <tpiepho@gmail.com>
To: thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de,
	lee.jones@linaro.org, heiko@sntech.de, linux-pwm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Cc: Simon South <simon@simonsouth.net>
Subject: Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing
Date: Fri, 20 Nov 2020 17:09:58 -0800	[thread overview]
Message-ID: <2177360.ElGaqSPkdT@zen.local> (raw)
In-Reply-To: <20200919193306.1023-1-simon@simonsouth.net>

On Saturday, September 19, 2020 12:33:06 PM PST Simon South wrote:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the

> 
> @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device 
*pdev)
...
        ret = pwmchip_add(&pc->chip);
...
>  	}
>  
>  	/* Keep the PWM clk enabled if the PWM appears to be up and 
running. */
> -	if (!pwm_is_enabled(pc->chip.pwms))
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	if ((ctrl & enable_conf) != enable_conf)
>  		clk_disable(pc->clk);
>  

I came across this while trying to get a PBP working better.  It seems like 
the issue is the driver was calling pwm_is_enabled() without first requesting 
the pwm with pwm_get().  Which wouldn't even be possible normally, how would 
one get the pwm_chip to call pwm_is_enabled on, but the driver already has the 
pointer.

Anyway, it seems like this solution has a race.  Isn't the pwm live and 
requestable as soon as pwmchip_add() returns?  Which would mean that disabling 
the clock here could race with other code requesting and enabling the pwm.

Seems like it would be safer to check the initial state and turn off the clock 
before calling pwmchip_add.



  parent reply	other threads:[~2020-11-21  1:10 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-19 19:33 [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing Simon South
2020-09-21  8:01 ` Uwe Kleine-König
2020-09-23 10:49 ` Heiko Stübner
2020-09-23 11:41 ` Thierry Reding
2020-11-21  1:09 ` Trent Piepho [this message]
2020-11-30  0:36   ` Simon South
2020-11-30  0:44     ` [PATCH] pwm: rockchip: Eliminate potential race condition when probing Simon South
2020-12-10 17:48       ` Thierry Reding
2020-12-10 21:00         ` Trent Piepho
2020-12-11 10:44           ` Robin Murphy
2020-12-19 20:32         ` Simon South

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=2177360.ElGaqSPkdT@zen.local \
    --to=tpiepho@gmail.com \
    --cc=heiko@sntech.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=simon@simonsouth.net \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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).