linux-kernel - Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 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]

Message-ID: <60a058ba-0bd3-9a5e-5021-4f80b9df65a8@quicinc.com>
Date:   Thu, 16 Nov 2023 08:54:04 +0530
From:   Jishnu Prakash <quic_jprakash@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     Jonathan Cameron <jic23@...nel.org>, <agross@...nel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linus.walleij@...aro.org>, <Jonathan.Cameron@...wei.com>,
        <sboyd@...nel.org>, <quic_subbaram@...cinc.com>,
        <quic_collinsd@...cinc.com>, <quic_kamalw@...cinc.com>,
        <marijn.suijten@...ainline.org>,
        <andriy.shevchenko@...ux.intel.com>,
        <krzysztof.kozlowski@...aro.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        Lee Jones <lee@...nel.org>,
        Leonard Göhrs <l.goehrs@...gutronix.de>,
        Haibo Chen <haibo.chen@....com>, <linux-iio@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>,
        <linux-arm-msm-owner@...r.kernel.org>
Subject: Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Hi Dmitry,

On 10/23/2023 1:33 PM, Dmitry Baryshkov wrote:
> On Mon, 23 Oct 2023 at 09:15, Jishnu Prakash<quic_jprakash@...cinc.com>  wrote:
>> Hi Jonathan,
>>
>> On 7/8/2023 9:29 PM, Jonathan Cameron wrote:
>>> On Sat, 8 Jul 2023 12:58:31 +0530
>>> Jishnu Prakash<quic_jprakash@...cinc.com>  wrote:
> +
>>>> +    return devm_iio_device_register(dev, indio_dev);
>>>> +}
>>>> +
>>>> +static int adc5_gen3_exit(struct platform_device *pdev)
>>>> +{
>>> As you are mixing devm manged cleanup and the explicit sort the
>>> result is that you remove the userspace interfaces 'after' you run
>>> everything in here. I'm thinking disabling the channels at least
>>> isn't a good idea in that case.
>>>
>>> If you want to use devm (which is good) then you need to work out how
>>> to register additional callbacks during probe to tear down everything in
>>> the right order (typically the reverse of what happens in probe)
>>> devm_add_action_or_reset() is the way to add those extra callbacks.
>>>
>>> If not, just don't use devm for at least those bits that will end up
>>> running out of order (such as iio_device_register()) and manually call their
>>> cleanup routines instead.
>> I checked some other examples in the iio/adc/ folder, I think I see what
>> you mean here. It looks like drivers with a remove callback always use
>> iio_device_register and iio_device_unregister instead of the devm_*
>> variant, due to the issue with sysfs removal as you said.
>>
>> I'll update the probe and remove functions similarly, to do explicit
>> cleanups as required, avoiding devm_ usage for places where it should be
>> avoided.
> I think you got the message all wrong. There is nothing bad with using
> devm_. As a matter of fact it is a preferred form in most of the
> cases. However you have to be careful to tear down your device in the
> correct order. And as Jonathan pointed
> out, you might add necessary hooks manually by calling
> devm_add_action_or_reset().
>
> [skipped the rest]

Thanks for your comment. I checked the code again and I think we can do 
the teardown with a devm_add_action() call and drop the remove API 
entirely in favor of using devm_* APIs , I'll update this in the next 
patchset.

Thanks,

Jishnu


>