Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-KÃnig wrote:
> Hello Thierry,
> 
> On Wed, Apr 01, 2020 at 08:28:33PM +0200, Thierry Reding wrote:
> > On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-KÃnig wrote:
> > > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > > > I think there's a fine line to be walked here. I agree that we should
> > > > aim to have as much consistency between drivers as possible. At the same
> > > > time I think we need to be pragmatic. As Lokesh said, the particular use
> > > > case here requires this type of on-the-fly adjustment of the PWM period
> > > > without stopping and restarting the PWM. It doesn't work otherwise. So
> > > > th alternative that you're proposing is to say that we don't support
> > > > that use-case, even though it works just fine given this particular
> > > > hardware. That's not really an option.
> > > 
> > > I understand your opinion here. The situation now is that in current
> > > mainline the driver stops the hardware for reconfiguration and it
> > > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > > (accepting that maybe a wrong period is emitted). What if someone relies
> > > on the old behaviour? What if in a year someone comes and claims the
> > > wrong period is bad for their usecase and changes back to
> > > stop-to-update?
> > 
> > Relying on that old behaviour is wrong.  If you really need to rely on
> > the PWM being stopped before reconfiguration, then you should
> > explicitly request that by first disabling the PWM, then reconfiguring
> > it and then reenabling it.
> 
> You got me wrong here. See below.
> 
> > > When I write a consumer driver, do I have a chance to know how the PWM,
> > > that I happen to use, behaves? To be able to get my consumer driver
> > > reliable I might need to know that however.
> > 
> > No, there's currently no way of knowing. As such, I think the most
> > sensible thing to do at this point is to work with the API in order to
> > get the behaviour that you want.
> 
> I want to switch from (duty_cycle, period) = (d1, p1) to
> (duty_cycle, period) = (d2, p2) without seeing (d1, p2) or (d2, p1) on
> the wire. How do I do this with Lokesh's patch applied?
> 
> In my eyes this is something sensible to rely on. I agree that relying
> on a stop implemented by the driver is broken, but not wanting to seeing
> a (d1, p2) is sane.

I suppose it could be a sane requirement under some circumstances. In
other circumstances it may not matter. Making it a hard requirement
unconditionally would preclude the driver's usage in cases where it
doesn't matter.

Now, for many cases that als can never happen because the period is
always fixed. For example, the PWM consumer bindings specify that a
consumer should provide a fixed period in device tree. So once the
PWM has been configured that will never change, so you can only ever
be in a situation where you want to go from (d1, p) to (d2, p), so
there won't actually be a glitch.

That's certainly not true for all cases, and specifically with sysfs
you can always change the period as well as the duty-cycle.

> > Now if you're unlucky the driver won't support that and you'll notice
> > eventually. But there's also nothing you can do about that if the
> > hardware doesn't work that way. Even if the PWM framework had a way of
> > querying these types of peculiarities it wouldn't really proved much of
> > an advantage since the driver would just refuse to probe rather than
> > attempting to do this and maybe succeeding because you got lucky.
> 
> It depends on the use case. If it's about a display that then has no
> backlight the damage is limited. If however I drive a step motor that
> rotates a laser some unexpected behaviour that only happens once in
> 100000 cases and so isn't catched during development is quite bad.

You probably wouldn't be using sysfs to control the PWM in such a case.
My understanding is that stepper motors also typically use either a
fixed duty-cycle or a fixed period. I would also think that it wouldn't
matter if you stopped a PWM, reconfigured it and then started it again
for that particular application.

So I understand that there can be situations where this kind of glitch
isn't optimal, but I think we also need to be pragmatic and see what the
real use-cases are. I doubt that anyone trying to control a potentially
lethal laser would be using some standard PWM IP and run Linux to
control it. There are just too many things that could go wrong
(scheduling interfering, kernel crashing, ...) for that to be a good
idea.

> > > > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > > > don't know what the alternative would be. Do we want to tell people that
> > > > > > a given PWM controller can't be used if it doesn't work according to our
> > > > > > expectations? That's hard to argue if that controller works just fine
> > > > > > for all known use-cases.
> > > > > 
> > > > > I'd like have some official policy here which of the alternatives is the
> > > > > preferred cheat.
> > > > > 
> > > > > The situation here is that period and duty_cycle cannot be updated
> > > > > atomically. So the two options are:
> > > > > 
> > > > >  - stop shortly
> > > > >  - update with hardware running and maybe emit a broken period
> > > > 
> > > > I think we can already support both of those with the existing API. If
> > > > a consumer wants to stop the PWM while reconfiguring, they should be
> > > > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > > > equivalent) and for the second case they can just do pwm_config() (or
> > > > the atomic equivalent).
> > > 
> > > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > > consumer driver author and I want: atomic update and if this is not
> > > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > > So I cannot benefit from a good driver/hardware that can do atomic
> > > updates? Or I have to patch each driver that I actually use to use
> > > stop-to-update?
> > 
> > You could do what everybody does and just assume that atomic update
> > works.
> 
> There are people out there that are more demanding. If you have 1000000
> machines in the field and only then find out that they all fail to
> operate correctly with a certain small but positive probability and you
> have to send someone to each machine to fix that, that's bad.

Agreed. But that's not really what we're talking about here, right? This
isn't some undefined behaviour that would spontaneously trigger. We're
still talking about deterministic behaviour that's going to be the same
whether you do it once in a test lab or a million times in the field. A
PWM-controlled backlight that you adjust the brightness of in the lab is
going to work the same way as in the field. Any potential glitch would
be happening in the lab as well. It would in fact, constantly happen.

> > If it works with the particular PWM device/driver that you
> > have that may be all you care about. Obviously that may not be true
> > for a different chip.
> > 
> > On the other hand, if you absolutely must ensure that there must never
> > be any glitches whatsoever but don't care whether the PWM goes through a
> > disable/enable sequence, then doing so explicitly is going to be your
> > best bet. From a consumer driver point of view it isn't going to matter
> > because even if we had a way of distinguishing between these
> > capabilities you'd still have to have code to deal with both.
> 
> There is a nice upside however: You have a good chance to notice this
> problem before the 1000000 devices are shipped to production and then
> escalate the problem and let the responsible people make an informed
> choice.

I suspect that what will really happen is that somebody will just hack
the driver and remove the restriction, check that it works and ship it.

> > So the only relevant use-case here would be if you had a requirement to
> > perform on-the-fly-and-strictly-correct updates. The API doesn't give
> > those kinds of guarantees. So we would need an extension that consumers
> > can query to determine whether what they want to do will work.
> > 
> > But like I said above, the chances that you will run into this are slim
> > because use-cases are typically known ahead of time and devices are
> > designed to be able to support them. So if you build a device that needs
> > to support these strict requirements, then you need to make sure the
> > hardware supports it, too. And when the hardware supports it, then the
> > driver should implement ->config() or ->apply() in a way that allows
> > this.
> 
> In my bubble that sometimes fails. The hardware is done and only then
> someone notices that a certain case wasn't considered.

Of course this sometimes happens. And then it's actually great if the
API isn't all too pedantic and still allows you to adjust that backlight
brightness rather than flatout refusing to do anything, leaving you with
a bunch of devices that you have to scrap and then do a redesign.

So ultimately you end up "faking it", but if it works, it works.

For the rare cases where you can't fake it, it doesn't really matter
whether the framework refuses to do something because it knows that the
hardware isn't capable of doing it, or whether the driver will simply
try its best to do what it was asked to do and fails to deliver. You'll
notice one way or another and then you need to go back to the drawing
board and redo the design.

