linux-kernel - Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master 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]

Message-ID: <20160328192629.GG2350@sirena.org.uk>
Date:	Mon, 28 Mar 2016 12:26:29 -0700
From:	Mark Brown <broonie@...nel.org>
To:	Purna Chandra Mandal <purna.mandal@...rochip.com>
Cc:	linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	Joshua Henderson <digitalpeer@...italpeer.com>
Subject: Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

On Wed, Mar 23, 2016 at 07:12:56PM +0530, Purna Chandra Mandal wrote:

> +	switch (bits_per_word) {
> +	default:
> +	case 8:

Are you sure that all bits per word settings other than those explicitly
supported should be handled in exactly the same fashion as 8 bits per
word?  That doesn't seem entirely expected.  I'd expect the default to
be to return an error.

> +static int pic32_spi_prepare_message(struct spi_master *master,
> +				     struct spi_message *msg)
> +{
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer *xfer;
> +	struct pic32_spi *pic32s;
> +
> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32s->mesg = msg;
> +	pic32_spi_disable_chip(pic32s);
> +
> +	if (!pic32_spi_debug)
> +		return 0;

This appears to be some half implemented debugging code which is never
enabled, please remove it.

> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
> +			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
> +		print_hex_dump(KERN_DEBUG, "tx_buf ",
> +			       DUMP_PREFIX_ADDRESS,
> +			       16, 1, xfer->tx_buf,
> +			       min_t(u32, xfer->len, 16), 1);
> +	}

Similarly here, the core already has extensive trace stuff in it which
you can use.

> +	/* transact by DMA mode */
> +	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
> +		if (ret) {
> +			dev_err(&spi->dev, "dma submit error\n");
> +			return ret;
> +		}
> +
> +		/* DMA issued, wait for completion */
> +		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
> +		goto out_wait_for_xfer;
> +	}

...

> +
> +out_wait_for_xfer:

Please write normal code with an if/else rather than using gotos, it's a
lot easier to follow.

> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32_spi_disable_chip(pic32s);

Do we really need tod isable the hardware after every single message?
It's normally more efficient to just leave the hardware powered until it
goes idle and unprepare_transfer_hardware() is called.

> +	/* SPI master supports only one spi-device at a time.
> +	 * So multiple spi_setup() to different devices is not allowed.
> +	 */
> +	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {

unlikely() is for fast paths, outside of them it's just noise.

> +	/* But spi_setup() can be called multiple times by same device.
> +	 * In that case stop current on-going transaction and release
> +	 * resource(s).
> +	 */
> +	if (pic32s->spi_dev == spi)
> +		pic32_spi_cleanup(spi);

This is broken, it will break in progress transfers.  setup() shouldn't
be doing anything that disrupts them, anything that can't be run when
other transfers are in progress needs to be deferred till we actually do
the transfers (or done earlier in probe).

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