| Message ID | 20210324125548.45983-1-aardelean@deviqon.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | platform/x86: toshiba_acpi: move acpi add/remove to device-managed routines | expand |
On Wed, 24 Mar 2021 14:55:38 +0200 Alexandru Ardelean <aardelean@deviqon.com> 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. hmm. I am dubious about 'removing' the support for non devm_ in the long run because it can lead to requiring fiddly changes in existing drivers (like this one :) and I don't want to put that barrier in front of anyone using IIO. However, I'm more than happy to see them used in very few drivers and nice warning text added to suggest people might want to look at whether then can move to a device managed probe flow Jonathan > > 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(-) >
On Mon, 29 Mar 2021 at 15:38, Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 24 Mar 2021 14:55:38 +0200 > Alexandru Ardelean <aardelean@deviqon.com> 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. > hmm. I am dubious about 'removing' the support for non devm_ in the long > run because it can lead to requiring fiddly changes in existing drivers > (like this one :) and I don't want to put that barrier in front of anyone > using IIO. Yeah. I also feel that the current driver is a bit fiddly. I was undecided [when doing the series], whether to send it as a whole, or start with sending just a few patches that make sense on their own. I might go via the second route and send these individually. > > However, I'm more than happy to see them used in very few drivers and > nice warning text added to suggest people might want to look at whether > then can move to a device managed probe flow > > Jonathan > > > > > 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(-) > > >
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(-) >
On Tue, Mar 30, 2021 at 11:21 AM Hans de Goede <hdegoede@redhat.com> wrote: > > 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 ? yes well, we still have plenty of users iio_device_alloc() / iio_device_register() inside drivers/iio but the toshipa_acpi driver is the more quirky user of these functions [treewide] i wanted to jump on those simpler IIO cases, but i thought i would leave those to new contributors [for a while]; the complexity of those conversions is good enough to get some people started to contribute changes that are a bit more useful than checkpatch fixes, comment fixes [etc]; [personally] i feel that these devm_ conversions are complex enough to maybe get people wanting to dig more into some kernel design stuff > > 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 ? yes, i don't have any hw to test this > > 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... ack well, i don't insist in pushing this series; another thought was to just send bits of this set, which are simple enough to consider even on their own; maybe i'll look for a toshiba laptop with support for this stuff; i'll see thanks :) Alex > > 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(-) > > >
Hi, On 3/30/21 11:22 AM, Alexandru Ardelean wrote: > On Tue, Mar 30, 2021 at 11:21 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> 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 ? > > yes > well, we still have plenty of users iio_device_alloc() / > iio_device_register() inside drivers/iio > > but the toshipa_acpi driver is the more quirky user of these functions > [treewide] > > i wanted to jump on those simpler IIO cases, but i thought i would > leave those to new contributors [for a while]; > the complexity of those conversions is good enough to get some people > started to contribute changes that are a bit more useful than > checkpatch fixes, comment fixes [etc]; > > [personally] i feel that these devm_ conversions are complex enough to > maybe get people wanting to dig more into some kernel design stuff I like how you think about onboarding new people. >> 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 ? > > yes, i don't have any hw to test this > >> >> 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... > > ack > well, i don't insist in pushing this series; Ok, lets park this series then for now, because IMHO it is just a tad too complex to merge without it being tested (and without another important reason like it being part of some larger cleanup / refactoring). 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(-) >>> >> >
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. 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(-)