On Tue, 2020-08-25 at 16:20 +0200, Jerome Brunet wrote:

On Tue 25 Aug 2020 at 12:20, Philipp Zabel [off-list ref] wrote:

quoted

On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
[...]

quoted

In practice, I think your proposition would work since the drivers
sharing this USB reset line are likely to be probed/suspended/resumed at
the same time but ...

If we imagine a situation where 2 device share a reset line, 1 go in
suspend, the other does not - if the first device as control of the
reset, it could trigger it and break the 2nd device. Same goes for
probe/remove()

I agree it could be seen as unlikely but leaving such race condition
open looks dangerous to me.

You are right, this is not good enough.

quoted

quoted

Is this something that would be feasible for your combination of
drivers? Otherwise it is be unclear to me under which condition a driver
should be allowed to call the proposed reset_control_clear().

I was thinking of reset_control_clear() as the counter part of
reset_control_reset().

I'm not particularly fond of reset_control_clear as a name, because it
is very close to "clearing a reset bit" which usually would deassert a
reset line (or the inverse).

It was merely a suggestion :) any other name you prefer is fine by me

Naming is hard. All metaphors I can think of are either a obscure or
clash with other current usage. My instinct would be to call this
"resetting the (reset) control", but _reset() is already taken, with the
opposite meaning. How about _rewind() or _rearm()? Not sure if those are
good metaphors either, but at least there is no obvious but incorrect
interpretation. I kind of wish reset_control_reset() would be called
reset_control_trigger() instead.

quoted

quoted

When a reset_control_reset() has been called once, "triggered_count" is
incremented which signals that the ressource under the reset is
"in_use" and the reset should not be done again.

"triggered_count" would then have to be renamed to something like
"trigger_requested_count", or "use_count". I wonder it might be possible
to merge this with "deassert_count" as they'd share the same semantics
(while the count is > 0, the reset line must stay deasserted).

Sure. Could investigate this as a 2nd step ?

Yes.

I'd like to bring a solution for our meson-usb use case quickly - even
with the revert suggested, we are having an ugly warning around suspend

I understand. Let's still do this carefully :)

quoted

quoted

reset_control_clear()
would be the way to state that the ressource is no longer used and, that
from the caller perspective, the reset can fired again if necessary.

If we take the probe / suspend / resume example:
* 1st device using the shared will actually trigger it (as it is now)
* following device just increase triggered_count

If all devices go to suspend (calling reset_control_clear()) then
triggered_count will reach zero, allowing the first device resuming to
trigger the reset again ... this is important since it might not be the
one which would have got the exclusive control

If any device don't go to suspend, meaning the ressource under reset
keep on being used, no reset will performed. With exlusive control,
there is a risk that the resuming device resets something already in use.

Regarding the condition, on shared resets, call reset_control_reset()
should be balanced reset_control_clear() - no clear before reset.

Martin, is this something that would be useful for the current users of
the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
meson8b-usb2 with reset-meson)?

I'm not Martin but these devices are the origin of the request/suggestion.

So these two phy drivers are used together with dwc3-meson-g12a?
Will you change them to use the new API as well?

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel