linux-kernel - Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200602122205.GF5684@sirena.org.uk>
Date:   Tue, 2 Jun 2020 13:22:05 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Sumit Semwal <sumit.semwal@...aro.org>
Cc:     agross@...nel.org, bjorn.andersson@...aro.org, lgirdwood@...il.com,
        robh+dt@...nel.org, nishakumari@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, kgunda@...eaurora.org,
        rnayak@...eaurora.org
Subject: Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling

On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:

>  static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
>  {
> -	return regulator_enable_regmap(rdev);
> +	int ret;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regulator_enable_regmap(rdev);
> +	if (ret >= 0)
> +		reg->enabled = true;

Can we not read the register we just wrote to here?

> +	/*
> +	 * The SC(short circuit) fault would trigger PBS(Portable Batch
> +	 * System) to disable regulators for protection. This would
> +	 * cause the SC_DETECT status being cleared so that it's not
> +	 * able to get the SC fault status.
> +	 * Check if the regulator is enabled in the driver but
> +	 * disabled in hardware, this means a SC fault had happened
> +	 * and SCP handling is completed by PBS.
> +	 */
> +	if (!in_sc_err) {
> +
> +		reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> +
> +		ret = regmap_read_poll_timeout(labibb_reg->regmap,
> +					reg, val,
> +					!(val & LABIBB_CONTROL_ENABLE),
> +					POLLING_SCP_DONE_INTERVAL_US,
> +					POLLING_SCP_TIMEOUT);

Why do we need a timeout here?

> +						NULL);
> +		regulator_unlock(labibb_reg->rdev);
> +	}
> +	return IRQ_HANDLED;

This returns IRQ_HANDLED even if we didn't detect an interrupt source...
Especially given the need to check to see if the regulator was turned
off by the hardware it seems like there must be some false positives.

> +	} else {
> +		ret = devm_request_threaded_irq(reg->dev,
> +						sc_irq,
> +						NULL, labibb_sc_err_handler,
> +						IRQF_ONESHOT,
> +						"sc-err", reg);

This looks like we're requesting the interrupt before we register the
regulator which means the interrupt might fire without the regulator
being there.  The order of registration should be reversed.

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