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(-)
--
2.30.2
The 'toshiba_acpi_dev' object is allocated first and free'd last. We can
bind it's life-time to the parent ACPI device object. This is a first step
in using more device-managed allocated functions for this.
The main intent is to try to convert the IIO framework to export only
device-managed functions (i.e. devm_iio_device_alloc() and
devm_iio_device_register()). It's still not 100% sure that this is
possible, but for now, this is the process of taking it slowly in that
direction.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index fa7232ad8c39..6d298810b7bf 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2998,8 +2998,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
if (toshiba_acpi)
toshiba_acpi = NULL;
- kfree(dev);
-
return 0;
}
@@ -3016,6 +3014,7 @@ static const char *find_hci_method(acpi_handle handle)
static int toshiba_acpi_add(struct acpi_device *acpi_dev)
{
+ struct device *parent = &acpi_dev->dev;
struct toshiba_acpi_dev *dev;
const char *hci_method;
u32 dummy;
@@ -3033,7 +3032,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
return -ENODEV;
}
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ dev = devm_kzalloc(parent, sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
dev->acpi_dev = acpi_dev;
@@ -3045,7 +3044,6 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
ret = misc_register(&dev->miscdev);
if (ret) {
pr_err("Failed to register miscdevice\n");
- kfree(dev);
return ret;
}
--
2.30.2
This change moves the registration of the Toshiba ACPI miscdev to be
handled by the devm_add_action_or_reset() hook. This way, the miscdev will
be unregistered when the reference count of the parent device object goes
to zero.
This also changes the order of cleanup in toshiba_acpi_remove(), where the
miscdev was deregistered first. Now it will be deregistered right before
the toshiba_acpi_dev object is free'd.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index c5284601bc2a..53ef565378ef 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2963,8 +2963,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
{
struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
- misc_deregister(&dev->miscdev);
-
remove_toshiba_proc_entries(dev);
if (dev->accelerometer_supported && dev->indio_dev) {
@@ -3014,6 +3012,13 @@ static void toshiba_acpi_singleton_clear(void *data)
toshiba_acpi = NULL;
}
+static void toshiba_acpi_misc_deregister(void *data)
+{
+ struct miscdevice *miscdev = data;
+
+ misc_deregister(miscdev);
+}
+
static int toshiba_acpi_add(struct acpi_device *acpi_dev)
{
struct device *parent = &acpi_dev->dev;
@@ -3056,6 +3061,11 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
return ret;
}
+ ret = devm_add_action_or_reset(parent, toshiba_acpi_misc_deregister,
+ &dev->miscdev);
+ if (ret)
+ return ret;
+
acpi_dev->driver_data = dev;
dev_set_drvdata(&acpi_dev->dev, dev);
--
2.30.2
This change converts the registration of the backlight data with the
devm_backlight_device_register() function.
This way, the backlight_device_unregister() call is no longer required, and
the order of deregistration is made to be more symmetrical with the
registration order.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 556f2cc99bad..ada2a2d8c913 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2876,7 +2876,8 @@ static int toshiba_acpi_setup_keyboard(struct device *parent,
return error;
}
-static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
+static int toshiba_acpi_setup_backlight(struct device *parent,
+ struct toshiba_acpi_dev *dev)
{
struct backlight_properties props;
int brightness;
@@ -2924,11 +2925,12 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
if (dev->tr_backlight_supported)
props.max_brightness++;
- dev->backlight_dev = backlight_device_register("toshiba",
- &dev->acpi_dev->dev,
- dev,
- &toshiba_backlight_data,
- &props);
+ dev->backlight_dev = devm_backlight_device_register(parent,
+ "toshiba",
+ &dev->acpi_dev->dev,
+ dev,
+ &toshiba_backlight_data,
+ &props);
if (IS_ERR(dev->backlight_dev)) {
ret = PTR_ERR(dev->backlight_dev);
pr_err("Could not register toshiba backlight device\n");
@@ -2999,8 +3001,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
sysfs_remove_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
- backlight_device_unregister(dev->backlight_dev);
-
led_classdev_unregister(&dev->led_dev);
led_classdev_unregister(&dev->kbd_led);
led_classdev_unregister(&dev->eco_led);
@@ -3104,9 +3104,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
ret = get_tr_backlight_status(dev, &dummy);
dev->tr_backlight_supported = !ret;
- ret = toshiba_acpi_setup_backlight(dev);
+ ret = toshiba_acpi_setup_backlight(parent, dev);
if (ret)
- goto error;
+ return ret;
toshiba_illumination_available(dev);
if (dev->illumination_supported) {
--
2.30.2
This change binds the creation of the proc entries to the parent object,
via the devm_add_action_or_reset() call.
This way when the parent object's refcount goes to zero, the proc entries
are removed in the reverse other in which they were created.
This is the last bit of the toshiba_acpi_remove() function, so in this
change this function is removed.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 45 ++++++++++++++---------------
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 8e8917979047..56ee5cd1e90c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1671,7 +1671,23 @@ static int __maybe_unused version_proc_show(struct seq_file *m, void *v)
#define PROC_TOSHIBA "toshiba"
-static void create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
+static void remove_toshiba_proc_entries(void *data)
+{
+ struct toshiba_acpi_dev *dev = data;
+
+ if (dev->backlight_dev)
+ remove_proc_entry("lcd", toshiba_proc_dir);
+ if (dev->video_supported)
+ remove_proc_entry("video", toshiba_proc_dir);
+ if (dev->fan_supported)
+ remove_proc_entry("fan", toshiba_proc_dir);
+ if (dev->hotkey_dev)
+ remove_proc_entry("keys", toshiba_proc_dir);
+ remove_proc_entry("version", toshiba_proc_dir);
+}
+
+static int create_toshiba_proc_entries(struct device *parent,
+ struct toshiba_acpi_dev *dev)
{
if (dev->backlight_dev)
proc_create_data("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir,
@@ -1687,19 +1703,8 @@ static void create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
&keys_proc_ops, dev);
proc_create_single_data("version", S_IRUGO, toshiba_proc_dir,
version_proc_show, dev);
-}
-static void remove_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
-{
- if (dev->backlight_dev)
- remove_proc_entry("lcd", toshiba_proc_dir);
- if (dev->video_supported)
- remove_proc_entry("video", toshiba_proc_dir);
- if (dev->fan_supported)
- remove_proc_entry("fan", toshiba_proc_dir);
- if (dev->hotkey_dev)
- remove_proc_entry("keys", toshiba_proc_dir);
- remove_proc_entry("version", toshiba_proc_dir);
+ return devm_add_action_or_reset(parent, remove_toshiba_proc_entries, dev);
}
static const struct backlight_ops toshiba_backlight_data = {
@@ -3012,15 +3017,6 @@ static void print_supported_features(struct toshiba_acpi_dev *dev)
pr_cont("\n");
}
-static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
-{
- struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
-
- remove_toshiba_proc_entries(dev);
-
- return 0;
-}
-
static const char *find_hci_method(acpi_handle handle)
{
if (acpi_has_method(handle, "GHCI"))
@@ -3230,7 +3226,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
if (ret)
return ret;
- create_toshiba_proc_entries(dev);
+ ret = create_toshiba_proc_entries(parent, dev);
+ if (ret)
+ return ret;
toshiba_acpi = dev;
@@ -3340,7 +3338,6 @@ static struct acpi_driver toshiba_acpi_driver = {
.flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
.ops = {
.add = toshiba_acpi_add,
- .remove = toshiba_acpi_remove,
.notify = toshiba_acpi_notify,
},
.drv.pm = &toshiba_acpi_pm,
--
2.30.2
With this change the deregistration of the LED objects is made symmetrical
(and in reverse) with the registration. We also can get rid of the calls
to led_classdev_unregister(), because the LED objects will be cleaned up
when the reference to the parent device object goes to zero.
This change also unifies the reference to the parent object from
'&acpi_dev->dev' and '&dev->acpi_dev->dev' to 'parent', since it's the same
reference, and makes the code-lines a bit shorter.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index ada2a2d8c913..e787c140eec2 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -3001,10 +3001,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
sysfs_remove_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
- led_classdev_unregister(&dev->led_dev);
- led_classdev_unregister(&dev->kbd_led);
- led_classdev_unregister(&dev->eco_led);
-
if (dev->wwan_rfk) {
rfkill_unregister(dev->wwan_rfk);
rfkill_destroy(dev->wwan_rfk);
@@ -3114,7 +3110,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->led_dev.max_brightness = 1;
dev->led_dev.brightness_set = toshiba_illumination_set;
dev->led_dev.brightness_get = toshiba_illumination_get;
- led_classdev_register(&acpi_dev->dev, &dev->led_dev);
+ ret = devm_led_classdev_register(parent, &dev->led_dev);
+ if (ret)
+ return ret;
}
toshiba_eco_mode_available(dev);
@@ -3123,7 +3121,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->eco_led.max_brightness = 1;
dev->eco_led.brightness_set = toshiba_eco_mode_set_status;
dev->eco_led.brightness_get = toshiba_eco_mode_get_status;
- led_classdev_register(&dev->acpi_dev->dev, &dev->eco_led);
+ ret = devm_led_classdev_register(parent, &dev->eco_led);
+ if (ret)
+ return ret;
}
toshiba_kbd_illum_available(dev);
@@ -3139,7 +3139,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->kbd_led.max_brightness = 1;
dev->kbd_led.brightness_set = toshiba_kbd_backlight_set;
dev->kbd_led.brightness_get = toshiba_kbd_backlight_get;
- led_classdev_register(&dev->acpi_dev->dev, &dev->kbd_led);
+ ret = devm_led_classdev_register(parent, &dev->kbd_led);
+ if (ret)
+ return ret;
}
ret = toshiba_touchpad_get(dev, &dummy);
--
2.30.2
The only reason to do this is to enforce the ordering of deinitialization,
when the conversion of the device-managed functions is done.
The singleton object should be cleared right before it is free'd.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 6d298810b7bf..c5284601bc2a 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2995,9 +2995,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
rfkill_destroy(dev->wwan_rfk);
}
- if (toshiba_acpi)
- toshiba_acpi = NULL;
-
return 0;
}
@@ -3012,6 +3009,11 @@ static const char *find_hci_method(acpi_handle handle)
return NULL;
}
+static void toshiba_acpi_singleton_clear(void *data)
+{
+ toshiba_acpi = NULL;
+}
+
static int toshiba_acpi_add(struct acpi_device *acpi_dev)
{
struct device *parent = &acpi_dev->dev;
@@ -3035,6 +3037,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev = devm_kzalloc(parent, sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
+
+ ret = devm_add_action_or_reset(parent,
+ toshiba_acpi_singleton_clear,
+ NULL);
+ if (ret)
+ return ret;
+
dev->acpi_dev = acpi_dev;
dev->method_hci = hci_method;
dev->miscdev.minor = MISC_DYNAMIC_MINOR;
--
2.30.2
This change uses device managed functions to handle the deregistration of
the keyboard resources when the refcount of the parent device goes to zero.
For the input device devm_input_allocate_device() must be used, and after
that it will be bound also for auto-deregistration when
input_device_register() is called.
The work object is registered for uninit with devm_add_action(), which will
be called on device unregister only.
The i8042 filter is registered with devm_add_action() as well, but it is
done last in the toshiba_acpi_setup_keyboard() function. This is a little
quirky, because this relies on the fact that there can a single
toshiba_acpi_dev object.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 55 +++++++++++++++++++----------
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 53ef565378ef..556f2cc99bad 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -186,7 +186,6 @@ struct toshiba_acpi_dev {
unsigned int video_supported:1;
unsigned int fan_supported:1;
unsigned int system_event_supported:1;
- unsigned int ntfy_supported:1;
unsigned int info_supported:1;
unsigned int tr_backlight_supported:1;
unsigned int kbd_illum_supported:1;
@@ -2756,9 +2755,23 @@ static void toshiba_acpi_process_hotkeys(struct toshiba_acpi_dev *dev)
}
}
-static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
+static void toshiba_acpi_cancel_hotkey_work(void *data)
+{
+ struct work_struct *hotkey_work = data;
+
+ cancel_work_sync(hotkey_work);
+}
+
+static void toshiba_acpi_i8042_remove_filter(void *data)
+{
+ i8042_remove_filter(toshiba_acpi_i8042_filter);
+}
+
+static int toshiba_acpi_setup_keyboard(struct device *parent,
+ struct toshiba_acpi_dev *dev)
{
const struct key_entry *keymap = toshiba_acpi_keymap;
+ bool ntfy_supported = false;
acpi_handle ec_handle;
int error;
@@ -2779,7 +2792,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
if (toshiba_hotkey_event_type_get(dev, &dev->hotkey_event_type))
pr_notice("Unable to query Hotkey Event Type\n");
- dev->hotkey_dev = input_allocate_device();
+ dev->hotkey_dev = devm_input_allocate_device(parent);
if (!dev->hotkey_dev)
return -ENOMEM;
@@ -2798,7 +2811,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
dev->hotkey_event_type);
error = sparse_keymap_setup(dev->hotkey_dev, keymap, NULL);
if (error)
- goto err_free_dev;
+ goto err_null_dev;
/*
* For some machines the SCI responsible for providing hotkey
@@ -2811,13 +2824,19 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
if (ec_handle && acpi_has_method(ec_handle, "NTFY")) {
INIT_WORK(&dev->hotkey_work, toshiba_acpi_hotkey_work);
+ error = devm_add_action(parent,
+ toshiba_acpi_cancel_hotkey_work,
+ &dev->hotkey_work);
+ if (error)
+ return error;
+
error = i8042_install_filter(toshiba_acpi_i8042_filter);
if (error) {
pr_err("Error installing key filter\n");
- goto err_free_dev;
+ return error;
}
- dev->ntfy_supported = 1;
+ ntfy_supported = true;
}
/*
@@ -2840,13 +2859,19 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
goto err_remove_filter;
}
+ if (ntfy_supported) {
+ error = devm_add_action(parent,
+ toshiba_acpi_i8042_remove_filter,
+ NULL);
+ goto err_remove_filter;
+ }
+
return 0;
- err_remove_filter:
- if (dev->ntfy_supported)
+err_remove_filter:
+ if (ntfy_supported)
i8042_remove_filter(toshiba_acpi_i8042_filter);
- err_free_dev:
- input_free_device(dev->hotkey_dev);
+err_null_dev:
dev->hotkey_dev = NULL;
return error;
}
@@ -2974,14 +2999,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
sysfs_remove_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
- if (dev->ntfy_supported) {
- i8042_remove_filter(toshiba_acpi_i8042_filter);
- cancel_work_sync(&dev->hotkey_work);
- }
-
- if (dev->hotkey_dev)
- input_unregister_device(dev->hotkey_dev);
-
backlight_device_unregister(dev->backlight_dev);
led_classdev_unregister(&dev->led_dev);
@@ -3080,7 +3097,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->kbd_function_keys_supported = !ret;
dev->hotkey_event_type = 0;
- if (toshiba_acpi_setup_keyboard(dev))
+ if (toshiba_acpi_setup_keyboard(parent, dev))
pr_info("Unable to activate hotkeys\n");
/* Determine whether or not BIOS supports transflective backlight */
--
2.30.2
This change converts the wwan_rfkill object to be free'd automatically when
the parent refcount goes to zero.
There are 2 cleanup operations required: rfkill_unregister() and
rfkill_destroy(). Since they don't have any devm_ variants, they are hooked
via devm_add_action_or_reset().
The main reason to do this is to enforce ordering on cleanup, when the
Toshiba ACPI device is cleaned up.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 40 ++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 12860ef60e4d..a1249f6dde9a 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -2591,7 +2591,22 @@ static const struct rfkill_ops wwan_rfk_ops = {
.poll = toshiba_acpi_wwan_poll,
};
-static int toshiba_acpi_setup_wwan_rfkill(struct toshiba_acpi_dev *dev)
+static void toshiba_acpi_rfkill_destroy(void *data)
+{
+ struct rfkill *wwan_rfk = data;
+
+ rfkill_destroy(wwan_rfk);
+}
+
+static void toshiba_acpi_rfkill_unreg(void *data)
+{
+ struct rfkill *wwan_rfk = data;
+
+ rfkill_unregister(wwan_rfk);
+}
+
+static int toshiba_acpi_setup_wwan_rfkill(struct device *parent,
+ struct toshiba_acpi_dev *dev)
{
int ret = toshiba_wireless_status(dev);
@@ -2608,15 +2623,27 @@ static int toshiba_acpi_setup_wwan_rfkill(struct toshiba_acpi_dev *dev)
return -ENOMEM;
}
+ ret = devm_add_action_or_reset(parent, toshiba_acpi_rfkill_destroy,
+ dev->wwan_rfk);
+ if (ret)
+ return ret;
+
rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
ret = rfkill_register(dev->wwan_rfk);
if (ret) {
pr_err("Unable to register WWAN rfkill device\n");
- rfkill_destroy(dev->wwan_rfk);
+ return ret;
}
- return ret;
+ ret = devm_add_action_or_reset(parent, toshiba_acpi_rfkill_unreg,
+ dev->wwan_rfk);
+ if (ret) {
+ dev->wwan_rfk = NULL;
+ return ret;
+ }
+
+ return 0;
}
/*
@@ -2996,11 +3023,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
sysfs_remove_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
- if (dev->wwan_rfk) {
- rfkill_unregister(dev->wwan_rfk);
- rfkill_destroy(dev->wwan_rfk);
- }
-
return 0;
}
@@ -3189,7 +3211,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
toshiba_wwan_available(dev);
if (dev->wwan_supported)
- toshiba_acpi_setup_wwan_rfkill(dev);
+ toshiba_acpi_setup_wwan_rfkill(parent, dev);
toshiba_cooling_method_available(dev);
--
2.30.2
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.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
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:
--
2.30.2
This change moves the creation of the Toshiba ACPI group to be
automatically removed when the parent refcount goes to zero.
The main reason to do this, is to also enforce that the order of removal is
mirroring the order of initialization.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a1249f6dde9a..8e8917979047 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -200,7 +200,6 @@ struct toshiba_acpi_dev {
unsigned int usb_three_supported:1;
unsigned int wwan_supported:1;
unsigned int cooling_method_supported:1;
- unsigned int sysfs_created:1;
unsigned int special_functions;
bool kbd_event_generated;
@@ -3019,10 +3018,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
remove_toshiba_proc_entries(dev);
- if (dev->sysfs_created)
- sysfs_remove_group(&dev->acpi_dev->dev.kobj,
- &toshiba_attr_group);
-
return 0;
}
@@ -3049,6 +3044,13 @@ static void toshiba_acpi_misc_deregister(void *data)
misc_deregister(miscdev);
}
+static void toshiba_acpi_sysfs_remove(void *data)
+{
+ struct kobject *kobj = data;
+
+ sysfs_remove_group(kobj, &toshiba_attr_group);
+}
+
static int toshiba_acpi_add(struct acpi_device *acpi_dev)
{
struct device *parent = &acpi_dev->dev;
@@ -3219,21 +3221,20 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
ret = sysfs_create_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
- if (ret) {
- dev->sysfs_created = 0;
- goto error;
- }
- dev->sysfs_created = !ret;
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(parent,
+ toshiba_acpi_sysfs_remove,
+ &dev->acpi_dev->dev.kobj);
+ if (ret)
+ return ret;
create_toshiba_proc_entries(dev);
toshiba_acpi = dev;
return 0;
-
-error:
- toshiba_acpi_remove(acpi_dev);
- return ret;
}
static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
--
2.30.2
On Wed, 24 Mar 2021 14:55:38 +0200
Alexandru Ardelean <[email protected]> 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 <[email protected]> wrote:
>
> On Wed, 24 Mar 2021 14:55:38 +0200
> Alexandru Ardelean <[email protected]> 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(-)
> >
>
On Wed, 24 Mar 2021 14:55:40 +0200
Alexandru Ardelean <[email protected]> wrote:
> The only reason to do this is to enforce the ordering of deinitialization,
> when the conversion of the device-managed functions is done.
>
> The singleton object should be cleared right before it is free'd.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Whilst this might help get towards your goal, I'm curious to why
this singleton actually needs to exist in the first place.
It doesn't feel like it would be very hard to remove.
1) embed the kbd_bl_work in struct toshiba_acpi_dev with appropriate changes to it's init.
use container_of() magic to get to that.
2) For the toshiba_iio_accel_read_raw() stash a copy of the pointer in iio_priv(indio_dev)
Note that I'm very suspicious of existing sizeing of the private region.
3) For miscdevice handling you should be able to use container_of() to get to the
toshiba_acpi_dev structure from file->private_data
There are a few places where the struct toshiba_acpi_dev will need passing into functions
that currently get it from the global, plus one place where I'm fairly sure an element
of that structure gets set twice in a row via different copies of the pointer.
Also nice to not use dev as the name for struct toshiba_acpi_dev * as that is
makes for some confusing code when we have a bunch of struct device * in use
as well.
So this is fine, but kind of feels like the code shouldn't be there in the first place!
Jonathan
> ---
> drivers/platform/x86/toshiba_acpi.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 6d298810b7bf..c5284601bc2a 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2995,9 +2995,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> rfkill_destroy(dev->wwan_rfk);
> }
>
> - if (toshiba_acpi)
> - toshiba_acpi = NULL;
> -
> return 0;
> }
>
> @@ -3012,6 +3009,11 @@ static const char *find_hci_method(acpi_handle handle)
> return NULL;
> }
>
> +static void toshiba_acpi_singleton_clear(void *data)
> +{
> + toshiba_acpi = NULL;
> +}
> +
> static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> {
> struct device *parent = &acpi_dev->dev;
> @@ -3035,6 +3037,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> dev = devm_kzalloc(parent, sizeof(*dev), GFP_KERNEL);
> if (!dev)
> return -ENOMEM;
> +
> + ret = devm_add_action_or_reset(parent,
> + toshiba_acpi_singleton_clear,
> + NULL);
> + if (ret)
> + return ret;
> +
> dev->acpi_dev = acpi_dev;
> dev->method_hci = hci_method;
> dev->miscdev.minor = MISC_DYNAMIC_MINOR;
On Wed, 24 Mar 2021 14:55:39 +0200
Alexandru Ardelean <[email protected]> wrote:
> The 'toshiba_acpi_dev' object is allocated first and free'd last. We can
> bind it's life-time to the parent ACPI device object. This is a first step
> in using more device-managed allocated functions for this.
>
> The main intent is to try to convert the IIO framework to export only
> device-managed functions (i.e. devm_iio_device_alloc() and
> devm_iio_device_register()). It's still not 100% sure that this is
> possible, but for now, this is the process of taking it slowly in that
> direction.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Might just be me, but naming anything dev that isn't a struct device *
is downright confusing?
> ---
> drivers/platform/x86/toshiba_acpi.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index fa7232ad8c39..6d298810b7bf 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2998,8 +2998,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> if (toshiba_acpi)
> toshiba_acpi = NULL;
>
> - kfree(dev);
> -
> return 0;
> }
>
> @@ -3016,6 +3014,7 @@ static const char *find_hci_method(acpi_handle handle)
>
> static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> {
> + struct device *parent = &acpi_dev->dev;
> struct toshiba_acpi_dev *dev;
> const char *hci_method;
> u32 dummy;
> @@ -3033,7 +3032,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> return -ENODEV;
> }
>
> - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + dev = devm_kzalloc(parent, sizeof(*dev), GFP_KERNEL);
> if (!dev)
> return -ENOMEM;
> dev->acpi_dev = acpi_dev;
> @@ -3045,7 +3044,6 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> ret = misc_register(&dev->miscdev);
> if (ret) {
> pr_err("Failed to register miscdevice\n");
> - kfree(dev);
> return ret;
> }
>
On Wed, 24 Mar 2021 14:55:41 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change moves the registration of the Toshiba ACPI miscdev to be
> handled by the devm_add_action_or_reset() hook. This way, the miscdev will
> be unregistered when the reference count of the parent device object goes
> to zero.
>
> This also changes the order of cleanup in toshiba_acpi_remove(), where the
> miscdev was deregistered first. Now it will be deregistered right before
> the toshiba_acpi_dev object is free'd.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Reorder looks right to me, but maybe I'm missing something subtle.
Acked-by: Jonathan Cameron <[email protected]>
One unrelated comment inline.
Jonathan
> ---
> drivers/platform/x86/toshiba_acpi.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index c5284601bc2a..53ef565378ef 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2963,8 +2963,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> {
> struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
>
> - misc_deregister(&dev->miscdev);
> -
> remove_toshiba_proc_entries(dev);
>
> if (dev->accelerometer_supported && dev->indio_dev) {
> @@ -3014,6 +3012,13 @@ static void toshiba_acpi_singleton_clear(void *data)
> toshiba_acpi = NULL;
> }
>
> +static void toshiba_acpi_misc_deregister(void *data)
> +{
> + struct miscdevice *miscdev = data;
> +
> + misc_deregister(miscdev);
> +}
> +
> static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> {
> struct device *parent = &acpi_dev->dev;
> @@ -3056,6 +3061,11 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> return ret;
> }
>
> + ret = devm_add_action_or_reset(parent, toshiba_acpi_misc_deregister,
> + &dev->miscdev);
> + if (ret)
> + return ret;
> +
> acpi_dev->driver_data = dev;
> dev_set_drvdata(&acpi_dev->dev, dev);
Why are we carrying two copies of the same thing? (obviously unrelated
to your patch :)
>
On Wed, 24 Mar 2021 14:55:42 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change uses device managed functions to handle the deregistration of
> the keyboard resources when the refcount of the parent device goes to zero.
>
> For the input device devm_input_allocate_device() must be used, and after
> that it will be bound also for auto-deregistration when
> input_device_register() is called.
>
> The work object is registered for uninit with devm_add_action(), which will
> be called on device unregister only.
>
> The i8042 filter is registered with devm_add_action() as well, but it is
> done last in the toshiba_acpi_setup_keyboard() function. This is a little
> quirky, because this relies on the fact that there can a single
> toshiba_acpi_dev object.
I think this is the wrong approach here. Given function can fail
without probe() returning an error, go up one level and treat this whole
function as something to be device managed. That way you can register
the unwind function only if the call in probe succeeds.
Jonathan
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 55 +++++++++++++++++++----------
> 1 file changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 53ef565378ef..556f2cc99bad 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -186,7 +186,6 @@ struct toshiba_acpi_dev {
> unsigned int video_supported:1;
> unsigned int fan_supported:1;
> unsigned int system_event_supported:1;
> - unsigned int ntfy_supported:1;
> unsigned int info_supported:1;
> unsigned int tr_backlight_supported:1;
> unsigned int kbd_illum_supported:1;
> @@ -2756,9 +2755,23 @@ static void toshiba_acpi_process_hotkeys(struct toshiba_acpi_dev *dev)
> }
> }
>
> -static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
> +static void toshiba_acpi_cancel_hotkey_work(void *data)
> +{
> + struct work_struct *hotkey_work = data;
> +
> + cancel_work_sync(hotkey_work);
> +}
> +
> +static void toshiba_acpi_i8042_remove_filter(void *data)
> +{
> + i8042_remove_filter(toshiba_acpi_i8042_filter);
> +}
> +
> +static int toshiba_acpi_setup_keyboard(struct device *parent,
> + struct toshiba_acpi_dev *dev)
> {
> const struct key_entry *keymap = toshiba_acpi_keymap;
> + bool ntfy_supported = false;
> acpi_handle ec_handle;
> int error;
>
> @@ -2779,7 +2792,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
> if (toshiba_hotkey_event_type_get(dev, &dev->hotkey_event_type))
> pr_notice("Unable to query Hotkey Event Type\n");
>
> - dev->hotkey_dev = input_allocate_device();
> + dev->hotkey_dev = devm_input_allocate_device(parent);
Hmm. Doing this within a function that can fail without resulting in driver probe failure
is rather messy. I think you should call a manual free so this isn't left around until
driver remove.
Or decide this function is too complex and do the devm_ management one layer out using
a devm_add_action_or_reset(dev, toshiba_acpi_unsetup_keyboard, *) or similar to do
it in one go. That would only be registered if this function succeeded in the first place.
> if (!dev->hotkey_dev)
> return -ENOMEM;
>
> @@ -2798,7 +2811,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
> dev->hotkey_event_type);
> error = sparse_keymap_setup(dev->hotkey_dev, keymap, NULL);
> if (error)
> - goto err_free_dev;
> + goto err_null_dev;
>
> /*
> * For some machines the SCI responsible for providing hotkey
> @@ -2811,13 +2824,19 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
> if (ec_handle && acpi_has_method(ec_handle, "NTFY")) {
> INIT_WORK(&dev->hotkey_work, toshiba_acpi_hotkey_work);
>
> + error = devm_add_action(parent,
> + toshiba_acpi_cancel_hotkey_work,
> + &dev->hotkey_work);
> + if (error)
> + return error;
> +
> error = i8042_install_filter(toshiba_acpi_i8042_filter);
> if (error) {
> pr_err("Error installing key filter\n");
> - goto err_free_dev;
> + return error;
Does this not want to do goto err_null_dev?
Same with the one above.
> }
>
> - dev->ntfy_supported = 1;
> + ntfy_supported = true;
> }
>
> /*
> @@ -2840,13 +2859,19 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
> goto err_remove_filter;
> }
>
> + if (ntfy_supported) {
> + error = devm_add_action(parent,
> + toshiba_acpi_i8042_remove_filter,
> + NULL);
> + goto err_remove_filter;
> + }
> +
> return 0;
>
> - err_remove_filter:
> - if (dev->ntfy_supported)
> +err_remove_filter:
> + if (ntfy_supported)
> i8042_remove_filter(toshiba_acpi_i8042_filter);
> - err_free_dev:
> - input_free_device(dev->hotkey_dev);
> +err_null_dev:
> dev->hotkey_dev = NULL;
How about, having a local hotkey_dev variable and only setting
it in the good path just before returning? That way we can return
directly above and have a cleaner flow.
> return error;
> }
> @@ -2974,14 +2999,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> sysfs_remove_group(&dev->acpi_dev->dev.kobj,
> &toshiba_attr_group);
>
> - if (dev->ntfy_supported) {
> - i8042_remove_filter(toshiba_acpi_i8042_filter);
> - cancel_work_sync(&dev->hotkey_work);
> - }
> -
> - if (dev->hotkey_dev)
> - input_unregister_device(dev->hotkey_dev);
> -
> backlight_device_unregister(dev->backlight_dev);
>
> led_classdev_unregister(&dev->led_dev);
> @@ -3080,7 +3097,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> dev->kbd_function_keys_supported = !ret;
>
> dev->hotkey_event_type = 0;
> - if (toshiba_acpi_setup_keyboard(dev))
> + if (toshiba_acpi_setup_keyboard(parent, dev))
> pr_info("Unable to activate hotkeys\n");
>
> /* Determine whether or not BIOS supports transflective backlight */
On Wed, 24 Mar 2021 14:55:44 +0200
Alexandru Ardelean <[email protected]> wrote:
> With this change the deregistration of the LED objects is made symmetrical
> (and in reverse) with the registration. We also can get rid of the calls
> to led_classdev_unregister(), because the LED objects will be cleaned up
> when the reference to the parent device object goes to zero.
>
> This change also unifies the reference to the parent object from
> '&acpi_dev->dev' and '&dev->acpi_dev->dev' to 'parent', since it's the same
> reference, and makes the code-lines a bit shorter.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index ada2a2d8c913..e787c140eec2 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -3001,10 +3001,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> sysfs_remove_group(&dev->acpi_dev->dev.kobj,
> &toshiba_attr_group);
>
> - led_classdev_unregister(&dev->led_dev);
> - led_classdev_unregister(&dev->kbd_led);
> - led_classdev_unregister(&dev->eco_led);
> -
> if (dev->wwan_rfk) {
> rfkill_unregister(dev->wwan_rfk);
> rfkill_destroy(dev->wwan_rfk);
> @@ -3114,7 +3110,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> dev->led_dev.max_brightness = 1;
> dev->led_dev.brightness_set = toshiba_illumination_set;
> dev->led_dev.brightness_get = toshiba_illumination_get;
> - led_classdev_register(&acpi_dev->dev, &dev->led_dev);
> + ret = devm_led_classdev_register(parent, &dev->led_dev);
> + if (ret)
> + return ret;
> }
>
> toshiba_eco_mode_available(dev);
> @@ -3123,7 +3121,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> dev->eco_led.max_brightness = 1;
> dev->eco_led.brightness_set = toshiba_eco_mode_set_status;
> dev->eco_led.brightness_get = toshiba_eco_mode_get_status;
> - led_classdev_register(&dev->acpi_dev->dev, &dev->eco_led);
> + ret = devm_led_classdev_register(parent, &dev->eco_led);
> + if (ret)
> + return ret;
> }
>
> toshiba_kbd_illum_available(dev);
> @@ -3139,7 +3139,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> dev->kbd_led.max_brightness = 1;
> dev->kbd_led.brightness_set = toshiba_kbd_backlight_set;
> dev->kbd_led.brightness_get = toshiba_kbd_backlight_get;
> - led_classdev_register(&dev->acpi_dev->dev, &dev->kbd_led);
> + ret = devm_led_classdev_register(parent, &dev->kbd_led);
> + if (ret)
> + return ret;
> }
>
> ret = toshiba_touchpad_get(dev, &dummy);
On Wed, 24 Mar 2021 14:55:43 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change converts the registration of the backlight data with the
> devm_backlight_device_register() function.
> This way, the backlight_device_unregister() call is no longer required, and
> the order of deregistration is made to be more symmetrical with the
> registration order.
>
This one looks fine to me.
> Signed-off-by: Alexandru Ardelean <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 556f2cc99bad..ada2a2d8c913 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2876,7 +2876,8 @@ static int toshiba_acpi_setup_keyboard(struct device *parent,
> return error;
> }
>
> -static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
> +static int toshiba_acpi_setup_backlight(struct device *parent,
> + struct toshiba_acpi_dev *dev)
> {
> struct backlight_properties props;
> int brightness;
> @@ -2924,11 +2925,12 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
> if (dev->tr_backlight_supported)
> props.max_brightness++;
>
> - dev->backlight_dev = backlight_device_register("toshiba",
> - &dev->acpi_dev->dev,
> - dev,
> - &toshiba_backlight_data,
> - &props);
> + dev->backlight_dev = devm_backlight_device_register(parent,
> + "toshiba",
> + &dev->acpi_dev->dev,
> + dev,
> + &toshiba_backlight_data,
> + &props);
> if (IS_ERR(dev->backlight_dev)) {
> ret = PTR_ERR(dev->backlight_dev);
> pr_err("Could not register toshiba backlight device\n");
> @@ -2999,8 +3001,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> sysfs_remove_group(&dev->acpi_dev->dev.kobj,
> &toshiba_attr_group);
>
> - backlight_device_unregister(dev->backlight_dev);
> -
> led_classdev_unregister(&dev->led_dev);
> led_classdev_unregister(&dev->kbd_led);
> led_classdev_unregister(&dev->eco_led);
> @@ -3104,9 +3104,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> ret = get_tr_backlight_status(dev, &dummy);
> dev->tr_backlight_supported = !ret;
>
> - ret = toshiba_acpi_setup_backlight(dev);
> + ret = toshiba_acpi_setup_backlight(parent, dev);
> if (ret)
> - goto error;
> + return ret;
>
> toshiba_illumination_available(dev);
> if (dev->illumination_supported) {
On Wed, 24 Mar 2021 14:55:45 +0200
Alexandru Ardelean <[email protected]> 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 <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> 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:
On Wed, 24 Mar 2021 14:55:46 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change converts the wwan_rfkill object to be free'd automatically when
> the parent refcount goes to zero.
> There are 2 cleanup operations required: rfkill_unregister() and
> rfkill_destroy(). Since they don't have any devm_ variants, they are hooked
> via devm_add_action_or_reset().
>
> The main reason to do this is to enforce ordering on cleanup, when the
> Toshiba ACPI device is cleaned up.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Superficially looks fine to me though I don't know much about rfkill.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 40 ++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 12860ef60e4d..a1249f6dde9a 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2591,7 +2591,22 @@ static const struct rfkill_ops wwan_rfk_ops = {
> .poll = toshiba_acpi_wwan_poll,
> };
>
> -static int toshiba_acpi_setup_wwan_rfkill(struct toshiba_acpi_dev *dev)
> +static void toshiba_acpi_rfkill_destroy(void *data)
> +{
> + struct rfkill *wwan_rfk = data;
> +
> + rfkill_destroy(wwan_rfk);
> +}
> +
> +static void toshiba_acpi_rfkill_unreg(void *data)
> +{
> + struct rfkill *wwan_rfk = data;
> +
> + rfkill_unregister(wwan_rfk);
> +}
> +
> +static int toshiba_acpi_setup_wwan_rfkill(struct device *parent,
> + struct toshiba_acpi_dev *dev)
> {
> int ret = toshiba_wireless_status(dev);
>
> @@ -2608,15 +2623,27 @@ static int toshiba_acpi_setup_wwan_rfkill(struct toshiba_acpi_dev *dev)
> return -ENOMEM;
> }
>
> + ret = devm_add_action_or_reset(parent, toshiba_acpi_rfkill_destroy,
> + dev->wwan_rfk);
> + if (ret)
> + return ret;
> +
> rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>
> ret = rfkill_register(dev->wwan_rfk);
> if (ret) {
> pr_err("Unable to register WWAN rfkill device\n");
> - rfkill_destroy(dev->wwan_rfk);
> + return ret;
> }
>
> - return ret;
> + ret = devm_add_action_or_reset(parent, toshiba_acpi_rfkill_unreg,
> + dev->wwan_rfk);
> + if (ret) {
> + dev->wwan_rfk = NULL;
> + return ret;
> + }
> +
> + return 0;
> }
>
> /*
> @@ -2996,11 +3023,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> sysfs_remove_group(&dev->acpi_dev->dev.kobj,
> &toshiba_attr_group);
>
> - if (dev->wwan_rfk) {
> - rfkill_unregister(dev->wwan_rfk);
> - rfkill_destroy(dev->wwan_rfk);
> - }
> -
> return 0;
> }
>
> @@ -3189,7 +3211,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>
> toshiba_wwan_available(dev);
> if (dev->wwan_supported)
> - toshiba_acpi_setup_wwan_rfkill(dev);
> + toshiba_acpi_setup_wwan_rfkill(parent, dev);
>
> toshiba_cooling_method_available(dev);
>
On Wed, 24 Mar 2021 14:55:48 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change binds the creation of the proc entries to the parent object,
> via the devm_add_action_or_reset() call.
> This way when the parent object's refcount goes to zero, the proc entries
> are removed in the reverse other in which they were created.
>
> This is the last bit of the toshiba_acpi_remove() function, so in this
> change this function is removed.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
I'm not 100% sure the series is worth the effort, but it does clean up
some of the ordering which definitely looks like a plus point.
Jonathan
> ---
> drivers/platform/x86/toshiba_acpi.c | 45 ++++++++++++++---------------
> 1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 8e8917979047..56ee5cd1e90c 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -1671,7 +1671,23 @@ static int __maybe_unused version_proc_show(struct seq_file *m, void *v)
>
> #define PROC_TOSHIBA "toshiba"
>
> -static void create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
> +static void remove_toshiba_proc_entries(void *data)
> +{
> + struct toshiba_acpi_dev *dev = data;
> +
> + if (dev->backlight_dev)
> + remove_proc_entry("lcd", toshiba_proc_dir);
> + if (dev->video_supported)
> + remove_proc_entry("video", toshiba_proc_dir);
> + if (dev->fan_supported)
> + remove_proc_entry("fan", toshiba_proc_dir);
> + if (dev->hotkey_dev)
> + remove_proc_entry("keys", toshiba_proc_dir);
> + remove_proc_entry("version", toshiba_proc_dir);
> +}
> +
> +static int create_toshiba_proc_entries(struct device *parent,
> + struct toshiba_acpi_dev *dev)
> {
> if (dev->backlight_dev)
> proc_create_data("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir,
> @@ -1687,19 +1703,8 @@ static void create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
> &keys_proc_ops, dev);
> proc_create_single_data("version", S_IRUGO, toshiba_proc_dir,
> version_proc_show, dev);
> -}
>
> -static void remove_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
> -{
> - if (dev->backlight_dev)
> - remove_proc_entry("lcd", toshiba_proc_dir);
> - if (dev->video_supported)
> - remove_proc_entry("video", toshiba_proc_dir);
> - if (dev->fan_supported)
> - remove_proc_entry("fan", toshiba_proc_dir);
> - if (dev->hotkey_dev)
> - remove_proc_entry("keys", toshiba_proc_dir);
> - remove_proc_entry("version", toshiba_proc_dir);
> + return devm_add_action_or_reset(parent, remove_toshiba_proc_entries, dev);
> }
>
> static const struct backlight_ops toshiba_backlight_data = {
> @@ -3012,15 +3017,6 @@ static void print_supported_features(struct toshiba_acpi_dev *dev)
> pr_cont("\n");
> }
>
> -static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> -{
> - struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
> -
> - remove_toshiba_proc_entries(dev);
> -
> - return 0;
> -}
> -
> static const char *find_hci_method(acpi_handle handle)
> {
> if (acpi_has_method(handle, "GHCI"))
> @@ -3230,7 +3226,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> if (ret)
> return ret;
>
> - create_toshiba_proc_entries(dev);
> + ret = create_toshiba_proc_entries(parent, dev);
> + if (ret)
> + return ret;
>
> toshiba_acpi = dev;
>
> @@ -3340,7 +3338,6 @@ static struct acpi_driver toshiba_acpi_driver = {
> .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> .ops = {
> .add = toshiba_acpi_add,
> - .remove = toshiba_acpi_remove,
> .notify = toshiba_acpi_notify,
> },
> .drv.pm = &toshiba_acpi_pm,
On Wed, 24 Mar 2021 14:55:47 +0200
Alexandru Ardelean <[email protected]> wrote:
> This change moves the creation of the Toshiba ACPI group to be
> automatically removed when the parent refcount goes to zero.
>
> The main reason to do this, is to also enforce that the order of removal is
> mirroring the order of initialization.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Hmm. The manual handling of the sysfs_create_group() is unfortunate (as opposed
to just setting a groups pointer) but there doesn't seem to be an easy way to fix
that with the current architecture. Ah well
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index a1249f6dde9a..8e8917979047 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -200,7 +200,6 @@ struct toshiba_acpi_dev {
> unsigned int usb_three_supported:1;
> unsigned int wwan_supported:1;
> unsigned int cooling_method_supported:1;
> - unsigned int sysfs_created:1;
> unsigned int special_functions;
>
> bool kbd_event_generated;
> @@ -3019,10 +3018,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>
> remove_toshiba_proc_entries(dev);
>
> - if (dev->sysfs_created)
> - sysfs_remove_group(&dev->acpi_dev->dev.kobj,
> - &toshiba_attr_group);
> -
> return 0;
> }
>
> @@ -3049,6 +3044,13 @@ static void toshiba_acpi_misc_deregister(void *data)
> misc_deregister(miscdev);
> }
>
> +static void toshiba_acpi_sysfs_remove(void *data)
> +{
> + struct kobject *kobj = data;
> +
> + sysfs_remove_group(kobj, &toshiba_attr_group);
> +}
> +
> static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> {
> struct device *parent = &acpi_dev->dev;
> @@ -3219,21 +3221,20 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>
> ret = sysfs_create_group(&dev->acpi_dev->dev.kobj,
> &toshiba_attr_group);
> - if (ret) {
> - dev->sysfs_created = 0;
> - goto error;
> - }
> - dev->sysfs_created = !ret;
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(parent,
> + toshiba_acpi_sysfs_remove,
> + &dev->acpi_dev->dev.kobj);
> + if (ret)
> + return ret;
>
> create_toshiba_proc_entries(dev);
>
> toshiba_acpi = dev;
>
> return 0;
> -
> -error:
> - toshiba_acpi_remove(acpi_dev);
> - return ret;
> }
>
> static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
On Mon, 29 Mar 2021 at 17:30, Jonathan Cameron <[email protected]> wrote:
>
> On Wed, 24 Mar 2021 14:55:39 +0200
> Alexandru Ardelean <[email protected]> wrote:
>
> > The 'toshiba_acpi_dev' object is allocated first and free'd last. We can
> > bind it's life-time to the parent ACPI device object. This is a first step
> > in using more device-managed allocated functions for this.
> >
> > The main intent is to try to convert the IIO framework to export only
> > device-managed functions (i.e. devm_iio_device_alloc() and
> > devm_iio_device_register()). It's still not 100% sure that this is
> > possible, but for now, this is the process of taking it slowly in that
> > direction.
> >
> > Signed-off-by: Alexandru Ardelean <[email protected]>
>
> Might just be me, but naming anything dev that isn't a struct device *
> is downright confusing?
>
I found it a bit odd as well, but I decided to not take it in
consideration for now.
>
>
>
> > ---
> > drivers/platform/x86/toshiba_acpi.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> > index fa7232ad8c39..6d298810b7bf 100644
> > --- a/drivers/platform/x86/toshiba_acpi.c
> > +++ b/drivers/platform/x86/toshiba_acpi.c
> > @@ -2998,8 +2998,6 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
> > if (toshiba_acpi)
> > toshiba_acpi = NULL;
> >
> > - kfree(dev);
> > -
> > return 0;
> > }
> >
> > @@ -3016,6 +3014,7 @@ static const char *find_hci_method(acpi_handle handle)
> >
> > static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> > {
> > + struct device *parent = &acpi_dev->dev;
> > struct toshiba_acpi_dev *dev;
> > const char *hci_method;
> > u32 dummy;
> > @@ -3033,7 +3032,7 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> > return -ENODEV;
> > }
> >
> > - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > + dev = devm_kzalloc(parent, sizeof(*dev), GFP_KERNEL);
> > if (!dev)
> > return -ENOMEM;
> > dev->acpi_dev = acpi_dev;
> > @@ -3045,7 +3044,6 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> > ret = misc_register(&dev->miscdev);
> > if (ret) {
> > pr_err("Failed to register miscdevice\n");
> > - kfree(dev);
> > return ret;
> > }
> >
>
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 <[email protected]> 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 <[email protected]> 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(-)
>>>
>>
>