linux-kernel - Re: [PATCH v2 2/4] input: keyboard: tegra: use devm

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:	Sun, 6 Jan 2013 16:30:28 +0530
From:	Laxman Dewangan <ldewangan@...dia.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Stephen Warren <swarren@...dia.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource
 allocation

On Sunday 06 January 2013 04:48 AM, Dmitry Torokhov wrote:
> On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote:
>> HI Dmitry,
>> Thanks for quick review.
>>
>> I will take care of your comment in next version. Some have my answer.
>>
>>
>> On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote:
>>> Hi Laxman,
>>>
>>> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
>>>> Use devm_* for memory, clock, input device allocation. This reduces
>>>> code for freeing these resources.
>>>>   	err = tegra_kbd_setup_keymap(kbc);
>>>> -	if (err) {
>>>> +	if (err < 0) {
>>> Why is this change? As far as I can see tegra_kbd_setup_keymap() never
>>> returns positive values.
>> Ok, mostly errors are in negative and hence this change, I will
>> revert it and will keep original.
>>
>>>>   		dev_err(&pdev->dev, "failed to setup keymap\n");
>>>> -		goto err_put_clk;
>>>> +		return err;
>>>>   	}
>>>>   	__set_bit(EV_REP, input_dev->evbit);
>>>> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>>>>   	err = request_irq(kbc->irq, tegra_kbc_isr,
>>>>   			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
>>>> -	if (err) {
>>>> +	if (err < 0) {
>>> Neither request_irq(). BTW, why not devm_request_irq?
>> I understand from Mark B on different patches that using
>> devm_request_irq() can create race condition when removing device.
>> Interrupt can occur when device resource release is in process and
>> so it can cause isr call which can use the freed pointer.
>> devm_request_irq() should be avoided.
> devm_request_irq() has a potential of creating a race condition, but it
> depents on the driver. In this particular case tegra driver ensures that
> interrupts are inhibited when input device is unregistered by providing
> tegra_kbc_close() method, so in this particular case it is safe to
> use devm_request_irq().
>
> Also, when using managed input devices, the unregistering and final
> freeing is a 2-step process, so even in absence of close() method, if
> initialization sequence was:
>
> 	devm_input_allocate_device()
> 	...
> 	devm_request_irq()
> 	...
> 	input_unregister_device()
>
> then order of freeing resources (behind the scenes) will be
>
> 	devm_input_device_unregister();
> 	/* input device is still present in memory and can
> 	 * handle input_event() calls.
> 	 */
> 	free_irq();
> 	devm_input_device_release();
>
> So using managed request_irq() _together_ with managed input devices is
> OK.

Thanks for detail explanation. I will modify and sent the new version of 
patch.

--
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/