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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Apr 2021 18:11:39 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mazziesaccount@...il.com, Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-power@...rohmeurope.com, linux-arm-msm@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)