> > > > Some hardware may actually require the PWM to be disabled before
> > > > reconfiguring, so they won't be able to strictly adhere to the second
> > > > use-case.
> > > > 
> > > > But as discussed above, I don't want to strive for a lowest common
> > > > denominator that would preclude some more specific use-cases from
> > > > working if the hardware supports it.
> > > > 
> > > > So I think we should aim for drivers to implement the semantics as
> > > > closely as possible. If the hardware doesn't support some of these
> > > > requirements strictly while a particular use-case depends on that, then
> > > > that just means that the hardware isn't compatible with that use-case.
> > > > Chances are that the system just isn't going to be designed to support
> > > > that use-case in the first place if the hardware can't do it.
> > > > 
> > > > The sysfs interface is a bit of a special case here because it isn't
> > > > possible to know what use-cases people are going to come up with.
> > > 
> > > In my eyes the sysfs interface isn't special here. You also don't know
> > > what the OMAP PWM hardware is used for.
> > 
> > But platform designers will know what their device will be used for.
> 
> Right. But if you rely on the people chosing the components for the
> platform to notice that the SoC's PWM has a glitch that is bad for the
> use case I think that's naive.

See above. If the glitch is bad enough to make it unusable you're going
to notice and you have to redesign, whether the PWM framework rejects a
configuration or not.

My point is that if the glitch is minor enough not to be noticed, the
PWM framework not getting in the way may make you (and whoever's got
their money in this) very happy.

> > > > It's most likely that they'll try something and if it doesn't work
> > > > they can see if a driver patch can improve things.
> > > 
> > > So either the group who prefers "stop-to-update" or the group who
> > > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > > driver patch?
> > 
> > No, the group that prefers "stop-to-update" should make that explicit
> > and write a consumer driver that first disables, then reconfigured and
> > then reenables the PWM.
> 
> Both groups prefer atomic updates, and just prefer different cluches if
> atomic isn't possible. So doing an explicit stop is bad. Also I'd expect
> the timing of a driver doing the stop is better than when the consumer
> explicitly stops.

Yes, perhaps. It obviously depends a lot on what exactly the time is
that we're talking about here. If we're talking about a single pulse
every second, whether the driver stops and restarts or whether the
consumer does is probably negligible.

But again, I think if we really run into cases where the consumer knows
exactly which behaviour it wants, then I think we need to extend the API
to enable it to transport such notions.

We currently can't, and assumptions baked into the API are bound to be
the wrong ones every now and then.

> > If they don't *need* to update the PWM on the fly, then performing two
> > additional steps that would be happening anyway won't matter, right?
> 
> I don't agree here. Moreover the consumer might be preempted between
> stop and reconfiguration. (And you don't want to disable preemption over
> a pwm-API calls as they might take quite some time assuming they block
> until periods are completed.)

I hate to break it to you, but if timing is really that critical, you're
probably better off not using Linux.

