linux-kernel - Re: [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed functions for accelerometer

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]

Message-ID: <20210329155437.65932679@jic23-huawei>
Date:   Mon, 29 Mar 2021 15:54:37 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <aardelean@...iqon.com>
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org, coproscefalo@...il.com,
        hdegoede@...hat.com, mgross@...ux.intel.com, linux@...iqon.com
Subject: Re: [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed
 functions for accelerometer

On Wed, 24 Mar 2021 14:55:45 +0200
Alexandru Ardelean <aardelean@...iqon.com> wrote:

> This change converts the IIO registration to use devm_iio_device_alloc()
> and devm_iio_device_register().
> With this change we can remove the manual deregistrations an freeing of the
> IIO data.
> 
> This also makes the deregistration symmetrical with the registration.
> 
> One side-effect (that is undesired), is that if devm_iio_device_register()
> fails, then the IIO object will not be free'd and will stick around until
> the parent object is free'd. This is because there is no
> devm_iio_device_free() function anymore in IIO.
> However, this is a pretty bad corner-case that should not happen under
> normal operation.

Hmm. The way this driver papers over failed elements is rather irritating,
though I'm sure there are reasons for it.  Indeed, I'd not worry about the
left over iio_dev structures
 
> 
> Signed-off-by: Alexandru Ardelean <aardelean@...iqon.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

> ---
>  drivers/platform/x86/toshiba_acpi.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index e787c140eec2..12860ef60e4d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2992,11 +2992,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>  
>  	remove_toshiba_proc_entries(dev);
>  
> -	if (dev->accelerometer_supported && dev->indio_dev) {
> -		iio_device_unregister(dev->indio_dev);
> -		iio_device_free(dev->indio_dev);
> -	}
> -
>  	if (dev->sysfs_created)
>  		sysfs_remove_group(&dev->acpi_dev->dev.kobj,
>  				   &toshiba_attr_group);
> @@ -3149,7 +3144,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  
>  	toshiba_accelerometer_available(dev);
>  	if (dev->accelerometer_supported) {
> -		dev->indio_dev = iio_device_alloc(&acpi_dev->dev, sizeof(*dev));
> +		dev->indio_dev = devm_iio_device_alloc(parent, sizeof(*dev));
>  		if (!dev->indio_dev) {
>  			pr_err("Unable to allocate iio device\n");
>  			goto iio_error;
> @@ -3164,10 +3159,10 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  		dev->indio_dev->num_channels =
>  					ARRAY_SIZE(toshiba_iio_accel_channels);
>  
> -		ret = iio_device_register(dev->indio_dev);
> +		ret = devm_iio_device_register(parent, dev->indio_dev);
>  		if (ret < 0) {
>  			pr_err("Unable to register iio device\n");
> -			iio_device_free(dev->indio_dev);
> +			dev->indio_dev = NULL;
>  		}
>  	}
>  iio_error: