Re: [PATCH v6 3/8] regulator: IRQ based event/error notification helpers

[Matti Vaittinen]

Hello Andy, All.

On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > Provide helper function for IC's implementing regulator
> > notifications
> > when an IRQ fires. The helper also works for IRQs which can not be
> > acked.
> > Helper can be set to disable the IRQ at handler and then re-
> > enabling it
> > on delayed work later. The helper also adds
> > regulator_get_error_flags()
> > errors in cache for the duration of IRQ disabling.
> 
> Thanks for an update, my comments below. After addressing them, feel
> free to add
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > 
> >  static int _regulator_get_error_flags(struct regulator_dev *rdev,
> >                                         unsigned int *flags)
> >  {
> > -       int ret;
> > +       int ret, tmpret;
> > 
> >         regulator_lock(rdev);
> > 
> > +       ret = rdev_get_cached_err_flags(rdev);
> > +
> >         /* sanity check */
> > -       if (!rdev->desc->ops->get_error_flags) {
> > +       if (rdev->desc->ops->get_error_flags) {
> > +               tmpret = rdev->desc->ops->get_error_flags(rdev,
> > flags);
> > +               if (tmpret > 0)
> > +                       ret |= tmpret;
> 
> Oh, I don't like this. Easy fix is to rename ret (okay, it's been
> used
> elsewhere, so adding then) to something meaningful, like error_flags,
> then you can easily understand that value should be positive and
> error
> codes are negative.

No wonder if this looks hairy. I think I have got this plain wrong. The
rdev_get_cached_err_flags() is not updating the flags. Looks like just
plain mistake from my side. I think I've mixed the returning flags via
parameter and return value. This must be fixed. Well spotted.


> + */
> > +void *devm_regulator_irq_helper(struct device *dev,
> > +                               const struct regulator_irq_desc *d,
> > int irq,
> > +                               int irq_flags, int common_errs,
> > +                               int *per_rdev_errs,
> > +                               struct regulator_dev **rdev, int
> > rdev_amount)
> 
> I didn't get why you need the ** pointer instead of plain pointer.

We have an array of pointers. And we give a pointer to the first
pointer. Maybe it's the lack of coffee but I don't see why a single
pointer would be correct? rdev structures are not in contagious memory,
pointers to rdevs are. So we need address of first pointer, right?
+#include <linux/device.h>


> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of_irq.h>
> 
> Not sure how this header is used. I haven't found any direct users of
> it. Perhaps you wanted interrupt.h?

Thanks. I think this specific header may be a leftover from first draft
where I thought I'll use named IRQs. The header was for
 of_irq_get_byname(). That ended up as a mess for everything else but
platform devices :) I'll check the headers, thanks.

> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> 
> + Blank line? I would separate group of generic headers with
> particular to the subsystem

I don't see this being used in regulator subsystem - and to tell the
truth, I don't really see the value.

> > +#include <linux/regulator/driver.h>

...

> > +
> > +reread:
> > +       if (d->fatal_cnt && h->retry_cnt > d->fatal_cnt) {
> > +               if (d->die)
> > +                       ret = d->die(rid);
> > +               else
> > +                       die_loudly("Regulator HW failure? - no IC
> > recovery");
> > +
> > +               /*
> > +                * If the 'last resort' IC recovery failed we will
> > have
> > +                * nothing else left to do...
> > +                */
> > +               if (ret)
> > +                       die_loudly("Regulator HW failure? - IC
> > recovery failed");
> 
> Looking at the above code this will be executed if and only if
> d->die() is defined, correct?
> In that case, why not
> 
> if (d->die) {
>   ret = ...
>   if (ret)
>    rdev_die_loudly(...);
> } else
>    rdev_die_loudly(...);
> 
> ?

I think this should simply be:

if (!d->die)
	die_loudly("Regulator HW failure? - no IC recovery");

ret = d->die(rdev);
if (ret)
	die_loudly(...);

...

> > +static void init_rdev_errors(struct regulator_irq *h)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < h->rdata.num_states; i++) {
> > +               if (h->rdata.states[i].possible_errs)
> > +                       /* Can we trust writing this boolean is
> > atomic? */
> 
> No. boolean is a compiler / platform specific and it may potentially
> be written in a non-atomic way.

Hmm.. I don't think this really is a problem here. We only use the
use_cached_err for true/false evaluation - and if error getting api is
called after the boolean is changed - then cached error is used, if
before, then it is not used. Even if the value of the boolean was read
in the middle of writing it, it will still evaluate either true or
false - there is no 'maybe' state :)

My point, I guess we can do the change without locking here. Please
correct me if I am wrong. I'll just drop this comment.

> 
> re-enable / reenable
> 
> > + *             added to status. If that is the case it may be
> > desirable to
> > + *             return REGULATOR_ERROR_CLEARED and not
> > REGULATOR_ERROR_ON to
> > + *             allow IRQ fire again and to generate notifications
> > also for
> > + *             the new issues.
> > + *
> > + * This structure is passed to map_event and renable for reporting
> > regulator
> 
> Ditto.

the "renable" is referring to the callback function pointer which is
named "renable".


Best Regards
-- Matti Vaittinen