> > > > One possible extension that I can imagine would be to introduce some
> > > > sort of capability structure that drivers can fill in to describe the
> > > > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > > > could describe that they are able to change the period and/or duty cycle
> > > > while the PWM is on. There could be another capability bit that says
> > > > that the current period will finish before new settings are applied. Yet
> > > > another capability could describe that duty-cycle and period can be
> > > > applied atomically. Consumers could then check those capabilities to see
> > > > if they match their requirements.
> > > > 
> > > > But then again, I think that would just make things overly complicated.
> > > > None of the existing consumers need that, so it doesn't seem like there
> > > > is much demand for that feature. In practice I suspect most consumers
> > > > work fine despite potentially small deviations in how the PWM behaves.
> > > 
> > > I think the status quo is what I asked about above: People use sysfs and
> > > if the PWM behaves different than needed, the driver is patched and most
> > > of the time not mainlined. If your focus is to support a certain
> > > industrial system with a defined use case, this is fine. If however you
> > > target for an universal framework that works for any combination of
> > > consumer + lowlevel driver without patching (that at least is able to
> > > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> > 
> > Again, my response to this is: how is this going to be beneficial? In
> > practice the way that this would work is that the consumer driver would
> > fail if presented with a PWM that doesn't meet the strict requirements.
> > Now if the requirements really are that strict, then that sounds like a
> > good idea.
> > 
> > But one issue I foresee with this is that we'll end up giving consumers
> > too much of a toolkit to restrict things. What if the consumer driver
> > author assumes wrongly that a given set of requirements exists?
> 
> If you want to catch usage errors you're doomed anyhow.
> 
> > What if for some combination of hardware that doesn't strictly conform
> > to those requirements it might still work?
> 
> Then this is a bug that can be fixed. And it's a bug that is not in the
> PWM framework's area, so we even don't really have to care.

It's still the responsibility of the PWM framework to provide the knobs
that make sense in terms of what the hardware capabilities are.

> > Sometimes you may not notice the difference, at other times there may
> > be some impact like a visual glitch or so, but that may be something
> > that users are willing to accept rather than not have support at all.
> 
> If laxer requirements are ok, they shouldn't request strict ones.

Well yeah. That's exactly why I'm pushing back on this. So far I haven't
yet seen a case where there actually were any such strict requirements.

> > > Also this means that whenever a system designer changes something on
> > > their machine (kernel update, different hardware, an new usecase for a
> > > PWM) they might have to reverify if the given PWM driver behaves as
> > > needed.
> > 
> > I don't expect this type of change to happen very often.
> 
> Yeah, I also expect that most things don't break on such a change. But
> Lokesh's patch is an example that justifies to be vigilant because the
> implemented change in behaviour is a good one for Lokesh but might break
> things for someone else. And I don't expect that the common test
> coverage to be that good to catch the glitch during test.

That's certainly true, but so far nobody's complained about this patch
breaking their use-case, so I'm going to assume for now that it's
harmless. If that changes at some point we need to revisit this of
course.

However, it's no use sitting on patches waiting for somebody to report a
regression, otherwise we could never make changes to a driver. Does the
absence of a regression report mean that there is no regression, or that
the regression just hasn't been noticed yet?

> > There's always going to be some type of fine-tuning before a driver's
> > behaviour is completely stabilized. And then there could still always
> > be other factors impacting behaviour that aren't even related to the
> > PWM framework.
> 
> I think I don't get what you intend to say here. I understand "As
> updating the kernel might make a PLL unstable, changing behaviour for
> the PWM is fine."

What I'm trying to say is that it's normal for a driver to go through
these kinds of changes. It used to work for some use-cases, now it's
being extended to work for one more. The behaviour is now slightly
different, but hopefully it won't impact existing use-cases.

That's in contrast to saying that we can never change existing drivers
because it may cause existing use-cases to fail.

Furthermore there are so many moving parts in Linux that upgrading to a
new kernel always requires you to revalidate functionality.

> > I suspect that most people will have an array of tests to validate that
> > everything still works after a kernel update. Obviously we don't want a
> > new kernel to behave completely differently, but we're not talking about
> > that here. This is merely dropping a needless disable/enable from a
> > configuration. If somebody was relying on this happening they were wrong
> > to rely on it to begin with because the API does not guarantee it.
> > 
> > > My suggestion for now is to start documenting how the drivers behave
> > > expanding how limitations are documented in some drivers. So maybe
> > > change from "Limitations" to "Implementation and Hardware Details"?
> > 
> > Yes, collecting such information is always a good idea.
> 
> So Lokesh's patch should at least be amended to add something like
> 
>  * Limitations:
>  * - When changing both duty cycle and period, we cannot prevent in
>  *   software that the output might produce a period with mixed
>  *   settings (new period length and old duty cycle) without stopping
>  *   the hardware.
> 
> at the top of the driver.

Something to that effect already exists in the driver.

Thierry

Attachment: signature.asc
Description: PGP signature