On Tue, 2020-08-25 at 16:20 +0200, Jerome Brunet wrote: > 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 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. > > > 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 :) > > > 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
