Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hi Maxime,

Am Mittwoch, den 16.12.2015, 11:29 +0100 schrieb Maxime Ripard:
> On Mon, Dec 14, 2015 at 10:50:55AM +0100, Philipp Zabel wrote:
> > Am Montag, den 14.12.2015, 10:36 +0100 schrieb Maxime Ripard:
> > > Hi,
> > > 
> > > On Fri, Dec 11, 2015 at 04:41:58PM +0100, Hans de Goede wrote:
> > > > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > > > index c4c097d..1cca8ce 100644
> > > > --- a/include/linux/reset.h
> > > > +++ b/include/linux/reset.h
> > > > @@ -11,6 +11,8 @@ int reset_control_reset(struct reset_control *rstc);
> > > >  int reset_control_assert(struct reset_control *rstc);
> > > >  int reset_control_deassert(struct reset_control *rstc);
> > > >  int reset_control_status(struct reset_control *rstc);
> > > > +int reset_control_assert_shared(struct reset_control *rstc);
> > > > +int reset_control_deassert_shared(struct reset_control *rstc);
> > > 
> > > Shouldn't that be handled in reset_control_get directly?

I think I see your point now. Maybe we should add a flags parameter to
reset_control_get and/or wrap it in two versions,
reset_control_get_exclusive and reset_control_get_shared (or just add
the _shared variant). Then reset_control_get(_exclusive) could return
-EBUSY if a reset line is already in use.

> > This is about different expectations of the caller.
> > A driver calling reset_control_assert expects the reset line to be
> > asserted after the call.
> 
> Is that behaviour documented explicitly somewhere?

/**                                                                                                                                           
 * reset_control_assert - asserts the reset line
 * @rstc: reset controller
 */

Also, that expected behavior matches the function name, which I like.
So I still welcome adding new API calls for the shared/refcounting
variant.

> > A driver calling reset_control_assert_shared
> > just signals that it doesn't care about the state of the reset line
> > anymore.
> > We could just as well call the two new functions
> > reset_control_deassert_get and reset_control_deassert_put.
> 
> What happens if you mix them? What happens if you have several drivers
> ignoring this API?

The core should give useful error messages and disallow non-shared reset
calls on shared lines.

> The current default API totally allows to have several drivers getting
> the same reset line, and happily poking that reset line without any
> way for the others to A) know they're not the single users B) let them
> know their device has been reset.

That's why I'd like the WARN_ON and error return in reset_control_* when
the reset_line reference count is > 1.

> And not being able to tell at the consumer level if and when our
> device is going to be reset behind our back is a big issue. Because
> then, we simply have to expect it can be reset at any point in time,
> good luck writing a driver with that expectation.

Yes, that is unacceptable.

> The reset framework should make sure that the shared case is an
> exception, and not the default case (and make sure that it cannot
> happen in the default case). Just like for any other framework with
> similar resources constraints.

regards
Philipp