linux-kernel - Re: [PATCH 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

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:   Tue, 10 Jul 2018 19:10:24 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Girish Mahadevan <girishm@...eaurora.org>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
        dianders@...omium.org, swboyd@...omium.org,
        linux-arm-msm@...r.kernel.org, sdharia@...eaurora.org,
        kramasub@...eaurora.org, boris.brezillon@...tlin.com
Subject: Re: [PATCH 2/2] spi: Introduce new driver for Qualcomm QuadSPI
 controller

On Thu, Jul 05, 2018 at 03:46:42PM -0600, Girish Mahadevan wrote:

Overall this looks pretty good, but there were a few small issues
(mostly cosmetic):

> +	/*
> +	 * Ensure that the configuration goes through by reading back
> +	 * a register from the IO space.
> +	 */
> +	mb();
> +	mstr_cfg = readl_relaxed((ctrl->base + MSTR_CONFIG));
> +	pm_runtime_put_sync(ctrl->dev);
> +	return ret;

There's no need to use _put_sync() unless you *really* need the put to
go through immediately, if you don't need it then it'll just slow things
down.

> +	wr_cnts = (rd_fifo_status & WR_CNTS_MSK) >> WR_CNTS_SHFT;
> +	fifo_rdy = (rd_fifo_status & FIFO_RDY) ? true : false;
> +
> +	if (!fifo_rdy) {
> +		dev_dbg(ctrl->dev, "%s: Spurious IRQ 0x%x",
> +			__func__, rd_fifo_status);
> +		return IRQ_NONE;
> +	}

Please just use a normal if statement, it's easier to read.

> +	wr_fifo_bytes =
> +	readl_relaxed(ctrl->base + PIO_XFER_STATUS) >> WR_FIFO_BYTES_SHFT;

Just use something like

+	wr_fifo_bytes = readl_relaxed(ctrl->base + PIO_XFER_STATUS)
+				>> WR_FIFO_BYTES_SHFT;

Again, more legible.

> +static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
> +{
> +	u32 int_status;
> +	struct qcom_qspi *ctrl = dev_id;
> +	irqreturn_t ret = IRQ_HANDLED;
> +
> +	int_status = readl_relaxed(ctrl->base + MSTR_INT_STATUS);
> +	writel_relaxed(int_status, ctrl->base + MSTR_INT_STATUS);
> +
> +	if (int_status & WR_FIFO_EMPTY)
> +		ret = pio_write(ctrl);
> +
> +	if (int_status & RESP_FIFO_RDY)
> +		ret = pio_read(ctrl);

What if both bits are set and return different statuses?

> +static bool qcom_qspi_supports_op(struct spi_mem *mem,
> +				const struct spi_mem_op *op)
> +{
> +	return true;
> +}

So we definitely support all possible ops?

> +static int qcom_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> +	return 0;
> +}

If this can be empty it's prboably better to fix the callers so that
they don't need to provide it (looking at the code it seems that this is
already the case so you can just remove it).

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