linux-kernel - Re: [PATCH v4 3/7] soc: qcom: add QCOM PBS driver

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:   Wed, 30 Aug 2023 20:20:19 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Anjelique Melendez <quic_amelende@...cinc.com>, pavel@....cz,
        lee@...nel.org, thierry.reding@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        agross@...nel.org, andersson@...nel.org
Cc:     luca.weiss@...rphone.com, u.kleine-koenig@...gutronix.de,
        quic_subbaram@...cinc.com, quic_gurus@...cinc.com,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-pwm@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH v4 3/7] soc: qcom: add QCOM PBS driver

On 30.08.2023 20:05, Anjelique Melendez wrote:
> Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
> driver supports configuring software PBS trigger events through PBS RAM
> on Qualcomm Technologies, Inc (QTI) PMICs.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.com>
> ---
[...]

> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
I've seen your answer in v2, but I'm still not convinced about
two things:

1. why are you using bulk APIs with count=1 instead of just
   regmap_write/read?
2. do we expect the accesses to ever fail (realistically), if not
   we don't have to care about the retval and skip the conditional
   error message (1-2 cycles less per invocation)

You insisted on keeping the error messages, but firstly you'll soon
get an angry response from Krzysztof saying register accesses can't
fail (hence making checking the retval useless) and secondly I think
spmi core already spits out some errors on disallowed r/w

If you agree access failures are very edge cases, you can simply
convert all r/w ops to regmap_read/write/modify_bits and pass
pbs->base + reg

> +{
> +	int ret;
> +
> +	address += pbs->base;
> +	ret = regmap_bulk_read(pbs->regmap, address, val, 1);
> +	if (ret)
> +		dev_err(pbs->dev, "Failed to read address=%#x sid=%#x ret=%d\n",
> +			address, to_spmi_device(pbs->dev->parent)->usid, ret);
> +
> +	return ret;
> +}
[...]

> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> +	int ret, retries = 2000, delay = 1000;
> +	u8 val;
> +
> +	while (retries--) {
> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +			/* PBS error - clear SCRATCH2 register */
> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +			if (ret < 0)
> +				return ret;
> +
> +			dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> +			return -EINVAL;
> +		}
> +
> +		if (val & BIT(bit_pos)) {
> +			dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> +			return 0;
> +		}
> +
> +		usleep_range(delay, delay + 100);
> +	}
Since the SCRATCH2_ERROR path exits the loop, this can simply be
made into:

regmap_read_poll_timeout

if (SCARTCH2_ERROR)
  do something
  return einval

return etimedout

[...]

> +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> +{
> +	u8 val;
> +	u16 bit_pos;
> +	int ret;
Reverse-Christmas-tree?

> +
> +	if (!bitmap) {
> +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> +		return -EINVAL;
> +	}
> +
> +	if (IS_ERR_OR_NULL(pbs))
> +		return -EINVAL;
> +
> +	mutex_lock(&pbs->lock);
> +	ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +		/* PBS error - clear SCRATCH2 register */
> +		ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +		if (ret < 0)
> +			goto out;
> +	}
Probably deserves an error message?

> +
> +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> +		if (bitmap & BIT(bit_pos)) {
if (!(bitmap & BIT(bit_pos))
    continue

would save you a level of indentation

Konrad