Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Simon South <simon@simonsouth.net>]

Trent Piepho <tpiepho@gmail.com> writes:
> 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.

Yes, I think that's true. Glad you caught this.

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

Yes. I tested this and it works, but on further consideration it seems
to me the code is actually doing things backwards: Instead of enabling
every PWM's clock and then turning off the clocks for PWMs that are not
themselves enabled, it should enable only the clocks for PWMs that
appear to be in use and leave the remaining clocks in their default
(presumably disabled) state. This should produce the same end result but
without the driver enabling clocks indiscriminately and without creating
a race condition.

I'll follow up with a patch for review that implements this change. I've
tested it as best I can on my own Pinebook Pro; everything works as it
did previously, with the display backlight on, no kernel hang and no
other apparent side effects.

> It seems like the issue is the driver was calling pwm_is_enabled()
> without first requesting the pwm with pwm_get().

This used to work because pwmchip_add() happened to invoke
rockchip_pwm_get_state(), which populates the PWM's pwm_state structure
from which pwm_is_enabled() reads. And I think that's why the code was
written the way it was originally: By waiting until pwmchip_add()
returned the PWM's state could be queried in a convenient manner,
without resorting to peeking at the hardware's registers.

Commit cfc4c189bc70 ("pwm: Read initial hardware state at request time")
changed this and pwmchip_add() no longer has the side effect of
populating the state structure, so the call to pwm_is_enabled() no
longer worked reliably. That's what I fixed with the patch you're
replying to, though now I wish I'd seen the larger issue.

As to why this code is needed in the first place (as I wondered recently
while working on it again), it seems to be a somewhat hacky way of
initializing the enable_count reference counter (see drivers/clk/clk.c)
of the already running clock to 1 instead of 0. This is necessary
because on SoCs like the RK3399 that use only a single clock for each
PWM, the driver treats the "APB" and "bus" clocks as referring to the
same physical device ("pc->pclk = pc->clk"), and without it functions
like rockchip_pwm_get_state() that enable the APB clock on entry and
disable it on exit would end up halting the PWM entirely.

-- 
Simon South
simon@simonsouth.net

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel