Messages in this thread | | | Date | Fri, 2 Apr 2021 18:11:39 +0100 | From | Mark Brown <> | Subject | Re: [RFC PATCH v3 3/7] regulator: IRQ based event/error notification helpers |
| |
On Thu, Mar 11, 2021 at 12:22:36PM +0200, Matti Vaittinen wrote:
> @@ -0,0 +1,423 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 ROHM Semiconductors
Please make the entire comment a C++ one so things look more consistent.
> +static void regulator_notifier_isr_work(struct work_struct *work) > +{
> + if (d->fatal_cnt && h->retry_cnt > d->fatal_cnt) { > + if (d->die) > + ret = d->die(rid); > + else > + BUG(); > + > + /* > + * If the 'last resort' IC recovery failed we will have > + * nothing else left to do... > + */ > + BUG_ON(ret);
This isn't good... we should be trying to provide more system level handling of this, if nothing else it's quite possibly not a software bug here but rather a hardware failure. An explicit message about what happened would be more likely to be understood as a hardware failure, and something which allows handling such as initiating a system shutdown would be good as well - I'm not sure if there's any existing mechanism to plumb userspace into, or perhaps some sort of policy configurable via sysfs. That could be built on later though, I think the main thing here is that the logging should be clearer and distinguishable from a random software fault which is what BUG_ON() looks like. The backtrace and whatnot that BUG_ON() provides aren't useful here and the message isn't going to be very distinctive, some custom prints will attract more attention.
> + /* Disable IRQ if HW keeps line asserted */ > + if (d->irq_off_ms) > + disable_irq_nosync(irq); > + /* > + * IRQ seems to be for us. Let's fire correct notifiers / store error
Missing blank lines in the file.
> + * This structure is passed to map_event and renable for reporting reulator
regulator. [unhandled content-type:application/pgp-signature] | |