linux-kernel - Re: [PATCH v2 2/2] spi: pic32-sqi: add SPI driver for PIC32 SQI 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:	Thu, 14 Apr 2016 06:55:46 +0100
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
Subject: Re: [PATCH v2 2/2] spi: pic32-sqi: add SPI driver for PIC32 SQI
 controller.

On Wed, Apr 13, 2016 at 06:52:58PM +0530, Purna Chandra Mandal wrote:

> +	enable = readl(sqi->regs + PESQI_INT_ENABLE_REG);
> +	status = readl(sqi->regs + PESQI_INT_STAT_REG);
> +	if (!status)
> +		return IRQ_NONE;
> +

For robustness the check should be if there was anything handled, not if
there was anything set.

> +static dma_addr_t pic32_sqi_map_transfer(struct pic32_sqi *sqi,
> +					 struct spi_transfer *transfer)
> +{
> +	struct device *dev = &sqi->master->dev;

Don't open code DMA mapping of the buffers, use the core support.

> +	reg = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sqi->regs = devm_ioremap_resource(&pdev->dev, reg);
> +	if (!sqi->regs) {
> +		dev_err(&pdev->dev, "mem map failed\n");

devm_ioremap_resource() will log for you.

> +	clk_prepare_enable(sqi->sys_clk);
> +	clk_prepare_enable(sqi->base_clk);

Check the return value please.

> +	/* install irq handlers */
> +	ret = devm_request_irq(&pdev->dev, sqi->irq, pic32_sqi_isr,
> +			       0, dev_name(&pdev->dev), sqi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "request-irq %d, failed ?\n", sqi->irq);
> +		goto err_free_ring;
> +	}

This will free before the clocks are disabled.  Are you sure that's
safe?  It's generally not good to mix devm_ and non-devm operations
especially things like these that aren't simple frees of data.  It is
safer to use a normal request_irq().

> +static int pic32_sqi_remove(struct platform_device *pdev)
> +{
> +	struct pic32_sqi *sqi = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(sqi->base_clk);
> +	clk_disable_unprepare(sqi->sys_clk);
> +
> +	/* release memory */
> +	ring_desc_ring_free(sqi);

This will free the descriptor ring before the interrupt...

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