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]

Message-ID: <bce453f4-a776-a534-34f2-c8fcd09a3420@kernel.org>
Date:	Mon, 15 Aug 2016 16:36:28 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Quentin Schulz <quentin.schulz@...e-electrons.com>,
	Alexander Stein <alexander.stein@...tec-electronic.com>
Cc:	linux-kernel@...r.kernel.org, jdelvare@...e.com,
	linux@...ck-us.net, 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 10:33, 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.
> 
> 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?).
If that did happen I'd be tempted to introduce a new function to be
used outside of probe.  I definitely don't like rewriting in individual
drivers.

The defer question is still rather open in my mind. Will address it in the
other thread.

> 
> 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.
Ultimately we probably need to rethink how the registration works
(it was written before deferred probing came along and that has rather
changed things for us!)

For now I'm not convinced that deferring is that painful even if we
do have to try reprobing every time a new module gets probed..

Jonathan
> 
> Thanks,
> Quentin
> --
> 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
>