linux-kernel - Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

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:   Tue, 13 Sep 2016 09:06:34 +0200
From:   Quentin Schulz <quentin.schulz@...e-electrons.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     jdelvare@...e.com, linux@...ck-us.net, jic23@...nel.org,
        knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        maxime.ripard@...e-electrons.com, wens@...e.org,
        thomas.petazzoni@...e-electrons.com,
        antoine.tenart@...e-electrons.com, linux-kernel@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

On 12/09/2016 15:56, Lee Jones wrote:
> On Mon, 12 Sep 2016, Quentin Schulz wrote:
>> On 12/09/2016 11:59, Lee Jones wrote:
>>> On Mon, 12 Sep 2016, Quentin Schulz wrote:
>>>
>>>> On 12/09/2016 11:18, Lee Jones wrote:
>>>>> On Thu, 08 Sep 2016, Quentin Schulz wrote:
>>>>> [...]
>>>>>> +
>>>>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match);
>>>>>
>>>>> Place this directly under the table.
>>>>>
>>>>>> +static struct platform_driver sun4i_gpadc_mfd_driver = {
>>>>>> +	.driver = {
>>>>>> +		.name = "sun4i-adc-mfd",
>>>>>> +		.of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match),
>>>>>> +	},
>>>>>> +	.probe = sun4i_gpadc_mfd_probe,
>>>>>
>>>>> No .remove?
>>>>>
>>>>
>>>> No, everything in probe is handled with devm functions.
>>>
>>> Don't you need to undo the register write you did?
>>>
>>
>> The regmap_write I use is there to disable all interrupts on hardware
>> side before the irq_chip handles all interrupts by itself. The
>> interrupts are not used in the MFD driver.
>>
>> Thus, I chose to disable the hardware interrupts in the remove function
>> of drivers using the interrupts (only the IIO yet but the touchscreen
>> driver later also which will be using a third interrupt). When the MFD
>> driver is removed, the MFD cells will all be removed, thus calling their
>> own remove functions, thus disabling hardware interrupts used in each
>> driver. So the hardware interrupts disabling would be called twice.
> 
> This does send some little alarm bells ringing.  I'd normally expect
> the .remove function to undo everything you did in .probe.  So, if you
> are disabling the IRQs from within the leaf drivers, shouldn't you be
> initialising them in the leaf driver's respective .probes?
> 

I use the regmap_write in the MFD driver's probe to disable all
interrupts before requesting irq_chip to guarantee the interrupts are in
a known state, being disabled. It is to insure no interrupt will occur
unwittingly before we want the leaf drivers to handle them.

The disabling of irqs in the remove is handled rather by
devm_regmap_del_irq_chip than by an explicit regmap_write in the
driver's removal function. It performs the exact same thing.

I always use devm functions for requesting either an irq_chip or the
irqs themselves. In that case, when the device is removed, the irqs are
freed on leaf drivers' (where the irqs are requested) removal while the
removal of irq_chip in the MFD driver will also free all irqs mapped to
this irq_chip thanks to devm_regmap_del_irq_chip. Therefore, the
interrupts are disabled by devm functions.

The regmap_update_bits in probe and removal of the ADC driver to disable
irqs are actually redundant because the devm functions already handle
the irqs disabling.

Thanks,
Quentin