linux-kernel - Re: [PATCH 5/5] max44000: Initial triggered buffer support

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:	Sun, 17 Apr 2016 09:41:38 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Crestez Dan Leonard <leonard.crestez@...el.com>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Daniel Baluta <daniel.baluta@...el.com>
Subject: Re: [PATCH 5/5] max44000: Initial triggered buffer support

On 11/04/16 17:11, Crestez Dan Leonard wrote:
> On 04/07/2016 10:59 PM, Peter Meerwald-Stadler wrote:
>>>   static int max44000_probe(struct i2c_client *client,
>>>                 const struct i2c_device_id *id)
>>>   {
>>> @@ -513,6 +569,12 @@ static int max44000_probe(struct i2c_client *client,
>>>           return ret;
>>>       }
>>>
>>> +    ret = iio_triggered_buffer_setup(indio_dev, NULL, max44000_trigger_handler, NULL);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>>       return iio_device_register(indio_dev);
>>
>> no devm_ possible anymore :-)
> 
> It's not clear to me why explicit calls to
> iio_triggered_buffer_cleanup are done in every driver. Wouldn't it be
> possible for iio_dev_release to call iio_triggered_buffer_cleanup?
> That would require triggered_buffer_cleanup to explictly NULL the
> fields it deallocates so that duplicate cleanups don't crash.
Because not all devices are using the triggered buffer setup function
thus they can't use the cleanup one.  There are different buffers for
starters and the core has no way to identify them (very deliberately
as it should never care what they are).

There is also the general obvious correctness of code question.
If you do one thing in probe, it should be reverse in remove...

We could in theory do a devm version of the iio_triggered_buffer_cleanup
function though as then the tear down would occur in the write order and
as it is a devm_ call in the probe we'd know how the unwind was being
done...


> -- 
> Regards,
> Leonard