From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-power@fi.rohmeurope.com" <linux-power@fi.rohmeurope.com>,
"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
"linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
Date: Wed, 07 Apr 2021 08:02:04 +0300 [thread overview]
Message-ID: <55397166b1c4107efc2a013635f63af142d9b187.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <CAHp75VeoTVNDemV0qRA4BTVqOVfyR9UKGWhHgfeat8zVVGcu_Q@mail.gmail.com>
Morning Andy,
Thanks for the review! By the way, is it me or did your mail-client
spill this out using HTML?
On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> On Tuesday, April 6, 2021, Matti Vaittinen <
> matti.vaittinen@fi.rohmeurope.com> wrote:
> > +static void die_loudly(const char *msg)
> > +{
> > + pr_emerg(msg);
>
> Oh là là, besides build bot complaints, this has serious security
> implications. Never do like this.
I'm not even trying to claim that was correct. And I did send a fixup -
sorry for this. I don't intend to do this again.
Now, when this is said - If you have a minute, please educate me.
Assuming we know all the callers and that all the callers use this as
die_loudly("foobarfoo\n");
- what is the exploit mechanism?
> > + BUG();
> > +}
> > +
> > +/**
> > + * regulator_irq_helper - register IRQ based regulator event/error
> > notifier
> > + *
> > + * @dev: device to which lifetime the helper's
> > lifetime is
> > + * bound.
> > + * @d: IRQ helper descriptor.
> > + * @irq: IRQ used to inform events/errors to be
> > notified.
> > + * @irq_flags: Extra IRQ flags to be OR's with the default
> > IRQF_ONESHOT
> > + * when requesting the (threaded) irq.
> > + * @common_errs: Errors which can be flagged by this IRQ for
> > all rdevs.
> > + * When IRQ is re-enabled these errors will be
> > cleared
> > + * from all associated regulators
> > + * @per_rdev_errs: Optional error flag array describing errors
> > specific
> > + * for only some of the regulators. These
> > errors will be
> > + * or'ed with common erros. If this is given
> > the array
> > + * should contain rdev_amount flags. Can be
> > set to NULL
> > + * if there is no regulator specific error
> > flags for this
> > + * IRQ.
> > + * @rdev: Array of regulators associated with this
> > IRQ.
> > + * @rdev_amount: Amount of regulators associated wit this
> > IRQ.
> > + */
> > +void *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)
> > +{
> > + struct regulator_irq *h;
> > + int ret;
> > +
> > + if (!rdev_amount || !d || !d->map_event || !d->name)
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (irq <= 0) {
> > + dev_err(dev, "No IRQ\n");
> > + return ERR_PTR(-EINVAL);
>
> Why shadowing error code? Negative IRQ is anything but “no IRQ”.
This was a good point. The irq is passed here as parameter. From this
function's perspective the negative irq is invalid parameter - we don't
know how the caller has obtained it. Print could show the value
contained in irq though.
Now that you pointed this out I am unsure if this check is needed here.
If we check it, then I still think we should report -EINVAL for invalid
parameter. Other option is to just call the request_threaded_irq() -
log the IRQ request failure and return what request_threaded_irq()
returns. Do you think that would make sense?
> > +
> > +/**
> > + * regulator_irq_helper_cancel - drop IRQ based regulator
> > event/error notifier
> > + *
> > + * @handle: Pointer to handle returned by a successful
> > call to
> > + * regulator_irq_helper(). Will be NULLed upon
> > return.
> > + *
> > + * The associated IRQ is released and work is cancelled when the
> > function
> > + * returns.
> > + */
> > +void regulator_irq_helper_cancel(void **handle)
> > +{
> > + if (handle && *handle) {
>
> Can handle ever be NULL here ? (Yes, I understand that you export
> this)
To tell the truth - I am not sure. I *guess* that if we allow this to
be NULL, then one *could* implement a driver for IC where IRQs are
optional, in a way that when IRQs are supported the pointer to handle
is valid, when IRQs aren't supported the pointer is NULL. (Why) do you
think we should skip the check?
>
> > + struct regulator_irq *h = *handle;
> > +
> > + free_irq(h->irq, h);
> > + if (h->desc.irq_off_ms)
> > + cancel_delayed_work_sync(&h->isr_work);
> > +
> > + h = NULL;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(regulator_irq_helper_cancel);
> > +
> > +static void regulator_irq_helper_drop(struct device *dev, void
> > *res)
> > +{
> > + regulator_irq_helper_cancel(res);
> > +}
> > +
> > +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)
> > +{
> > + void **ptr;
> > +
> > + ptr = devres_alloc(regulator_irq_helper_drop, sizeof(*ptr),
> > GFP_KERNEL);
> > + if (!ptr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + *ptr = regulator_irq_helper(dev, d, irq, irq_flags,
> > common_errs,
> > + per_rdev_errs, rdev,
> > rdev_amount);
> > +
> > + if (IS_ERR(*ptr))
> > + devres_free(ptr);
> > + else
> > + devres_add(dev, ptr);
> > +
> > + return *ptr;
>
> Why not to use devm_add_action{_or_reset}()?
I just followed the same approach that has been used in other regulator
functions. (drivers/regulator/devres.c)
OTOH, the devm_add_action makes this little bit simpler so I'll convert
to use it.
Mark, do you have a reason of not using devm_add_action() in devres.c?
Should devm_add_action() be used in some other functions there? And
should this be moved to devres.c?
Best Regards
Matti Vaittinen
next prev parent reply other threads:[~2021-04-07 5:02 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-06 7:12 [PATCH v4 0/7] Extend regulator notification support Matti Vaittinen
2021-04-06 7:12 ` [PATCH v4 1/7] dt_bindings: Add protection limit properties Matti Vaittinen
2021-04-06 7:12 ` [PATCH v4 2/7] regulator: add warning flags Matti Vaittinen
2021-04-06 7:13 ` [PATCH v4 3/7] regulator: IRQ based event/error notification helpers Matti Vaittinen
2021-04-06 10:27 ` kernel test robot
2021-04-06 10:57 ` Matti Vaittinen
2021-04-06 12:08 ` kernel test robot
[not found] ` <CAHp75VeoTVNDemV0qRA4BTVqOVfyR9UKGWhHgfeat8zVVGcu_Q@mail.gmail.com>
2021-04-07 5:02 ` Matti Vaittinen [this message]
2021-04-07 9:10 ` Andy Shevchenko
2021-04-07 9:49 ` Vaittinen, Matti
2021-04-07 12:50 ` Andy Shevchenko
2021-04-09 3:20 ` Kees Cook
2021-04-09 7:08 ` Vaittinen, Matti
2021-04-12 12:24 ` Matti Vaittinen
2021-04-12 13:09 ` Mark Brown
2021-04-06 7:13 ` [PATCH v4 4/7] regulator: add property parsing and callbacks to set protection limits Matti Vaittinen
2021-04-06 7:14 ` [PATCH v4 5/7] dt-bindings: regulator: bd9576 add FET ON-resistance for OCW Matti Vaittinen
2021-04-06 7:14 ` [PATCH v4 6/7] regulator: bd9576: Support error reporting Matti Vaittinen
2021-04-06 7:16 ` [PATCH v4 7/7] regulator: bd9576: Fix the driver name in id table Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55397166b1c4107efc2a013635f63af142d9b187.camel@fi.rohmeurope.com \
--to=matti.vaittinen@fi.rohmeurope.com \
--cc=agross@kernel.org \
--cc=andy.shevchenko@gmail.com \
--cc=bjorn.andersson@linaro.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-power@fi.rohmeurope.com \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).