Messages in this thread | | | Subject | Re: [PATCH] reset: Add reset controller API | From | Amjad Ouled-Ameur <> | Date | Thu, 12 Nov 2020 14:44:40 +0100 |
| |
Hi Philipp,
Thank you very much for the review.
Please find my comments below:
On 02/10/2020 13:14, Philipp Zabel wrote:
> Hi Amjad, > > Thank you for the patch, comments below: > > On Thu, 2020-10-01 at 15:55 +0200, Amjad Ouled-Ameur wrote: >> An update on the patch title, since we don't add an API but extend it, >> The title should rather be: Add a new call to the reset framework > I think it should even say what functionality is added, for example > > "reset: make shared pulsed reset controls re-triggerable"
Will do !
>> Le jeu. 1 oct. 2020 à 15:28, Amjad Ouled-Ameur >> <aouledameur@baylibre.com> a écrit : >>> The current reset framework API does not allow to release what is done by >>> reset_control_reset(), IOW decrement triggered_count. Add the new >>> reset_control_resettable() call to do so. >>> >>> When reset_control_reset() has been called once, the counter >>> triggered_count, in the reset framework, is incremented i.e the resource >>> under the reset is in-use and the reset should not be done again. >>> reset_control_resettable() would be the way to state that the resource is >>> no longer used and, that from the caller's perspective, the reset can be >>> fired again if necessary. >>> >>> This patch will fix a usb suspend warning seen on the libretech-cc >>> related to the shared reset line. This warning was addressed by the >>> previously reverted commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared >>> reset control use") >>> >>> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com> >>> Reported-by: Jerome Brunet <jbrunet@baylibre.com> >>> --- >>> drivers/reset/core.c | 57 +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/reset.h | 1 + >>> 2 files changed, 58 insertions(+) >>> >>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >>> index 01c0c7aa835c..53653d4b55c4 100644 >>> --- a/drivers/reset/core.c >>> +++ b/drivers/reset/core.c >>> @@ -207,6 +207,19 @@ static int reset_control_array_reset(struct reset_control_array *resets) >>> return 0; >>> } >>> >>> +static int reset_control_array_resettable(struct reset_control_array *resets) >>> +{ >>> + int ret, i; >>> + >>> + for (i = 0; i < resets->num_rstcs; i++) { >>> + ret = reset_control_resettable(resets->rstc[i]); >>> + if (ret) >>> + return ret; >>> + } > This is tricky, as we can't really roll back decrementing > triggered_count in case just one of those fails. > > I think reset_control_array_resettable has to be open coded to first > check for errors and only then loop through all controls and decrement > their triggered_count.
I agree with this, it is risky to start decrementing before checking for errors. The V2 will include an open coded version of this function.
>>> + >>> + return 0; >>> +} >>> + >>> static int reset_control_array_assert(struct reset_control_array *resets) >>> { >>> int ret, i; >>> @@ -324,6 +337,50 @@ int reset_control_reset(struct reset_control *rstc) >>> } >>> EXPORT_SYMBOL_GPL(reset_control_reset); >>> >>> +/** >>> + * reset_control_resettable - decrements triggered_count of the controlled device >>> + * @rstc: reset controller > It is more important to document the purpose of the function than the > mechanism by which it is achieved. triggered_count is an implementation > detail. > > Maybe "allow shared reset line to be triggered again" or similar.
Roger that, will do in V2.
> >>> + * >>> + * On a shared reset line the actual reset pulse is only triggered once for the >>> + * lifetime of the reset_control instance, except if this function is used. >>> + * In fact, It decrements triggered_count that makes sure of not allowing >>> + * a reset if triggered_count is not null. >>> + * >>> + * This is a no-op in case triggered_count is already null i.e shared reset line >>> + * is ready to be triggered. > This is not a good idea IMHO. It would be better to document that calls > to this function must be balanced with calls to reset_control_reset, and > then throw a big warning below in case deassert_count ever dips below 0. > > Otherwise nothing stops drivers from silently decrementing other > driver's trigger count by accidentally calling this multiple times.
I do agree, accidental calls should be reported by warnings.
>>> + * >>> + * Consumers must not use reset_control_(de)assert on shared reset lines when >>> + * reset_control_reset has been used. >>> + * >>> + * If rstc is NULL it is an optional clear and the function will just >>> + * return 0. >>> + */ >>> +int reset_control_resettable(struct reset_control *rstc) >>> +{ >>> + if (!rstc) >>> + return 0; >>> + >>> + if (WARN_ON(IS_ERR(rstc))) >>> + return -EINVAL; >>> + >>> + if (reset_control_is_array(rstc)) >>> + return reset_control_array_resettable(rstc_to_array(rstc)); >>> + >>> + if (rstc->shared) { >>> + if (WARN_ON(atomic_read(&rstc->deassert_count) != 0)) >>> + return -EINVAL; >>> + >>> + if (atomic_read(&rstc->triggered_count) > 0) >>> + atomic_dec(&rstc->triggered_count); > I think this should be > > WARN_ON(atomic_dec_return(&rstc->triggered_count) < 0);
That is even better, having this warning means that the call has not
been properly used.
> > regards > Philipp
Next version of the patch will be sent soon, include everything we
have discussed.
Sincerely,
Amjad
| |