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]
Message-ID: <20240131085842.GF8551@google.com>
Date: Wed, 31 Jan 2024 08:58:42 +0000
From: Lee Jones <lee@...nel.org>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Anjelique Melendez <quic_amelende@...cinc.com>, pavel@....cz,
	thierry.reding@...il.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	agross@...nel.org, luca.weiss@...rphone.com,
	konrad.dybcio@...aro.org, 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
Subject: Re: [PATCH v8 3/7] soc: qcom: add QCOM PBS driver

Intentional generic top-post reply.

Please work quickly to resolve Bjorn's comments.

I'm being hounded over a broken LEDs tree due to the missing headerfile.

/end

On Tue, 30 Jan 2024, Bjorn Andersson wrote:

> On Thu, Dec 21, 2023 at 10:58:33AM -0800, Anjelique Melendez wrote:
> > diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
> [..]
> > +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> > +{
> > +	int ret, retries = 2000, delay = 1100;
> 
> retries and delay are not variable, please use defines instead.
> 
> > +	unsigned int val;
> > +
> > +	ret = regmap_read_poll_timeout(pbs->regmap,  pbs->base + PBS_CLIENT_SCRATCH2,
> > +					val, val & BIT(bit_pos), delay, delay * retries);
> > +
> > +	if (ret < 0) {
> > +		dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > +		ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > +		dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> > +	return 0;
> > +}
> > +
> > +/**
> > + * qcom_pbs_trigger_event() - Trigger the PBS RAM sequence
> > + * @pbs: Pointer to PBS device
> > + * @bitmap: bitmap
> > + *
> > + * This function is used to trigger the PBS RAM sequence to be
> > + * executed by the client driver.
> > + *
> > + * The PBS trigger sequence involves
> > + * 1. setting the PBS sequence bit in PBS_CLIENT_SCRATCH1
> > + * 2. Initiating the SW PBS trigger
> > + * 3. Checking the equivalent bit in PBS_CLIENT_SCRATCH2 for the
> > + *    completion of the sequence.
> > + * 4. If PBS_CLIENT_SCRATCH2 == 0xFF, the PBS sequence failed to execute
> > + *
> > + * Returns: 0 on success, < 0 on failure
> 
> Return: without the 's' is the appropriate form here.
> 
> > + */
> > +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> > +{
> > +	unsigned int val;
> > +	u16 bit_pos;
> > +	int ret;
> > +
> > +	if (!bitmap) {
> > +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> 
> No one is going to spot that hidden in the kernel log, and if someone
> sees it it does not give an indication to which client it is that's
> broken (if there are multiple clients...)
> 
> Instead do:
> 
> 	if (WARN_ON(!bitmap))
> 		return -EINVAL;
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (IS_ERR_OR_NULL(pbs))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&pbs->lock);
> > +	ret = regmap_read(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, &val);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > +		/* PBS error - clear SCRATCH2 register */
> > +		ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> > +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> > +		if (!(bitmap & BIT(bit_pos)))
> > +			continue;
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		/* Set the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > +					BIT(bit_pos), BIT(bit_pos));
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		/* Initiate the SW trigger */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_TRIG_CTL,
> > +					PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
> > +		if (ret < 0)
> > +			goto error;
> 
> In the case that this fails, you're jumping to error, which clears all
> of SCRATCH1, but you're leaving SCRATCH2 untouched.
> 
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> 
> Does it make sense to handle this error by jumping to error and trying
> to clear it once more - while leaving SCRATCH2?
> 
> Perhaps you should just ignore the errors from clearing SCRATCH1 and
> SCRATCH2? You where able to trigger the PBS and you got your ack...
> 
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> > +	}
> > +
> > +error:
> 
> We're passing "error" in the successful case as well, please name this
> "out_clear_scratch1" (or something) instead, to not confuse the reader.
> 
> > +	/* Clear all the requested bitmap */
> > +	ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1, bitmap, 0);
> > +
> > +out:
> > +	mutex_unlock(&pbs->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
> > +
> > +/**
> > + * get_pbs_client_device() - Get the PBS device used by client
> > + * @dev: Client device
> > + *
> > + * This function is used to get the PBS device that is being
> > + * used by the client.
> > + *
> > + * Returns: pbs_dev on success, ERR_PTR on failure
> 
> Return:
> 
> Regards,
> Bjorn

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