linux-kernel - Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

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: <877fvga2mb.fsf@free.fr>
Date:	Tue, 17 Feb 2015 18:07:08 +0100
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:	Gregory Clement <gregory.clement@...e-electrons.com>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Brian Norris <computersforpeace@...il.com>,
	Lior Amsalem <alior@...vell.com>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Thomas Petazzoni <thomas@...e-electrons.com>,
	Seif Mazareeb <seif@...vell.com>, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org,
	Sudhakar Gundubogula <sudhakar@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Boris Brezillon <boris@...e-electrons.com>,
	linux-mtd@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

Maxime Ripard <maxime.ripard@...e-electrons.com> writes:

> On Mon, Feb 16, 2015 at 10:36:02PM +0100, Robert Jarzmik wrote:
>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> index e512902..6e569e9 100644
>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> @@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>>  {}
>>  #endif
>>  
>> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
>> +{
>> +	struct pxa3xx_nand_info *info = data;
>> +
>> +	handle_data_pio(info);
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  {
>>  	struct pxa3xx_nand_info *info = devid;
>>  	unsigned int status, is_completed = 0, is_ready = 0;
>>  	unsigned int ready, cmd_done;
>> +	irqreturn_t ret = IRQ_HANDLED;
>>  
>>  	if (info->cs == 0) {
>>  		ready           = NDSR_FLASH_RDY;
>> @@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  		} else {
>>  			info->state = (status & NDSR_RDDREQ) ?
>>  				      STATE_PIO_READING : STATE_PIO_WRITING;
>> -			handle_data_pio(info);
>> +			ret = IRQ_WAKE_THREAD;
>>  		}
>>  	}
>>  	if (status & cmd_done) {
>> @@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  	if (is_ready)
>>  		complete(&info->dev_ready);
>>  NORMAL_IRQ_EXIT:
>> -	return IRQ_HANDLED;
>> +	return ret;
>>  }
>>  
>>  static inline int is_buf_blank(uint8_t *buf, size_t len)
>> @@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
>>  	/* initialize all interrupts to be disabled */
>>  	disable_int(info, NDSR_MASK);
>>  
>> -	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
>> +	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
>> +				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
>>  	if (ret < 0) {
>>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>>  		goto fail_free_buf;
>
> I just gave this patch a try, and it blows up quite badly:
> http://code.bulix.org/p96krc-87889?raw
Confirmed.
OTOH, replacing :
>> +			ret = IRQ_WAKE_THREAD;
With:
+			ret = IRQ_WAKE_THREAD;
+			goto NORMAL_IRQ_EXIT;

is fully working in my environment, now I got 10mn to test it.
I also added for cosmetics in pxa3xx_nand_irq_thread() a :
	info->state = STATE_CMD_DONE;

> It looks like there's more work here, most likely in the waitqueues
> wake up.
Not that much if I'm right, hein ?
And it enables the msleep() instead of mdelay().

And the flow of the driver is not changed for pxa3xx, only for Armada (ie. non
dma case) for which you need to introduce a delay anyway, and therefore change
the flow.

It will be Brian choice eventually, but if you say that you will submit that
approach for next cycle, and yours for stable, and that for next you'll convert
mdelay() to msleep(), I'll stop arguing.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/