Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Philipp Zabel]

Hi Geert,

On Wed, 2018-09-19 at 17:24 +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
[...]
> > I consider requesting exclusive access to a shared reset line a misuse
> > of the API. Are there such cases? Can they be fixed?
> 
> I guess there are plenty.

I did a cursory search for drivers that request exclusive reset controls
for resets that have multiple phandle references in the corresponding
DT. So far I have found none.

> Whether a line is shared or dedicated depends on SoC integration.
>
> The issue is that a driver requesting exclusive access has no way to know
> if the reset line is dedicated to its device or not.ÂIf no other
> driver requested the reset control (most drivers don't use reset
> controls), it will succeed.

True. It would be great to have a way to make sure an exclusive request
for a shared reset line never succeeds.

> > > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > > sure it has no impact on other hardware blocks.  This is e.g. the case
> > > for virtualization with device pass-through, where the host wants to
> > > reset any exported device before and after exporting it for use by the
> > > guest, for isolation.
> > > 
> > > Hence a new flag for dedicated resets is added to the internal methods,
> > > with a new public reset_control_get_dedicated() method, to obtain an
> > > exclusive handle to a reset that is dedicated to one specific hardware
> > > block.
> > 
> > I'm not sure a new flag is necessary, this is what exclusive resets
> > should be.
> 
> So perhaps the check should be done for the existing exclusive resets
> instead, without adding a new flag?

That would be my preference, if possible.

> > Also I fear there will be confusion about the difference between
> > exclusive (refering to the reset control) and dedicated (refering to
> > the reset line) reset controls.
> 
> Indeed, exclusive has multiple meanings here:
>   1. Exclusive vs. shared access to the reset control,
>   2. Reset line is dedicated to a single device, or shared with multiple
>      devices.

2. is the more important factor, and that's what I have in mind when
talking about exclusive vs. shared resets. Admittedly, the kernel doc
comments only mention 1.

> If we can simplify (exclusive == dedicated, 1.shared == 2.shared), life
> can become simpler.

Well, it still has to be possible for drivers to request 1.shared
control over a dedicated reset line, just because the same driver may
work on multiple SoCs, only some of which have that reset line 2.shared.
But if we could make sure that exclusive requestsÂare only possible for
dedicated reset lines, I'd be happier.

> > >   - I think __device_reset() should call __reset_control_get() with
> > >     dedicated=true.  However, that will impact existing users,
> > 
> > Which ones? And how?
> 
> I didn't actually check which drivers.
> If a reset is not dedicated, device_reset{,_optional}() will suddenly
> start to fail if
> a reset turns out to be not dedicated.
> Well, currently the device will be reset multiple times, so people would
> already have noticed...

Exactly. Naive exclusive control, as currently implemented,Âis bound to
fail if the reset line is shared. I am not aware of any cases where this
currently happens.
Of course there could always be those fragile cases where something just
works by accident and lucky timing.

[...]
> > I want to hear the device tree maintainers' opinion about this.
> > I'd very much like to have such a check for exclusive resets, but my
> > understanding is that we are not allowed to make the assumption that
> > other nodes' "reset" properties follow the common reset signal device
> > tree bindings.
> > 
> > Maybe this is something that should be checked in a device tree
> > validation step?
> 
> We already have SoCs where reset lines are shared among multiple on-chip
> devices. So dtc validation won't work, and a runtime check is needed.

Right, the dtb would have to be validated against driver code to get the
information whether a given phandle might be requested exclusively at
some point.

It would be better if this could be checked at runtime.

regards
Philipp