Re: [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines

[Hans de Goede]

Hi Alexadru, Jonathan,

On 3/24/21 1:55 PM, Alexandru Ardelean wrote:
> This changeset tries to do a conversion of the toshiba_acpi driver to use
> only device-managed routines. The driver registers as a singleton, so no
> more than one device can be registered at a time.
> 
> My main intent here is to try to convert the iio_device_alloc() and
> iio_device_register() to their devm_ variants.
> 
> Usually, when converting a registration call to device-managed variant, the
> init order must be preserved. And the deregistration order must be a mirror
> of the registration (in reverse order).
> 
> This change tries to do that, by using devm_ variants where available and
> devm_add_action_or_reset() where this isn't possible.
> Some deregistration ordering is changed, because it wasn't exactly
> mirroring (in reverse) the init order.
> 
> For the IIO subsystem, the toshiba_acpi driver is the only user of
> iio_device_alloc(). If this changeset is accepted (after discussion), I
> will propose to remove the iio_device_alloc() function.
> 
> While I admit this may look like an overzealous effort to use devm_
> everywhere (in IIO at least), for me it's a fun/interesting excercise.

Alexadru, thank you for the patches.

Jonathan, thank you for the reviews.

To be honest I'm currently inclined to not accept / merge these patches,
this is based on 2 assumptions from me, which might be wrong. let me explain.

If I understand things correctly, the main reason for this rework of
the toshiba_acpi code is to move iio_device_alloc() and iio_device_register()
to their devm_ variants, converting the last users in the tree ?

This would allow these 2 iio functions to then be e.g. marked as static /
private helpers inside the iio core, so that all new users can only use
the devm_ versions. But if I'm reading Jonathan's reaction correctly then
Jonathan is not planning to do that because they might still be useful
in some cases.

Jonathan have I correctly understood that you don't plan to make any
changes to the iio_device_alloc() and iio_device_register() functions
even if this gets merged ?

Which brings me to my next assumption, Alexandru, I don't read anything
about testing anywhere. So I'm currently under the assumption that
you don't have any hardware using the toshiba_acpi driver and that this
is thus untested ?

The not being tested state is my main reason for not wanting to merge
this. The toshiba_acpi driver likely does not have a whole lot of users,
so the chances of someone running release candidates or even just the
latest kernels on hardware which uses it are small.  This means that if
we accidentally introduce a bug with this series it might not get caught
until say lots of people start upgrading to Ubuntu 22.04 which is
the first Ubuntu kernel with your changes; and then at least one of the
hit users needs to have the skills to find us and get in contact about that.

TL;DR: we might break stuff and if we do it might be a long time until we
find out we did and then we have been shipping broken code for ages...

Regards,

Hans





> 
> Alexandru Ardelean (10):
>   platform/x86: toshiba_acpi: bind life-time of toshiba_acpi_dev to
>     parent
>   platform/x86: toshiba_acpi: use devm_add_action_or_reset() for
>     singleton clear
>   platform/x86: toshiba_acpi: bind registration of miscdev object to
>     parent
>   platform/x86: toshiba_acpi: use device-managed functions for input
>     device
>   platform/x86: toshiba_acpi: register backlight with device-managed
>     variant
>   platform/x86: toshiba_acpi: use devm_led_classdev_register() for LEDs
>   platform/x86: toshiba_acpi: use device-managed functions for
>     accelerometer
>   platform/x86: toshiba_acpi: use device-managed for wwan_rfkill
>     management
>   platform/x86: toshiba_acpi: use device-managed for sysfs removal
>   platform/x86: toshiba_acpi: bind proc entries creation to parent
> 
>  drivers/platform/x86/toshiba_acpi.c | 249 +++++++++++++++++-----------
>  1 file changed, 150 insertions(+), 99 deletions(-)
>