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

[Thierry Reding]

On Thu, Feb 07, 2019 at 09:27:51AM +0100, Thierry Reding wrote:
> On Wed, Feb 06, 2019 at 07:12:04PM +0100, Philipp Zabel wrote:
> > On Wed, 2019-02-06 at 17:00 +0100, Thierry Reding wrote:
> [...]
> > > That way we operate on the same reset control, but we wouldn't need to
> > > iterate over all existing reset controls in order to determine whether
> > > we can acquire or not.
> > 
> > How could we decide in reset_control_assert whether the provided rstc is
> > allowed to change the reset line if both rstc handles point to the same
> > struct reset_control?
> 
> The idea was that acquire/release would basically act as lock/unlock for
> the reset control. So consumers would always have to call acquire()
> before assert()/deassert()/reset() and they would be allowed to continue
> only if acquire() returns success. Of course that's something you can
> only enforce during code review, but that's pretty much the same as with
> any type of locking.
> 
> So basically the idea is that if a consumers acquire() call succeeds,
> the acquired flag gets set on the reset control and that consumer
> becomes the only user allowed to modify the reset control. Any other
> consumers would call acquire() and fail because the acquired is already
> true.
> 
> But what you proposed works for me. We can always find constructive ways
> to optimize this later if we need or want to.
> 
> Another possibility would be to keep an additional, separate list of the
> temporarily exclusive resets so that only that list would have to be
> iterated to find the ones that are relevant.
> 
> > > >  			if (WARN_ON(!rstc->shared || !shared))
> > > >  				return ERR_PTR(-EBUSY);
> > > 
> > > With the above I think we could just extend this list of conditions with
> > > 
> > > 	|| acquired
> > > 
> > > and things should work the same. Or perhaps I'm missing something.
> > >
> > > Other than that this really looks like a very nice solution.
> > 
> > Well, apart from the API function names...
> > devm_reset_control_get_optional_exclusive_released(dev, "id");
> > would be a mouthful.
> 
> Yeah, the combinations are somewhat awkward. However, I would expect the
> temporarily exclusive resets to be required in most cases, so that would
> at least make the name a little shorter.

Quick update: I finally had a bit of time to look at this and I've got
something that works. There were a couple of minor issues with the patch
and I had to extend support for acquire/release to reset arrays for this
particular use-case, but overall it seems to be working pretty well.

I'll clean up the patches that I have and then send out for review. It's
unfortunately going to be a bit of a mess staging these changes since
they are spread over three different subsystems and I think there can be
subtle failures between the PMC and SOR patches, so perhaps it'd be best
to apply those to one tree, or maybe even squash the changes into a
single commit to avoid any surprises.

Thierry

Attachment: signature.asc
Description: PGP signature