Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Mon, Apr 12, 2021 at 03:24:16PM +0300, Matti Vaittinen wrote: > Maybe this 'hardware protection, in-kernel, emergency HW saving > shutdown' - logic, should be pulled out of thermal_core.c (or at least > exported) for (other parts like) the regulators to use? That sounds sensible. signature.asc Description: PGP signature
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Fri, 2021-04-09 at 10:08 +0300, Matti Vaittinen wrote: > On Thu, 2021-04-08 at 20:20 -0700, Kees Cook wrote: > > On Wed, Apr 07, 2021 at 03:50:15PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti > > > wrote: > > > > On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote: > > > > > On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen > > > > > wrote: > > > > > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote: > > > > > > > On Tuesday, April 6, 2021, Matti Vaittinen < > > > > > > > matti.vaitti...@fi.rohmeurope.com> wrote: > > > > > > > > + BUG(); > > > > > > > > +} > > > > This, though, are you sure you want to use BUG()? Linus gets upset > > about > > such things: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on > > > > I see. I am unsure of what would be the best action in the regulator > case we are handling here. To give the context, we assume here a > situation where power has gone out of regulation and the hardware is > probably failing. First countermeasure to protect what is left of HW > is > to shut-down the failing regulator. BUG() was called here as a last > resort if shutting the power via regulator interface was not > implemented or working. > > Eg, we try to take what ever last measure we can to minimize the HW > damage - and BUG() was used for this in the qcom driver where I stole > the idea. Judging the comment related to BUG() in asm-generic/bug.h > > /* > * Don't use BUG() or BUG_ON() unless there's really no way out; one > > * example might be detecting data structure corruption in the middle > * > of an operation that can't be backed out of. If the (sub)system > * can > somehow continue operating, perhaps with reduced functionality, > * it's > probably not BUG-worthy. > * > * If you're tempted to BUG(), think > again: is completely giving up > * really the *only* solution? There > are usually better options, where > * users don't need to reboot ASAP and > can mostly shut down cleanly. > */ > https://elixir.bootlin.com/linux/v5.12-rc6/source/include/asm-generic/bug.h#L55 > > this really might be valid use-case. > > To me the real question is what happens after the BUG() - and if > there > is any generic handling or if it is platform/board specific? Does it > actually have any chance to save the HW? > > Mark already pointed that we might need to figure a way to punt a > "failing event" to the user-space to initiate better "safety > shutdown". > Such event does not currently exist so I think the main use-case here > is to do logging and potentially prevent enabling any further actions > in the failing HW. > > So - any better suggestions? > Maybe we should take same approach as is taken in thermal_core? Quoting the thermal documentation: "On an event of critical trip temperature crossing. Thermal framework allows the system to shutdown gracefully by calling orderly_poweroff(). In the event of a failure of orderly_poweroff() to shut down the system we are in danger of keeping the system alive at undesirably high temperatures. To mitigate this high risk scenario we program a work queue to fire after a pre-determined number of seconds to start an emergency shutdown of the device using the kernel_power_off() function. In case kernel_power_off() fails then finally emergency_restart() is called in the worst case." Maybe this 'hardware protection, in-kernel, emergency HW saving shutdown' - logic, should be pulled out of thermal_core.c (or at least exported) for (other parts like) the regulators to use? I don't like the idea relying in the user-space to be in shape it can handle the situation. I may be mistaken but I think a quick action might be required. Hence the in-kernel handling does not sound so bad to me. I am open to all education and suggestions. Meanwhile I am planning to just convert the BUG() to WARN(). I don't claim I know how BUG() is implemented on each platform - but my understanding is that it does not guarantee any power to be cut but just halts the calling process(?). I guess this does not guarantee what happens next - maybe it even keeps the power enabled and end up just deadlocking the system by reserved locks? I think thermal guys have been pondering this scenario for severe temperature protection shutdown so I would like to hear your opinions. Best Regards Matti Vaittinen
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Thu, 2021-04-08 at 20:20 -0700, Kees Cook wrote: > On Wed, Apr 07, 2021 at 03:50:15PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti > > wrote: > > > On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen > > > > wrote: > > > > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote: > > > > > > On Tuesday, April 6, 2021, Matti Vaittinen < > > > > > > matti.vaitti...@fi.rohmeurope.com> wrote: > > > > > > > + BUG(); > > > > > > > +} > > This, though, are you sure you want to use BUG()? Linus gets upset > about > such things: > https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on > I see. I am unsure of what would be the best action in the regulator case we are handling here. To give the context, we assume here a situation where power has gone out of regulation and the hardware is probably failing. First countermeasure to protect what is left of HW is to shut-down the failing regulator. BUG() was called here as a last resort if shutting the power via regulator interface was not implemented or working. Eg, we try to take what ever last measure we can to minimize the HW damage - and BUG() was used for this in the qcom driver where I stole the idea. Judging the comment related to BUG() in asm-generic/bug.h /* * Don't use BUG() or BUG_ON() unless there's really no way out; one * example might be detecting data structure corruption in the middle * of an operation that can't be backed out of. If the (sub)system * can somehow continue operating, perhaps with reduced functionality, * it's probably not BUG-worthy. * * If you're tempted to BUG(), think again: is completely giving up * really the *only* solution? There are usually better options, where * users don't need to reboot ASAP and can mostly shut down cleanly. */ https://elixir.bootlin.com/linux/v5.12-rc6/source/include/asm-generic/bug.h#L55 this really might be valid use-case. To me the real question is what happens after the BUG() - and if there is any generic handling or if it is platform/board specific? Does it actually have any chance to save the HW? Mark already pointed that we might need to figure a way to punt a "failing event" to the user-space to initiate better "safety shutdown". Such event does not currently exist so I think the main use-case here is to do logging and potentially prevent enabling any further actions in the failing HW. So - any better suggestions? Best Regards Matti Vaittinen
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Wed, Apr 07, 2021 at 03:50:15PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti
> wrote:
> > On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
> > > wrote:
> > > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > > > > On Tuesday, April 6, 2021, Matti Vaittinen <
> > > > > matti.vaitti...@fi.rohmeurope.com> wrote:
>
> Kees, there are two non-security guys discussing potential security
> matters. Perhaps you may shed a light on this and tell which of our
> stuff is risky and which is not and your recommendations on it.
Hi!
> > > > > > + 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?
I may not be following the thread exactly, here, but normally the issue
is just one of robustness and code maintainability. You can't be sure all
future callers will always pass in a const string, so better to always do:
pr_whatever("%s\n", string_var);
> > > Not a security guy, but my understanding is that this code may be
> > > used
> > > as a gadget in ROP technique of attacks.
The primary concern is with giving an attacker control over a format
string (which can be used to expose kernel memory). It used to be much
more serious when the kernel still implemented %n, which would turn such
things into a potential memory _overwrite_. We removed %n a long time
ago now. :)
> > Thanks Andy. It'd be interesting to learn more details as I am not a
> > security expert either :)
> >
> > > In that case msg can be anything. On top of that, somebody may
> > > mistakenly (inadvertently) put the code that allows user controller
> > > input to go to this path.
> >
> > Yes. This is a good reason to not to do this - but I was interested in
> > knowing if there is a potential risk even if:
> >
> > > > all the callers use this
> > > > as
> > > >
> > > > die_loudly("foobarfoo\n");
>
> I don't see direct issues, only indirect ones, for example, if by some
> reason the memory of this message appears writable. So, whoever
> controls the format string of printf() controls a lot. That's why it's
> preferable to spell out exact intentions in the explicit format
> string.
Right.
> > > > > > + BUG();
> > > > > > +}
This, though, are you sure you want to use BUG()? Linus gets upset about
such things:
https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
--
Kees Cook
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti
wrote:
> On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote:
> > On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
> > wrote:
> > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > > > On Tuesday, April 6, 2021, Matti Vaittinen <
> > > > matti.vaitti...@fi.rohmeurope.com> wrote:
Kees, there are two non-security guys discussing potential security
matters. Perhaps you may shed a light on this and tell which of our
stuff is risky and which is not and your recommendations on it.
> > > > > + 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?
> >
> > Not a security guy, but my understanding is that this code may be
> > used
> > as a gadget in ROP technique of attacks.
>
> Thanks Andy. It'd be interesting to learn more details as I am not a
> security expert either :)
>
> > In that case msg can be anything. On top of that, somebody may
> > mistakenly (inadvertently) put the code that allows user controller
> > input to go to this path.
>
> Yes. This is a good reason to not to do this - but I was interested in
> knowing if there is a potential risk even if:
>
> > > all the callers use this
> > > as
> > >
> > > die_loudly("foobarfoo\n");
I don't see direct issues, only indirect ones, for example, if by some
reason the memory of this message appears writable. So, whoever
controls the format string of printf() controls a lot. That's why it's
preferable to spell out exact intentions in the explicit format
string.
> > And last but not least, that some newbies might copy'n'paste bad
> > examples where they will expose security breach.
>
> Yes yes. As I said, I am not trying to say it is Ok. I was just
> wondering what are the risks if users of the print function were known.
>
> > With the modern world of Spectre, rowhammer, and other side channel
> > attacks I may believe that one may exhaust the regulator for getting
> > advantage on an attack vector.
> >
> > But again, not a security guy here.
>
> Thanks anyways :)
> > > > > + BUG();
> > > > > +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
Hello Andy,
On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
> wrote:
> > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > > On Tuesday, April 6, 2021, Matti Vaittinen <
> > > matti.vaitti...@fi.rohmeurope.com> wrote:
>
> ...
>
> > > > + 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?
>
> Not a security guy, but my understanding is that this code may be
> used
> as a gadget in ROP technique of attacks.
Thanks Andy. It'd be interesting to learn more details as I am not a
security expert either :)
> In that case msg can be anything. On top of that, somebody may
> mistakenly (inadvertently) put the code that allows user controller
> input to go to this path.
Yes. This is a good reason to not to do this - but I was interested in
knowing if there is a potential risk even if:
> > all the callers use this
> > as
> >
> > die_loudly("foobarfoo\n");
> And last but not least, that some newbies might copy'n'paste bad
> examples where they will expose security breach.
Yes yes. As I said, I am not trying to say it is Ok. I was just
wondering what are the risks if users of the print function were known.
> With the modern world of Spectre, rowhammer, and other side channel
> attacks I may believe that one may exhaust the regulator for getting
> advantage on an attack vector.
>
> But again, not a security guy here.
Thanks anyways :)
>
> > > > + BUG();
> > > > +}
> > > > +
>
> ...
>
> > > > errors will be
> > > > + * or'ed with common erros. If this is
> > > > given
>
> errors ?
Thanks. I didn't first spot the typo even though you pointed it to me.
Luckily my evolution has occasional problems when communicating with
the mail server. I had enough time to hit the cancel before sending out
a message where I wondered how I should clarify this :]
> ...
>
> > > > + 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?
>
> Why is the parameter signed type then?
> Shouldn't the caller take care of it?
>
> Otherwise, what is the difference between passing negative IRQ to
> request_irq() call?
> As you said, you shouldn't make assumptions about what caller meant
> by this.
>
> So, I would simply drop the check (from easiness of the code
> perspective).
Yep. I was going to drop the check. Good point. Thanks.
I'll send v6 shortly to address the issues you spotted Andy. Thanks.
>
> ...
>
> > > > +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?
>
> Just my guts feeling. I don't remember that I ever saw checks like
> this for indirect pointers.
> Of course it doesn't mean there are no such checks present or may be
> present.
I think I'll keep the check unless there is some reason why it should
be omitted.
> > > 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?
>
> I think the reason for this is as simple as a historical one,
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
wrote:
>
> Morning Andy,
>
> Thanks for the review! By the way, is it me or did your mail-client
> spill this out using HTML?
It's Gmail from my mobile phone, sorry for that. We have to blame
Google that they don't think through.
> On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > On Tuesday, April 6, 2021, Matti Vaittinen <
> > matti.vaitti...@fi.rohmeurope.com> wrote:
...
> > > + 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?
Not a security guy, but my understanding is that this code may be used
as a gadget in ROP technique of attacks.
In that case msg can be anything. On top of that, somebody may
mistakenly (inadvertently) put the code that allows user controller
input to go to this path.
And last but not least, that some newbies might copy'n'paste bad
examples where they will expose security breach.
With the modern world of Spectre, rowhammer, and other side channel
attacks I may believe that one may exhaust the regulator for getting
advantage on an attack vector.
But again, not a security guy here.
> > > + BUG();
> > > +}
> > > +
...
> > > errors will be
> > > + * or'ed with common erros. If this is given
errors ?
...
> > > + 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?
Why is the parameter signed type then?
Shouldn't the caller take care of it?
Otherwise, what is the difference between passing negative IRQ to
request_irq() call?
As you said, you shouldn't make assumptions about what caller meant by this.
So, I would simply drop the check (from easiness of the code perspective).
...
> > > +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?
Just my guts feeling. I don't remember that I ever saw checks like
this for indirect pointers.
Of course it doesn't mean there are no such checks present or may be present.
...
> > 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?
I think the reason for this is as simple as a historical one, i.e.
there was no such API that time.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
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.vaitti...@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(>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,
> > +
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
Hi Matti,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.12-rc6]
[also build test ERROR on next-20210401]
[cannot apply to regulator/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Matti-Vaittinen/Extend-regulator-notification-support/20210406-151758
base:e49d033bddf5b565044e2abe4241353959bc9120
config: x86_64-randconfig-a014-20210406 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
a46f59a747a7273cc439efaf3b4f98d8b63d2f20)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
#
https://github.com/0day-ci/linux/commit/50f89bb3fbe5e4a21c48a9ca64065f3f3a668e56
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Matti-Vaittinen/Extend-regulator-notification-support/20210406-151758
git checkout 50f89bb3fbe5e4a21c48a9ca64065f3f3a668e56
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/regulator/irq_helpers.c:46:11: error: expected ')'
pr_emerg(msg);
^
drivers/regulator/irq_helpers.c:46:2: note: to match this '('
pr_emerg(msg);
^
include/linux/printk.h:313:8: note: expanded from macro 'pr_emerg'
printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
^
1 error generated.
vim +46 drivers/regulator/irq_helpers.c
43
44 static void die_loudly(const char *msg)
45 {
> 46 pr_emerg(msg);
47 BUG();
48 }
49
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Tue, 2021-04-06 at 18:27 +0800, kernel test robot wrote:
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/kernel.h:16,
> from arch/x86/include/asm/percpu.h:27,
> from arch/x86/include/asm/current.h:6,
> from include/linux/sched.h:12,
> from include/linux/ratelimit.h:6,
> from include/linux/dev_printk.h:16,
> from include/linux/device.h:15,
> from drivers/regulator/irq_helpers.c:10:
>drivers/regulator/irq_helpers.c: In function 'die_loudly':
> > > drivers/regulator/irq_helpers.c:46:11: error: expected ')' before
> > > 'msg'
> 46 | pr_emerg(msg);
> | ^~~
>include/linux/printk.h:301:21: note: in definition of macro
> 'pr_fmt'
> 301 | #define pr_fmt(fmt) fmt
> | ^~~
>drivers/regulator/irq_helpers.c:46:2: note: in expansion of macro
> 'pr_emerg'
> 46 | pr_emerg(msg);
> | ^~~~
Ouch. Feeling stupid now.
I am sorry for the hassle folks. I'll fix this ASAP and resend.
>
> vim +46 drivers/regulator/irq_helpers.c
>
> 43
> 44static void die_loudly(const char *msg)
> 45{
> > 46pr_emerg(msg);
> 47BUG();
> 48}
> 49
Best Regards
Matti Vaittinen
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
Hi Matti,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.12-rc6]
[also build test ERROR on next-20210401]
[cannot apply to regulator/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Matti-Vaittinen/Extend-regulator-notification-support/20210406-151758
base:e49d033bddf5b565044e2abe4241353959bc9120
config: i386-randconfig-m021-20210406 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/50f89bb3fbe5e4a21c48a9ca64065f3f3a668e56
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Matti-Vaittinen/Extend-regulator-notification-support/20210406-151758
git checkout 50f89bb3fbe5e4a21c48a9ca64065f3f3a668e56
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:16,
from arch/x86/include/asm/percpu.h:27,
from arch/x86/include/asm/current.h:6,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from drivers/regulator/irq_helpers.c:10:
drivers/regulator/irq_helpers.c: In function 'die_loudly':
>> drivers/regulator/irq_helpers.c:46:11: error: expected ')' before 'msg'
46 | pr_emerg(msg);
| ^~~
include/linux/printk.h:301:21: note: in definition of macro 'pr_fmt'
301 | #define pr_fmt(fmt) fmt
| ^~~
drivers/regulator/irq_helpers.c:46:2: note: in expansion of macro 'pr_emerg'
46 | pr_emerg(msg);
| ^~~~
vim +46 drivers/regulator/irq_helpers.c
43
44 static void die_loudly(const char *msg)
45 {
> 46 pr_emerg(msg);
47 BUG();
48 }
49
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
