linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Alexandru Ardelean <aardelean@deviqon.com>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Cc: coproscefalo@gmail.com, mgross@linux.intel.com, jic23@kernel.org,
	linux@deviqon.com
Subject: Re: [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines
Date: Tue, 30 Mar 2021 10:20:21 +0200	[thread overview]
Message-ID: <0fba6355-aaec-b214-cf12-1add08cfca38@redhat.com> (raw)
In-Reply-To: <20210324125548.45983-1-aardelean@deviqon.com>

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


  parent reply	other threads:[~2021-03-30  8:21 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 12:55 Alexandru Ardelean
2021-03-24 12:55 ` [PATCH 01/10] platform/x86: toshiba_acpi: bind life-time of toshiba_acpi_dev to parent Alexandru Ardelean
2021-03-29 14:30   ` Jonathan Cameron
2021-03-30  6:49     ` Alexandru Ardelean
2021-03-24 12:55 ` [PATCH 02/10] platform/x86: toshiba_acpi: use devm_add_action_or_reset() for singleton clear Alexandru Ardelean
2021-03-29 14:27   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 03/10] platform/x86: toshiba_acpi: bind registration of miscdev object to parent Alexandru Ardelean
2021-03-29 14:33   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 04/10] platform/x86: toshiba_acpi: use device-managed functions for input device Alexandru Ardelean
2021-03-29 14:48   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 05/10] platform/x86: toshiba_acpi: register backlight with device-managed variant Alexandru Ardelean
2021-03-29 14:50   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 06/10] platform/x86: toshiba_acpi: use devm_led_classdev_register() for LEDs Alexandru Ardelean
2021-03-29 14:52   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 07/10] platform/x86: toshiba_acpi: use device-managed functions for accelerometer Alexandru Ardelean
2021-03-29 14:54   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 08/10] platform/x86: toshiba_acpi: use device-managed for wwan_rfkill management Alexandru Ardelean
2021-03-29 14:57   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 09/10] platform/x86: toshiba_acpi: use device-managed for sysfs removal Alexandru Ardelean
2021-03-29 15:09   ` Jonathan Cameron
2021-03-24 12:55 ` [PATCH 10/10] platform/x86: toshiba_acpi: bind proc entries creation to parent Alexandru Ardelean
2021-03-29 15:10   ` Jonathan Cameron
2021-03-29 12:38 ` [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines Jonathan Cameron
2021-03-29 14:01   ` Alexandru Ardelean
2021-03-30  8:20 ` Hans de Goede [this message]
2021-03-30  9:22   ` Alexandru Ardelean
2021-03-30  9:27     ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0fba6355-aaec-b214-cf12-1add08cfca38@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=aardelean@deviqon.com \
    --cc=coproscefalo@gmail.com \
    --cc=jic23@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@deviqon.com \
    --cc=mgross@linux.intel.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --subject='Re: [PATCH 00/10] platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).