linux-kernel - Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 24 Jul 2013 16:17:09 +0300
From:	Grygorii Strashko <grygorii.strashko@...com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	Samuel Ortiz <sameo@...ux.intel.com>,
	Kevin Hilman <khilman@...aro.org>,
	Graeme Gregory <gg@...mlogic.co.uk>,
	<linux-omap@...r.kernel.org>,
	Ruslan Bilovol <ruslan.bilovol@...com>,
	<linux-kernel@...r.kernel.org>,
	Naga Venkata Srikanth V <vnv.srikanth@...sung.com>,
	Oleg_Kosheliev <oleg.kosheliev@...com>
Subject: Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

On 07/24/2013 03:50 PM, Lee Jones wrote:
>>>> +	if (ret) {
>>>> +		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);
>>>
>>> Does the user really care which function we're returning from.
>>>
>>> Would it be better if you replace '__func__' with the device name?
>>
>> This module hasn't been converted to the device yet:(
>> (I mean "interrupt-controller").
>> But I'm thinking about it as the next step :) and then It will be
>> absolutely reasonable change to replace pr_*() with dev_*() and
>> remove __func__.
>
> I don't mean anything as compicated as that for 'this' patch. (NB: See my
> comment in subsequent patches about creating a 'struct twl6030' where
> you could store 'struct dev'.) In this patch I mean litterally
> replacing "%s: ", with "tw16030_irq: ". Simples. :)

Ok. I understand it now - will redo.


>
>> Now, the pointer on "dev" (in our case "twl-core" device) isn't passed
>> in IRQ handler, so It can't be used here.
>>
>> Of course it can be done, but would it make code better?
>> My opinion - no.
>
>>>> +	if (sts.bytes[2] & 0x10)
>>>> +		sts.bytes[2] |= 0x08;
>>>>
>>>> -		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
>>>> -			local_irq_disable();
>>>> -			if (sts.int_sts & 0x1) {
>>>> -				int module_irq = twl6030_irq_base +
>>>> +	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>>>> +		if (sts.int_sts & 0x1) {
>>>
>>> I'm a little confused by this. Where does sts.int_sts come from?
>>
>> See my comment above, pls
>
> Okay, that's my fault for not understanding unions properly as I've
> never had to use one, but now I do, thanks.
>
>>>> @@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
>>>>   {
>>>>   	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>>>>
>>>> -	if (twl6030_irq_base) {
>>>> +	if (!twl6030_irq_base) {
>>>>   		pr_err("twl6030: can't yet clean up IRQs?\n");
>>>>   		return -ENOSYS;
>>>>   	}
>>>> +
>>>> +	free_irq(twl_irq, NULL);
>>>> +
>>>
>>> If request_threaded_irq() fails, isn't there a chance that
>>> twl6030_irq_base will be allocated, but twl_irq will still be
>>> undefined?
>>
>> Yes. A mess is here (historically:), thanks. Will use twl_irq
>> instead of twl6030_irq_base (I did it, actually, in patch [3]:).
>
> Yes, I saw it. It would be better if you still fixed up this patch to
> be correct though. Even if you break it out and add it as [PATCH 1/x].
>
ok

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