linux-kernel - Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk

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: Mon, 8 Apr 2024 15:10:28 +0100
From: Mark Brown <broonie@...nel.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Vaishnav Achath <vaishnav.a@...com>,
	Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
	Rob Herring <robh@...nel.org>, linux-spi@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mips@...r.kernel.org,
	Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
	Gregory CLEMENT <gregory.clement@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection
 quirk

On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:

> Use hardware ability to read the FIFO depth thanks to
> CQSPI_REG_SRAMPARTITION that is partially read-only. Keep current
> behavior identical for existing compatibles.

The behaviour is not identical here - we now unconditionally probe the
FIFO depth on all hardware, the difference with the quirk is that we
will ignore any DT property specifying the depth.

> -	if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> +	if (!(ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) &&
> +	    of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
>  		dev_err(dev, "couldn't determine fifo-depth\n");

It's not obvious from just the code that we do handle having a FIFO
depth property and detection in the detection code, at least a comment
would be good.

> +static void cqspi_controller_detect_fifo_depth(struct cqspi_st *cqspi)
> +{
> +	const struct cqspi_driver_platdata *ddata = cqspi->ddata;
> +	struct device *dev = &cqspi->pdev->dev;
> +	u32 reg, fifo_depth;
> +
> +	/*
> +	 * Bits N-1:0 are writable while bits 31:N are read as zero, with 2^N
> +	 * the FIFO depth.
> +	 */
> +	writel(U32_MAX, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> +	reg = readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> +	fifo_depth = reg + 1;
> +
> +	if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> +		cqspi->fifo_depth = fifo_depth;
> +		dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> +	} else if (fifo_depth != cqspi->fifo_depth) {
> +		dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> +			 fifo_depth, cqspi->fifo_depth);
> +	}

It's not obvious to me that we should ignore an explicitly specified
property if the quirk is present - if anything I'd more expect to see
the new warning in that case, possibly with a higher severity if we're
saying that the quirk means we're more confident that the data reported
by the hardware is reliable.  I think what I'd expect is that we always
use an explicitly specified depth (hopefully the user was specifying it
for a reason?).

Pulling all the above together can we just drop the quirk and always do
the detection, or leave the quirk as just controlling the severity with
which we log any difference between detected and explicitly configured
depths?

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