Re: [RFC PATCH v3 3/7] regulator: IRQ based event/error notification helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux