linux-kernel - Re: [PATCH V3 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally

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: <56B5EEB0.3040505@osg.samsung.com>
Date:	Sat, 6 Feb 2016 10:01:36 -0300
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Laxman Dewangan <ldewangan@...dia.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	lee.jones@...aro.org, alexandre.belloni@...e-electrons.com
Cc:	cw00.choi@...sung.com, linux-kernel@...r.kernel.org,
	rtc-linux@...glegroups.com
Subject: Re: [PATCH V3 5/5] rtc: max77686: move initialisation of rtc regmap,
 irq chip locally

Hello Laxman,

On 02/06/2016 03:29 AM, Laxman Dewangan wrote:
> Hi Javier,
> '
> On Saturday 06 February 2016 11:00 AM, Javier Martinez Canillas wrote:
>> Hello Laxman,
>>
>> Sorry for not doing this before but today was a busy one.
>>
>
> Thanks for testing.
>
>
>> On 02/05/2016 11:37 AM, Laxman Dewangan wrote:
>>> Hi Krzysztof, Javier,
>>>
>>
>>> 3. Extension of 2
>>> Do regmap_add_irq_chip(), call  regmap_irq_get_virq() for creating irq mapping but dont do any interrupt registration i.e. comment request_threaded_irq() and hence free_irq().
>>> Then do unbind/bind and then suspend.
>>>    To make sure that this happen when only we have client registered interrupt or with mapping also.
>>>
>>
>> This fails, so the problem seems to be with the mapping.
>>
>> So I tried another scenario:
>>
>> 4. Call regmap_del_irq_chip() just after regmap_irq_get_virq() and try to S2R
>>    without doing any unbind before.
>>
>>    To test if this is a general issue with regmap_del_irq_chip() after doing
>>    the IRQ mapping and not something specific to the remove callback.
>>
>> The machine failed to boot. So now at least we have narrowed down the issue.
>>
>> I've looked at both regmap_irq_get_virq() and regmap_del_irq_chip() but I
>> couldn't find any obvious cause for the issue we are seeing. But it's late
>> Friday so probably I should just stop here and take a fresh look on Monday.
>>
>
> So the issue is that when we create mapping, we can not delete the irq_chip.
>
> I saw one function from irq framework irq_dispose_mapping(unsigned int virq).
>
> So we need to dispose the mapping before deleting irq chip.
>
>
> Becasue it is reproduced in normal boot also if we do create mapping and delete the irq chip data, I will also be able to validate if I get some time on weekend.
>

Thanks, that was it! I was searching yesterday for the regmap_irq_get_virq()
counterpart and did not find it, but I was looking at include/linux/regmap.h
and didn't think about looking at include/linux/irqdomain.h.

Maybe we should add a regmap_irq_put_virq() that's just a wrapper around
irq_dispose_mapping() to make the API consistent and avoid this confusion?
I see that most drivers don't dispose the mapping on their error/remove path.

Anyways, with this patch on top of yours 5/5 my system is able to suspend
after an unbind again:

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 45d639674c0b..8ea6d601482a 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -765,6 +765,7 @@ static int max77686_rtc_remove(struct platform_device *pdev)
  	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
  
  	free_irq(info->virq, info);
+	irq_dispose_mapping(info->virq);
  	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
  	if (info->rtc)
  		i2c_unregister_device(info->rtc);

This is a bug in the current MFD driver though so I would include it as
a separate patch prior to your patch that moves the regmap and i2c init
to the RTC driver.

When posting your new revsion feel free to add my {Reviewed,Tested}-by
tags for $SUBJECT and the new bugfix patch.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America