netdev - Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request

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:	Fri, 07 Mar 2014 12:42:28 +0100
From:	Christian Riesch <christian.riesch@...cron.at>
To:	David Miller <davem@...emloft.net>,
	Prabhakar Lad <prabhakar.csengg@...il.com>,
	Mugunthan V N <mugunthanvnm@...com>
cc:	davinci-linux-open-source@...ux.davincidsp.com,
	f.fainelli@...il.com, netdev@...r.kernel.org, jon@...gle.org
Subject: Re: [PATCH] net: davinci_emac: Replace devm_request_irq with
 request_irq

Hi,

--On March 06, 2014 16:57 -0500 David Miller <davem@...emloft.net> wrote:

> From: Christian Riesch <christian.riesch@...cron.at>
> Date: Wed, 5 Mar 2014 11:25:28 +0100
>
>> @@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
>>
>>  rollback:
>>
>> -	dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
>> +	dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
>> +
>> +	for (q = k; k >= 0; k--) {
>> +		for (m = i; m >= res->start; m--)
>> +			free_irq(m, ndev);
>> +		res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
>> +		m = res->end;
            ^^^^
So this should definitely read "i = res->end;".

>> +	}
>
> This final assignment in the loop of 'm' is unused?
>
> The inner for() loop always started with "m = i;"

But it is more than just that. The entire rollback mechanism of 
emac_dev_open is a mess. It just won't work.

1) Freeing the interrupts. Currently, the code tries to do a 
platform_get_resource(priv->pdev, IORESOURCE_IRQ, -1) in its last 
iteration. To make this work, the code should be something like

for (q = k; q >= 0; q--) {
	res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
	if (q != k)
		i = res->end;

	for (m = i; m >= res->start; m--)
		free_irq(m, ndev);
}		

2) Wrong order of err: and rollback: labels. Currently the function does 
something like this:

	pm_runtime_get
	request irq
		if requesting irqs fails, goto rollback
	setup phy
		if phy setup fails, goto err
	return 0

rollback:
	free irqs
err:
	pm_runtime_put

So if PHY initialization fails, the IRQs are never freed. So rollback and 
err must be exchanged, and some additonal code must be added to keep the 
correct values of k and i for rollback.

3) RX DMA descriptors are not freed in case of an error: Right before 
requesting the irqs, the function creates DMA descriptors for the RX 
channel. These RX descriptors are never freed when we jump to either 
rollback or err. And I think there is also no way to free them as long as 
cpdma_ctlr_start() has not been called (which is done between requesting 
irqs and phy setup in the code).

Problems 1+2 are easy to solve. Prabhakar, Mugunthan, any ideas for 3? I 
think cpsw_ndo_start() in cpsw.c has the same problem. It will also try to 
call cpdma_ctlr_stop in error handling before cpdma_ctlr_start was called 
in some cases.

Regards, Christian

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html