linux-kernel - Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

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: <f91d6d0a-e197-a66b-61b7-f2ea668429f7@topic.nl>
Date:   Mon, 6 Mar 2023 12:21:44 +0100
From:   Mike Looijmans <mike.looijmans@...ic.nl>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Caleb Connolly <caleb.connolly@...aro.org>,
        ChiYuan Huang <cy_huang@...htek.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@...icproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 04-03-2023 18:57, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <mike.looijmans@...ic.nl> wrote:
>
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> Hi Mike,
>
> A few minor things + one request for a test as trying to chase a possible
> ref count overflow around the runtime_pm was giving me a enough of a headache
> that it's easier to ask you just to poke it and see.  If it doesn't fail as
> I expect I'll take a closer look!
>
> Jonathan
>
> ...
>> +	data->client = client;
>> +	mutex_init(&data->lock);
>> +
>> +	indio_dev->name = "ads1100";
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = &ads1100_channel;
>> +	indio_dev->num_channels = 1;
>> +	indio_dev->info = &ads1100_info;
>> +
>> +	data->reg_vdd = devm_regulator_get(dev, "vdd");
>> +	if (IS_ERR(data->reg_vdd))
>> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
>> +				     "Failed to get vdd regulator\n");
>> +
>> +	ret = regulator_enable(data->reg_vdd);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
>> +				     "Failed to enable vdd regulator\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
>> +	if (ret)
>> +		return ret;
> Please could you check a subtle interaction of runtime pm and this devm managed
> flow.
>
> I think we can hit the following flow.
> 1) In runtime suspend (wait long enough for this to happen).
> 2) Unbind the driver (rmmod will do)
> 3) During the unbind we exit suspend then enter it again before we call remove
>     (that's just part of the normal remove flow).
> 4) We then end up calling regulator disable when it's already disabled.
>
> We've traditionally avoided that by having the remove explicitly call
> pm_runtime_get_sync() before we then disable runtime pm.  I don't
> think that happens with devm_pm_runtime_enable() but I could be missing
> a path where it does.
>
> If the sequence goes wrong you should get a warning about an unbalanced regulator
> disable.  The fix would be an extra devm_add_action_or_reset() before the
> devm_iio_device_register() below that just calls pm_runtime_get_sync()
> to force the state to on.
>
> Gah. These subtle paths always give me a headache.
> We don't normally have too much problem with this because many
> runtime_resume / suspend functions don't change reference counts.

Just did this test, waited a few seconds, checked 
/sys/kernel/debug/regulator... that the regulator had been disabled.

Then executed:
echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind

to unload the driver, and no messages were added to the kernel log.

I could see the driver going away and removing itself from iio and 
regulators.

Tried this a couple of times (using bind/unbind), and no problem reported.

Hopes this helps with your headaches...

-- 
Mike Looijmans