On Tue 25 Aug 2020 at 12:20, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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
