Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register

[Karol Wrona <k.wrona@samsung.com>]

On 09/28/2015 12:08 PM, Vaishali Thakkar wrote:
> On Mon, Sep 28, 2015 at 6:42 AM, Vaishali Thakkar
> <vthakkar1994@gmail.com> wrote:
>> On Sun, Sep 27, 2015 at 7:01 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 27/09/15 05:16, Vaishali Thakkar wrote:
>>>> On Mon, Sep 21, 2015 at 4:48 PM, Karol Wrona <k.wrona@samsung.com> wrote:
>>>>> On 09/21/2015 11:53 AM, Jonathan Cameron wrote:
>>>>>>
>>>>>>
>>>>>> On 21 September 2015 09:18:39 BST, Karol Wrona <k.wrona@samsung.com> wrote:
>>>>>>> On 09/20/2015 09:18 PM, Jonathan Cameron wrote:
>>>>>>>> On 14/09/15 17:08, Vaishali Thakkar wrote:
>>>>>>>>> Use resourced managed function devm_iio_device_register to
>>>>>>>>> make error path simpler. To be compatible with the change,
>>>>>>>>> the remove function is removed as it is now redundant.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>>>>>>> This patch is reasonable, but makes me wonder if there is an issue
>>>>>>>> in the remove path for this driver.  It relies on the ssp_sensors
>>>>>>> common
>>>>>>>> module.  That in ssp_spi.c uses the fact ssp_register_consumer
>>>>>>>> has saved the struct iio_dev into a local array in the core driver.
>>>>>>>> I think this means that a remove of this function will leave a
>>>>>>> possible
>>>>>>>> null pointer de reference.
>>>>>>> You are right. In this case we need something like
>>>>>>> ssp_deregister_consumer(...) in ssp_dev.c. So I think that "remove
>>>>>>> func"
>>>>>>> should stay. Of course we can switch to devm iio register.
>>>>>> Not if you want to remove the userspace interface before doing you new function call.  Doing so gives a nasty race.
>>>>> So better leave it out:
>>>>> iio_device_unregister(indio_dev) will disable the buffers and through
>>>>> ops disables the sensor and than remove iio dev ptr from the internal table.
>>>>>
>>>>
>>>> Sorry for the late reply. Yes, I guess I missed the point that
>>>> 'ssp_register_consumer' is
>>>> used in probe function. And I believe that devm_iio_register is useful
>>>> only when we
>>>> can convert all other functions to their devm counterparts and remove
>>>> function will go
>>>> away.
>>> Yes. Exactly.
>>>>
>>>> But then why don't we need ssp_deregister_consumer here in the remove function?
>>>> Is it automatically handled by  iio_device_unregister? I guess Karol
>>>> tried to explain
>>>> the same thing but I am still confused. Same case applies for
>>>> ssp_sensors/ssp_dev.c.
>>> We do indeed need an ssp_deregister_consumer function to be called in the remove.
>>> I don't think one currently exists, so that needs fixing.
>>
>> Ok. Then I'll send patches for both of these files.
> 
> Sorry, probably I was not clear enough in my last mail. I mean I'll
> send patches patches
> once we will have something like 'ssp_deregister_consumer'. But is
> there anyone who
> is working on this? Is it possible to introduce devm counterpart of the same?

If you want you can simply add ssp_deregister_consumer (to ssp_dev.c)
function and use it in the remove (in ssp sensor platform driver). I
will test it.

We can use devm_iio_device_register for iio dev itself but better check
what is called first - driver remove callback or the devm removes.
If we deregister internally the iio device from ssp we should be quite safe.


> How much will it be useful [taking note that there are only 3-4 files
> which are using it]?
> 
>>>>>>>
>>>>>>> Also the same can (rather should) be done for accelerometer sensor
>>>>>>> (ssp_accel_sensor.c).
>>>>>>>
>>>>>>> Vaishali, if you want please feel free and send patch.
>>>>>>>
>>>>>>>>
>>>>>>>> Now I suspect that case doesn't actually occur because the relevant
>>>>>>>> device elements are disabled whenever this module is removed.  Having
>>>>>>>> said that we might expect an ssp_unregister_consumer function that
>>>>>>>> sets the relevant pointer back to null on removal so as to avoid
>>>>>>>> any possible race conditions around driver removal / reprobing.
>>>>>>>> A spot of defensive programming rather than necessarily a bug to be
>>>>>>>> fixed!
>>>>>>>>
>>>>>>>> One little process thing. This driver was written by Karol so patches
>>>>>>>> should probably always cc Karol as well as the more general
>>>>>>>> maintainer / reviewers for IIO.  Added cc.
>>>>>>>>
>>>>>>>> Jonathan
>>>>>>>>> ---
>>>>>>>>>  drivers/iio/gyro/ssp_gyro_sensor.c | 12 +-----------
>>>>>>>>>  1 file changed, 1 insertion(+), 11 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c
>>>>>>> b/drivers/iio/gyro/ssp_gyro_sensor.c
>>>>>>>>> index 0a8afdd..ac88de7 100644
>>>>>>>>> --- a/drivers/iio/gyro/ssp_gyro_sensor.c
>>>>>>>>> +++ b/drivers/iio/gyro/ssp_gyro_sensor.c
>>>>>>>>> @@ -134,7 +134,7 @@ static int ssp_gyro_probe(struct platform_device
>>>>>>> *pdev)
>>>>>>>>>
>>>>>>>>>    platform_set_drvdata(pdev, indio_dev);
>>>>>>>>>
>>>>>>>>> -  ret = iio_device_register(indio_dev);
>>>>>>>>> +  ret = devm_iio_device_register(&pdev->dev, indio_dev);
>>>>>>>>>    if (ret < 0)
>>>>>>>>>            return ret;
>>>>>>>>>
>>>>>>>>> @@ -144,21 +144,11 @@ static int ssp_gyro_probe(struct
>>>>>>> platform_device *pdev)
>>>>>>>>>    return 0;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> -static int ssp_gyro_remove(struct platform_device *pdev)
>>>>>>>>> -{
>>>>>>>>> -  struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>>>>>> -
>>>>>>>>> -  iio_device_unregister(indio_dev);
>>>>>>>>> -
>>>>>>>>> -  return 0;
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>>  static struct platform_driver ssp_gyro_driver = {
>>>>>>>>>    .driver = {
>>>>>>>>>            .name = SSP_GYROSCOPE_NAME,
>>>>>>>>>    },
>>>>>>>>>    .probe = ssp_gyro_probe,
>>>>>>>>> -  .remove = ssp_gyro_remove,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>>  module_platform_driver(ssp_gyro_driver);
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>>> in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> --
>> Vaishali
> 
> 
>