From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756245AbbIUISm (ORCPT ); Mon, 21 Sep 2015 04:18:42 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:15208 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756009AbbIUISk (ORCPT ); Mon, 21 Sep 2015 04:18:40 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-ef-55ffbd5d0609 Subject: Re: [PATCH] iio: gyro: ssp_gyro_sensor: Use devm_iio_device_register To: Jonathan Cameron , Vaishali Thakkar References: <20150914160849.GA21748@localhost> <55FF068A.30302@kernel.org> Cc: Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org From: Karol Wrona Message-id: <55FFBD5F.5020304@samsung.com> Date: Mon, 21 Sep 2015 10:18:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-version: 1.0 In-reply-to: <55FF068A.30302@kernel.org> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLLMWRmVeSWpSXmKPExsVy+t/xa7qxe/+HGjR8N7R40LSKyWLX/zfM Fksmz2e1mHfkHYvF5V1z2Cx+7zrGbvF85TVWB3aPnbPusnt8+BjnsWlVJ5vHkjeHWD3ONx9h 9Pi8SS6ALYrLJiU1J7MstUjfLoErY+XORpaClRIVm642sjcw3hPuYuTkkBAwkbh29iYzhC0m ceHeerYuRi4OIYGljBLz7m0FSwgJPGeUWPDZsIuRg0NYwFfi3dpckLCIQIhEz9p+qBIviZv3 voH1MgusZpTYeO4nWIJNQF2iecdiMJtXQEti8YqVrCA2i4CqxN89d1lAbFGBCIlTZ9+yQdQI SvyYfA8szimgIbFgyRkmkL3MAnoS9y9qgYSZBeQlNq95yzyBUWAWko5ZCFWzkFQtYGRexSia WppcUJyUnmuoV5yYW1yal66XnJ+7iRES6F92MC4+ZnWIUYCDUYmH10Hgf6gQa2JZcWXuIUYJ DmYlEV6dWUAh3pTEyqrUovz4otKc1OJDjNIcLErivHN3vQ8REkhPLEnNTk0tSC2CyTJxcEo1 MKr2L5vtdGLLbtF7m998mcYRumTKRr9lYkpaS5XYX2hcv9d96YlPDrMQf/6M+ueG1u8FpwVK zJuV8sORx/LhuVnJvzkSG38F5vo1uFv3ThaculZzinHTK65wHZ96iwe5rxUSf88tYrTYGpFZ fv7uwgTdzeucMs+xfHDPl1yWMp9voqtFxy/HCCWW4oxEQy3mouJEAKWhIjJwAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 >