Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 25 Aug 2020 at 12:20, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
> [...]
>> 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.
>
>> > 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

>
>> 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 ?
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

>
>> 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.

>
> regards
> Philipp


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