linux-kernel - Re: [PATCHv4 2/3] drivers: spi: Add qspi flash 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]

Message-ID: <20130718104233.GG22506@sirena.org.uk>
Date:	Thu, 18 Jul 2013 11:42:33 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Sourav Poddar <sourav.poddar@...com>
Cc:	spi-devel-general@...ts.sourceforge.net, grant.likely@...aro.org,
	balbi@...com, rnayak@...com, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

On Thu, Jul 18, 2013 at 03:31:26PM +0530, Sourav Poddar wrote:

> QSPI is a kind of spi module that allows single,
> dual and quad read access to external spi devices. The module
> has a memory mapped interface which provide direct interface
> for accessing data form external spi devices.

Have you seen the ongoing thread about SPI buses with extra data lines?
How does this driver fit in with that?

> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_OCTEON)		+= spi-octeon.o
>  obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
>  obj-$(CONFIG_SPI_OMAP_100K)		+= spi-omap-100k.o
>  obj-$(CONFIG_SPI_OMAP24XX)		+= spi-omap2-mcspi.o
> +obj-$(CONFIG_QSPI_DRA7xxx)              += spi-ti-qspi.o
>  obj-$(CONFIG_SPI_ORION)			+= spi-orion.o
>  obj-$(CONFIG_SPI_PL022)			+= spi-pl022.o
>  obj-$(CONFIG_SPI_PPC4xx)		+= spi-ppc4xx.o

Please use SPI_ like the other drivers.

> +static int ti_qspi_prepare_xfer(struct spi_master *master)
> +{
> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(qspi->dev);
> +	if (ret < 0) {
> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

This is a very common pattern, it should probably be factored out into
the core, probably not even as ops but rather as an actual feature.

> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
> +		qspi->cmd |= QSPI_WC_CMD_INT_EN;
> +
> +		ret = qspi_transfer_msg(qspi, t);
> +		if (ret) {
> +			dev_dbg(qspi->dev, "transfer message failed\n");
> +			return -EINVAL;
> +		}
> +
> +		m->actual_length += t->len;
> +
> +		if (list_is_last(&t->transfer_list, &m->transfers))
> +			goto out;
> +	}

The use of list_is_last() here is *realy* strange - what's going on
there?

> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
> +{
> +	struct ti_qspi *qspi = dev_id;
> +	u16 mask, stat;
> +
> +	irqreturn_t ret = IRQ_HANDLED;
> +
> +	spin_lock(&qspi->lock);
> +
> +	stat = ti_qspi_readl(qspi, QSPI_SPI_STATUS_REG);
> +	mask = ti_qspi_readl(qspi, QSPI_INTR_ENABLE_SET_REG);
> +
> +	if (stat && mask)
> +		ret = IRQ_WAKE_THREAD;
> +
> +	spin_unlock(&qspi->lock);
> +
> +	return ret;

According to the above code we might interrupt for masked events...
that's a bit worrying isn't it?

> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> +			dev_name(&pdev->dev), qspi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
> +				irq);
> +		goto free_master;
> +	}

Standard question about devm_request_threaded_irq() - how can we be
certain it's safe to use during removal?

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