Re: [PATCH] reset: Don't WARN if trying to get a used reset control

[Thierry Reding]

On Tue, Feb 05, 2019 at 07:05:41PM +0100, Philipp Zabel wrote:
> Hi Thierry,
> 
> On Fri, 2019-02-01 at 15:00 +0100, Thierry Reding wrote:
> [...]
> > It sounds pretty good and elegant actually. Let me try to restate to see
> > if I understand correctly:
> > 
> > So basically what you're saying is that we would be changing the
> > definition of exclusive resets to make them exclusive in terms of usage
> > rather than rejecting to acquire them multiple times, right? So we would
> > reinstate the possibility to request exclusive resets from multiple
> > sources, but add an _acquire()/_release() protocol to make sure the
> > users don't get in each other's way.
> 
> Yes.ÂThough drivers currently expect that reset_control_get_exclusive
> returns 'acquired' reset lines, so that has to stay. This method should
> continue to fail if the same reset line is already acquired somewhere
> else.
> 
> There could be a new requestÂmethod (for lack of a better idea,
> for example: reset_control_get_exclusive_released) to request an
> initially released reset control. That method would not fail if the same
> reset line is already requested and acquired elsewhere.
> 
> > I had initially thought that this would have to be some other type of
> > reset (something between exclusive and shared) to avoid suprising the
> > current users of exclusive resets. However, on second thought, I don't
> > think that would be necessary. Given the WARN_ON, all users of exclusive
> > resets should at this point be truly exclusive and would therefore
> > continue to work normally.
> 
> I agree. But there must be no changes in behaviour for drivers that keep
> requesting exclusive resets and never all _acquire()/_release().
> 
> > One problematic case that comes to mind is how currently exclusive reset
> > controls will know that reset_control_assert() is safe to use if they
> > don't use reset_control_acquire() first.
> 
> Drivers that never call _acquire()/_release() must continue to work as
> they are, so exclusive reset controls have to be acquired by default.

I don't think they have to. See below.

> > Perhaps this is what you meant when you said:
> >
> > > That would also mean we'd have to add a way for the power domain drivers
> > > to request the reset control in the "released/don't care" state
> > > initially.
> 
> This is a consequence of the above. I thought that if
> reset_control_get_exclusive implicitly acquires the reset, there can't
> be two controls requested for the same reset line without another
> request method that returns its reset control in a 'released' state.
> For the power domain use case this does not seem to be necessary.
> 
> > I don't think we actually need that for power domains, because they will
> > typically be enabled over the duration of a device's driver's probe
> > anyway. So I think this would work:
> 
> Yes, since the driver that yields its reset line to the power domain
> driver dependsa on the power domain, by definition the power domain
> probes first and thus has the opportunity to request the reset control
> first and then immediately release it so that the other driver can be
> probed.
> 
> > 	power domains:
> > 
> > 		power_off()
> > 		{
> > 			reset_control_acquire();
> > 			reset_control_assert();
> > 		}
> > 
> > 		power_on()
> > 		{
> > 			reset_control_deassert();
> > 			reset_control_release();
> > 		}
> > 
> > 	SOR:
> > 
> > 		runtime_suspend()
> > 		{
> > 			reset_control_assert();
> > 			reset_control_release();
> > 		}
> > 
> > 		runtime_resume()
> > 		{
> > 			reset_control_acquire();
> > 			reset_control_deassert();
> > 		}
> 
> If the reset line is kept asserted while the power domain is turned off,
> it could indeed stay acquired by the power domain.

That's at least the way that it works on Tegra. Not sure if that is
universally applicable, though it seems logical to me from a hardware
point of view. It may not be required to keep the reset asserted while
the power domain is turned off, but I'd be surprised if there was a
requirement that it be deasserted while the power domain is off.

> > Hm... actually this means that power domains would have to request the
> > reset controls and call reset_control_acquire() during probe. That way
> > when the domain is powered on it will release the reset and free it up
> > for the SOR driver to use.
> 
> If you can guarantee that the power domain is powered up and releases
> the reset control before the SOR driver tries to request the reset line,
> that would work.

Well, that's the part that the acquire/release protocol would enforce.
If your device shares a reset control with a power domain, this should
work fine, provided that the device driver uses the reset only between
(or during) ->runtime_resume() and ->runtime_suspend().

> > This seems right because it also happens to work from a runtime PM
> > sequencing point of view.
> > 
> > I think the problem for single-user exclusive reset controls could be
> > solved by making use of the reference counting that's already used by
> > shared resets. We should be able to go ahead and use the reference
> > counting in general, and the acquire()/release() dance would only have
> > to be enforced when the reference count is > 1.
> 
> I'm not sure I understand what you mean by making use of the shared
> resets' refcounting. I suppose you could check the reset_control's
> refcnt to determine whether there are other users. Or did you want to
> repurpose deassert_count?

Yes, if we make the new acquire/release protocol only apply if the
reset control's refcnt is > 1, then I think we should be able to keep
exclusive resets working exactly the same way they are now. Since the
exclusive resets are guaranteed to have refcnt == 1 right now, there
should be no need for them to be implicitly acquired at request time.
However, when you start using them from multiple users, the acquire/
release protocol would kick in and refuse to assert/deassert before
you've acquired them.

I think we should also be able to keep the WARN_ON(), albeit maybe in
different form. We could have one if the user tries to acquire a reset
control that's already acquired. And we could also have a warning if we
assert or deassert a reset control that hasn't been acquired *and* that
has refcnt > 1. With the above I think "acquired" really needs to mean
acquired && refcnt > 1, and then the current exclusive resets should
work out of the box.

I was wondering whether maybe we needed a way to track the current owner
of a reset control, so that we could be clever about when acquire needs
to fail or succeed (e.g. acquiring twice by the same owner would be
okay), but I think that's overkill. I think this should just be treated
like a lock and drivers need to make sure they properly balance acquire
and release calls.

Thierry

Attachment: signature.asc
Description: PGP signature