Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224285 > unrolled thread

[PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register

Back to article view | Back to linux.kernel

Contents

  [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-09-14 18:10 +0200
    Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register Jonathan Cameron <jic23@kernel.org> - 2015-09-20 21:20 +0200
      Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register Karol Wrona <k.wrona@samsung.com> - 2015-09-21 10:20 +0200
        Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2015-09-21 12:00 +0200
          Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register Karol Wrona <k.wrona@samsung.com> - 2015-09-21 13:20 +0200

#1224285 — [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register

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>
---
 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);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1228957

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.

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229110

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.

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229170

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.
>
>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
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229222

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.


>>
>> 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
>>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web