LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Lars-Peter Clausen <lars@metafoo.de>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Eugen Hristev <eugen.hristev@microchip.com>,
	Jonathan Cameron <jic23@kernel.org>,
	ludovic.desroches@microchip.com, alexandre.belloni@bootlin.com,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	linux-input@vger.kernel.org, nicolas.ferre@microchip.com,
	robh@kernel.org
Subject: Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels
Date: Wed, 18 Apr 2018 14:37:01 +0200	[thread overview]
Message-ID: <79d51d4e-eea7-7295-7e32-26149d6aeadd@metafoo.de> (raw)
In-Reply-To: <20180418103548.000078b7@huawei.com>

On 04/18/2018 11:35 AM, Jonathan Cameron wrote:
> On Tue, 17 Apr 2018 12:19:06 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
>> Hi Eugen,
>>
>> On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
>>>
>>>
>>> On 17.04.2018 02:58, Dmitry Torokhov wrote:  
>>>> On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:  
>>>>> On Tue, 10 Apr 2018 11:57:52 +0300
>>>>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>>>>   
>>>>>> When requesting channels for a particular consumer device,
>>>>>> besides requesting the device (incrementing the reference counter), also
>>>>>> do it for the driver module of the iio dev. This will avoid the situation
>>>>>> where the producer IIO device can be removed and the consumer is still
>>>>>> present in the kernel.
>>>>>>
>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>> ---
>>>>>>   drivers/iio/inkern.c | 8 +++++++-
>>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>>>> index ec98790..68d9b87 100644
>>>>>> --- a/drivers/iio/inkern.c
>>>>>> +++ b/drivers/iio/inkern.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>   #include <linux/slab.h>
>>>>>>   #include <linux/mutex.h>
>>>>>>   #include <linux/of.h>
>>>>>> +#include <linux/module.h>
>>>>>>   #include <linux/iio/iio.h>
>>>>>>   #include "iio_core.h"
>>>>>> @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>>>>>>   	if (index < 0)
>>>>>>   		goto err_put;
>>>>>>   	channel->channel = &indio_dev->channels[index];
>>>>>> +	try_module_get(channel->indio_dev->driver_module);  
>>>>>
>>>>> And if it fails? (the module we are trying to get is going away...)
>>>>>
>>>>> We should try and handle it I think. Be it by just erroring out of here.  
>>>>
>>>> Even more, this has nothing to do with modules. A device can go away for
>>>> any number of reasons (we unbind it manually via sysfs, we pull the USB
>>>> plug from the host in case it is USB-connected device, we unload I2C
>>>> adapter for the bus device resides on, we kick underlying PCI device)
>>>> and we should be able to handle this in some fashion. Handling errors
>>>> from reads and ignoring garbage is one of methods.
>>>>
>>>> FWIW this is a NACK from me.
>>>>
>>>> Thanks.  
>>> Hello,
>>>
>>> This patch is actually a "best effort attempt" for the consumer driver
>>> (touch driver) to get a reference to the producer of the data (the IIO
>>> device), when it requests the specific channels.
>>> As of this moment, there is no attempt whatsoever for the consumer to have a
>>> reference on the producer driver. Thus, the producer can be removed at any
>>> time, and the consumer will fail ungraciously.  
>>
>> This is the root of the issue. The consumer should be prepared to handle
>> errors from producer.
>>
>>> I can change the perspective from "best effort" to "mandatory" to get a
>>> reference to the producer, or you wish to stop trying to get any reference
>>> at all (remove this patch completely) ?  
>>
>> You should take reference to the device itself (if it is not taken
>> already), so it does not disappear completely and you can continue using
>> IIO API to access it, and IIO API should be prepared to deal with "dead"
>> devices, but as I pointed in my other email, trying to pin the driver
>> is quite pointless as there are myriad other ways of device stopping
>> working besides module unloading.
>>
>> In any case, I think this problem is outside of the scope of this
>> patchset that adds a generic resistive touchscreen, so if you want to
>> continue working on this I'd recommend moving it into a separate series.
>>
>> Thanks.
>>
> Agreed, this one has come up a number of times before.  Quite a lot of
> work got done by (IIRC) Lars Peter Clausen to stabilize things in various
> unexpected 'going away' events.  Of course there may be paths we have
> added since that (it was years ago) that can cause trouble...
> 
> Anyhow, separate issue as Dmitry says, let's deal with it separately.

We do properly get a ref to the device. Not getting a ref to the module is
on purpose. Cause as Dmitry said it does nothing the device can disappear at
any point either way. I second the NACK.

  reply	other threads:[~2018-04-18 12:38 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-10  8:57 [PATCH v3 00/11] Add support for SAMA5D2 touchscreen Eugen Hristev
2018-04-10  8:57 ` [PATCH v3 01/11] iio: adc: at91-sama5d2_adc: fix channel configuration for differential channels Eugen Hristev
2018-04-15 19:23   ` Jonathan Cameron
2018-04-10  8:57 ` [PATCH v3 02/11] MAINTAINERS: add generic resistive touchscreen adc Eugen Hristev
2018-04-10  8:57 ` [PATCH v3 03/11] iio: Add channel for Position Relative Eugen Hristev
2018-04-15 19:29   ` Jonathan Cameron
2018-04-17  7:30     ` Eugen Hristev
2018-04-18  9:32       ` Jonathan Cameron
2018-04-10  8:57 ` [PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure threshold touchscreen property Eugen Hristev
2018-04-13 18:33   ` Rob Herring
2018-04-10  8:57 ` [PATCH v3 05/11] dt-bindings: input: touchscreen: resistive-adc-touch: create bindings Eugen Hristev
2018-04-13 18:34   ` Rob Herring
2018-04-10  8:57 ` [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels Eugen Hristev
2018-04-15 19:33   ` Jonathan Cameron
2018-04-16 23:58     ` Dmitry Torokhov
2018-04-17  7:39       ` Eugen Hristev
2018-04-17 19:19         ` Dmitry Torokhov
2018-04-18  9:35           ` Jonathan Cameron
2018-04-18 12:37             ` Lars-Peter Clausen [this message]
2018-04-10  8:57 ` [PATCH v3 07/11] iio: adc: at91-sama5d2_adc: add support for position and pressure channels Eugen Hristev
2018-04-15 19:45   ` Jonathan Cameron
2018-04-10  8:57 ` [PATCH v3 08/11] input: touchscreen: resistive-adc-touch: add generic resistive ADC touchscreen Eugen Hristev
2018-04-23 23:22   ` Dmitry Torokhov
2018-04-23 23:24     ` Dmitry Torokhov
2018-04-10  8:57 ` [PATCH v3 09/11] dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer info Eugen Hristev
2018-04-10  8:57 ` [PATCH v3 10/11] ARM: dts: at91: sama5d2: add channel cells for ADC device Eugen Hristev
2018-04-10  8:57 ` [PATCH v3 11/11] ARM: dts: at91: sama5d2: Add resistive touch device Eugen Hristev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=79d51d4e-eea7-7295-7e32-26149d6aeadd@metafoo.de \
    --to=lars@metafoo.de \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=eugen.hristev@microchip.com \
    --cc=jic23@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ludovic.desroches@microchip.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=robh@kernel.org \
    --subject='Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).