linux-kernel - Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late

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:	Mon, 15 Aug 2016 16:40:21 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Guenter Roeck <linux@...ck-us.net>,
	Alexander Stein <alexander.stein@...tec-electronic.com>
Cc:	Quentin Schulz <quentin.schulz@...e-electrons.com>,
	linux-kernel@...r.kernel.org, jdelvare@...e.com, knaack.h@....de,
	lars@...afoo.de, pmeerw@...erw.net,
	maxime.ripard@...e-electrons.com, wens@...e.org,
	lee.jones@...aro.org, linux-hwmon@...r.kernel.org,
	linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	thomas.petazzoni@...e-electrons.com,
	antoine.tenart@...e-electrons.com
Subject: Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

On 26/07/16 17:04, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>>>>>> and
>>>>>>> properties in the Device Tree or channels whose consumer_dev_name
>>>>>>> matches
>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>>>>>> which might be probed after iio_hwmon.
>>>>>>
>>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>>>> returning
>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>> driver/device
>>>>>> dependencies seems not right for me at this place.
>>>>>
>>>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>>>> driver? We'll have to change the error code, things we are apparently
>>>>> trying to avoid (see v2 patches' discussions).
>>>>
>>>> Maybe I didn't express my idea enough. I don't want to change the behavior
>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
>>>> below in mind.
>>>>
>>>> Best regards,
>>>> Alexander
>>>> ---
>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>> index b550ba5..e32d150 100644
>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>                 name = dev->of_node->name;
>>>>         
>>>>         channels = iio_channel_get_all(dev);
>>>>
>>>> -       if (IS_ERR(channels))
>>>> -               return PTR_ERR(channels);
>>>> +       if (IS_ERR(channels)) {
>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>> +                       return -EPROBE_DEFER;
>>>> +               else
>>>> +                       return PTR_ERR(channels);
>>>> +       }
>>>>
>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>         if (st == NULL) {
>>>
>>> Indeed, I misunderstood what you told me.
>>>
>>> Actually, the patch you proposed is part of my v1
>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>> (https://lkml.org/lkml/2016/7/15/215).
>>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>>> in -EPROBE_DEFER.
>>
>> Thanks for the links.
>>
>>> What I thought you were proposing was to change the -ENODEV return code
>>> inside iio_channel_get_all. This cannot be an option since the function
>>> might be called outside of a probe (it is not yet, but might be in the
>>> future?).
>>
>> AFAICS this is a helper function not knowing about device probing itself. And 
>> it should stay at that.
>>
>>> Of what I understood, two possibilities are then possible (proposed
>>> either by Guenter or Jonathan): either rework the iio framework to
>>> register iio map array earlier or to use late_initcall instead of init
>>> for the driver consuming the iio channels.
>>
>> Interestingly using this problem would not arise due to module dependencies. 
>> But using late_initcall would mean this needs to be done on any driver using 
>> iio channels? I would rather keep those consumers simple.
>>
> Me too, but that would imply a solution in iio. The change you propose above
> isn't exactly simple either, and would also be needed in each consumer driver.
> 
> Just for the record, I dislike the late_initcall solution as well, but I prefer
> it over blindly converting ENODEV to EPROBE_DEFER.
I'm falling on the other side on this one right now. Though I'd be tempted
to renaming the function to something like iio_channel_get_all_or_defer
to make it explicit that it can result in deferred probing.

To my mind late_initcall tricks are rather hideous as well.  At least 
deferred probing should always work (with a fair cost associated with it of
course)

Lots of discussions ongoing on how to do dependency resolution that will
hopefully mean deferred probing only occurs in the pathological cases in
the future anyway. Interesting to see where that goes.

Jonathan
> 
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>