Message ID | cover.1615454845.git.matti.vaittinen@fi.rohmeurope.com |
---|---|
Headers | show |
Series | Extend regulator notification support | expand |
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.
On Thu, Mar 11, 2021 at 12:23:02PM +0200, Matti Vaittinen wrote: > + /* > + * Existing logic does not warn if over_current_protection is given as > + * a constraint but driver does not support that. I think we should > + * warn about this type of issues as it is possible someone changes The "existing logic" bit here is for a changelog, not the code - as soon as the patch is applied the comment becomes inaccurate. This also seems like a separate patch.
On Thu, Mar 11, 2021 at 12:24:29PM +0200, Matti Vaittinen wrote: > Driver name was changed in MFD cell: > https://lore.kernel.org/lkml/560b9748094392493ebf7af11b6cc558776c4fd5.1613031055.git.matti.vaittinen@fi.rohmeurope.com/ > Fix the ID table to match this. This looks unrelated to the rest of the series?
On Thu, Mar 11, 2021 at 12:21:01PM +0200, Matti Vaittinen wrote: > Extend regulator notification support > > This is an RFC series for getting feedback on extending the regulator > notification and error flag support. Initial discussion on the topic can > be found here: This looks good apart from the fairly minor comments I sent on a couple of the patches and the schema problem Rob reported.
Hi Mark, Thanks for the review(s)! On Fri, 2021-04-02 at 18:18 +0100, Mark Brown wrote: > On Thu, Mar 11, 2021 at 12:23:02PM +0200, Matti Vaittinen wrote: > > > + /* > > + * Existing logic does not warn if over_current_protection is > > given as > > + * a constraint but driver does not support that. I think we > > should > > + * warn about this type of issues as it is possible someone > > changes > > The "existing logic" bit here is for a changelog, not the code - as > soon > as the patch is applied the comment becomes inaccurate. This also > seems > like a separate patch. I don't think this patch changed the logic but kept it as it is now. Eg, for the existing over_current_protection property we still silently ignore case where property is given but driver does not support setting it. For me this sounds like fragile approach and I did handle the new properties (like detection) in a different way. Thus the comment should stay valid - and thus I didn't think this warrants a new patch. If you think we should change the logic, then we should definitely do that in separate patch. That allows revert if existing setups break everywhere. How would you like this to be? I can change the logic if you see it's worth the risk of breaking existing setups. Best Regards Matti Vaittinen
On Fri, 2021-04-02 at 18:19 +0100, Mark Brown wrote: > On Thu, Mar 11, 2021 at 12:24:29PM +0200, Matti Vaittinen wrote: > > Driver name was changed in MFD cell: > > https://lore.kernel.org/lkml/560b9748094392493ebf7af11b6cc558776c4fd5.1613031055.git.matti.vaittinen@fi.rohmeurope.com/ > > Fix the ID table to match this. > > This looks unrelated to the rest of the series? Correct. I think I mentioned that somewhere - or at least I intended to do that. Probably in the cover-letter. I included this change to the series just to avoid conflicts. Do you want me to send it separately? Best Regards Matti Vaittinen
On Fri, 2021-04-02 at 18:11 +0100, Mark Brown wrote: > On Thu, Mar 11, 2021 at 12:22:36PM +0200, Matti Vaittinen wrote: > > + 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, I do agree. I'll add a print in next version. > 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. I like the idea but don't know of such existing mechanism. The input system power-key event is closest that comes to my mind - but I don't think that would be quite right. Additionally, I am unsure what level of user-space functionality can be expected to work? Maybe the severity of configured notifications should be used to decide whether to do in- kernel handling or to alert user-space. Anyways, that is something that requires further pondering - I'd propose improving this later. Best Regards Matti Vaittinen