As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.
So, to avoid open-coded arithmetic in the kzalloc() call inside the
create_attr_set() function the code must be refactored. Using the
struct_size() helper is the fast solution but it is better to switch
this code to common use of attributes.
Then, remove all the custom code to manage hotkey attributes and use the
attribute_group structure instead, refactoring the code accordingly.
Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
hotkey_radio_sw) use the is_visible callback from the same structure.
Moreover, now the hotkey_init_tablet_mode() function never returns a
negative number. So, the check after the call can be safely removed.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Len Baker <[email protected]>
---
Hi,
Following the suggestions made by Greg I have switch the code to common
use of attributes. However this code is untested. If someone could test
it would be great.
Thanks,
Len
Changelog v1 -> v2
- Don't use the struct_size helper and switch the code to common use of
attributes (Greg Kroah-Hartman).
drivers/platform/x86/thinkpad_acpi.c | 139 +++++----------------------
1 file changed, 26 insertions(+), 113 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 50ff04c84650..07b9710d500e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1001,79 +1001,6 @@ static struct platform_driver tpacpi_hwmon_pdriver = {
* sysfs support helpers
*/
-struct attribute_set {
- unsigned int members, max_members;
- struct attribute_group group;
-};
-
-struct attribute_set_obj {
- struct attribute_set s;
- struct attribute *a;
-} __attribute__((packed));
-
-static struct attribute_set *create_attr_set(unsigned int max_members,
- const char *name)
-{
- struct attribute_set_obj *sobj;
-
- if (max_members == 0)
- return NULL;
-
- /* Allocates space for implicit NULL at the end too */
- sobj = kzalloc(sizeof(struct attribute_set_obj) +
- max_members * sizeof(struct attribute *),
- GFP_KERNEL);
- if (!sobj)
- return NULL;
- sobj->s.max_members = max_members;
- sobj->s.group.attrs = &sobj->a;
- sobj->s.group.name = name;
-
- return &sobj->s;
-}
-
-#define destroy_attr_set(_set) \
- kfree(_set)
-
-/* not multi-threaded safe, use it in a single thread per set */
-static int add_to_attr_set(struct attribute_set *s, struct attribute *attr)
-{
- if (!s || !attr)
- return -EINVAL;
-
- if (s->members >= s->max_members)
- return -ENOMEM;
-
- s->group.attrs[s->members] = attr;
- s->members++;
-
- return 0;
-}
-
-static int add_many_to_attr_set(struct attribute_set *s,
- struct attribute **attr,
- unsigned int count)
-{
- int i, res;
-
- for (i = 0; i < count; i++) {
- res = add_to_attr_set(s, attr[i]);
- if (res)
- return res;
- }
-
- return 0;
-}
-
-static void delete_attr_set(struct attribute_set *s, struct kobject *kobj)
-{
- sysfs_remove_group(kobj, &s->group);
- destroy_attr_set(s);
-}
-
-#define register_attr_set_with_sysfs(_attr_set, _kobj) \
- sysfs_create_group(_kobj, &_attr_set->group)
-
static int parse_strtoul(const char *buf,
unsigned long max, unsigned long *value)
{
@@ -2042,8 +1969,6 @@ static u32 hotkey_acpi_mask; /* events enabled in firmware */
static u16 *hotkey_keycode_map;
-static struct attribute_set *hotkey_dev_attributes;
-
static void tpacpi_driver_event(const unsigned int hkey_event);
static void hotkey_driver_event(const unsigned int scancode);
static void hotkey_poll_setup(const bool may_warn);
@@ -3089,7 +3014,7 @@ static const struct attribute_group adaptive_kbd_attr_group = {
/* --------------------------------------------------------------------- */
-static struct attribute *hotkey_attributes[] __initdata = {
+static struct attribute *hotkey_attributes[] = {
&dev_attr_hotkey_enable.attr,
&dev_attr_hotkey_bios_enabled.attr,
&dev_attr_hotkey_bios_mask.attr,
@@ -3103,6 +3028,26 @@ static struct attribute *hotkey_attributes[] __initdata = {
&dev_attr_hotkey_source_mask.attr,
&dev_attr_hotkey_poll_freq.attr,
#endif
+ NULL
+};
+
+static umode_t hotkey_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ if (attr == &dev_attr_hotkey_tablet_mode.attr) {
+ if (!tp_features.hotkey_tablet)
+ return 0;
+ } else if (attr == &dev_attr_hotkey_radio_sw.attr) {
+ if (!tp_features.hotkey_wlsw)
+ return 0;
+ }
+
+ return attr->mode;
+}
+
+static const struct attribute_group hotkey_attr_group = {
+ .is_visible = hotkey_attr_is_visible,
+ .attrs = hotkey_attributes,
};
/*
@@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
hotkey_poll_stop_sync();
mutex_unlock(&hotkey_mutex);
#endif
-
- if (hotkey_dev_attributes)
- delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
+ sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
"restoring original HKEY status and mask\n");
@@ -3249,11 +3192,6 @@ static int hotkey_init_tablet_mode(void)
pr_info("Tablet mode switch found (type: %s), currently in %s mode\n",
type, in_tablet_mode ? "tablet" : "laptop");
- res = add_to_attr_set(hotkey_dev_attributes,
- &dev_attr_hotkey_tablet_mode.attr);
- if (res)
- return -1;
-
return in_tablet_mode;
}
@@ -3515,19 +3453,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
tpacpi_disable_brightness_delay();
- /* MUST have enough space for all attributes to be added to
- * hotkey_dev_attributes */
- hotkey_dev_attributes = create_attr_set(
- ARRAY_SIZE(hotkey_attributes) + 2,
- NULL);
- if (!hotkey_dev_attributes)
- return -ENOMEM;
- res = add_many_to_attr_set(hotkey_dev_attributes,
- hotkey_attributes,
- ARRAY_SIZE(hotkey_attributes));
- if (res)
- goto err_exit;
-
/* mask not supported on 600e/x, 770e, 770x, A21e, A2xm/p,
A30, R30, R31, T20-22, X20-21, X22-24. Detected by checking
for HKEY interface version 0x100 */
@@ -3636,18 +3561,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
pr_info("radio switch found; radios are %s\n",
enabled(status, 0));
}
- if (tp_features.hotkey_wlsw)
- res = add_to_attr_set(hotkey_dev_attributes,
- &dev_attr_hotkey_radio_sw.attr);
-
- res = hotkey_init_tablet_mode();
- if (res < 0)
- goto err_exit;
- tabletsw_state = res;
-
- res = register_attr_set_with_sysfs(hotkey_dev_attributes,
- &tpacpi_pdev->dev.kobj);
+ tabletsw_state = hotkey_init_tablet_mode();
+ res = sysfs_create_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
if (res)
goto err_exit;
@@ -3746,11 +3662,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
return 0;
err_exit:
- delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
- sysfs_remove_group(&tpacpi_pdev->dev.kobj,
- &adaptive_kbd_attr_group);
-
- hotkey_dev_attributes = NULL;
+ sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
+ sysfs_remove_group(&tpacpi_pdev->dev.kobj, &adaptive_kbd_attr_group);
return (res < 0) ? res : 1;
}
--
2.25.1
On Sun, Sep 26, 2021 at 01:19:08PM +0200, Len Baker wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
>
> So, to avoid open-coded arithmetic in the kzalloc() call inside the
> create_attr_set() function the code must be refactored. Using the
> struct_size() helper is the fast solution but it is better to switch
> this code to common use of attributes.
>
> Then, remove all the custom code to manage hotkey attributes and use the
> attribute_group structure instead, refactoring the code accordingly.
> Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
> hotkey_radio_sw) use the is_visible callback from the same structure.
>
> Moreover, now the hotkey_init_tablet_mode() function never returns a
> negative number. So, the check after the call can be safely removed.
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Len Baker <[email protected]>
> ---
> Hi,
>
> Following the suggestions made by Greg I have switch the code to common
> use of attributes. However this code is untested. If someone could test
> it would be great.
Much better, thanks.
But, I have a few questions here:
> @@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
> hotkey_poll_stop_sync();
> mutex_unlock(&hotkey_mutex);
> #endif
> -
> - if (hotkey_dev_attributes)
> - delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
> + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
Why do you have to manually add/remove these groups still?
A huge hint that something is going wrong is when you have to call a
sysfs_*() call from within a driver. There should be proper driver_*()
calls for you instead to get the job done.
As this is a platform device, why not set the dev_groups variable in the
platform_driver field so that these attribute groups get added and
removed automatically?
An example commit to look at that shows how this was converted for one
driver is 5bd08a4ae3d0 ("platform: x86: hp-wmi: convert platform driver
to use dev_groups"). See if that helps here as well.
thanks,
greg k-h
Hi Greg,
On Sun, Sep 26, 2021 at 01:32:16PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Sep 26, 2021 at 01:19:08PM +0200, Len Baker wrote:
>
> > @@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
> > hotkey_poll_stop_sync();
> > mutex_unlock(&hotkey_mutex);
> > #endif
> > -
> > - if (hotkey_dev_attributes)
> > - delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
> > + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
>
> Why do you have to manually add/remove these groups still?
>
> A huge hint that something is going wrong is when you have to call a
> sysfs_*() call from within a driver. There should be proper driver_*()
> calls for you instead to get the job done.
>
> As this is a platform device, why not set the dev_groups variable in the
> platform_driver field so that these attribute groups get added and
> removed automatically?
>
> An example commit to look at that shows how this was converted for one
> driver is 5bd08a4ae3d0 ("platform: x86: hp-wmi: convert platform driver
> to use dev_groups"). See if that helps here as well.
Ok, I will look at this and I will try to improve the patch. Anyway thanks
for the guidance and advices.
Again, thanks for your time.
Regards,
Len
Hi All,
On 9/26/21 1:32 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 26, 2021 at 01:19:08PM +0200, Len Baker wrote:
>> As noted in the "Deprecated Interfaces, Language Features, Attributes,
>> and Conventions" documentation [1], size calculations (especially
>> multiplication) should not be performed in memory allocator (or similar)
>> function arguments due to the risk of them overflowing. This could lead
>> to values wrapping around and a smaller allocation being made than the
>> caller was expecting. Using those allocations could lead to linear
>> overflows of heap memory and other misbehaviors.
>>
>> So, to avoid open-coded arithmetic in the kzalloc() call inside the
>> create_attr_set() function the code must be refactored. Using the
>> struct_size() helper is the fast solution but it is better to switch
>> this code to common use of attributes.
>>
>> Then, remove all the custom code to manage hotkey attributes and use the
>> attribute_group structure instead, refactoring the code accordingly.
>> Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
>> hotkey_radio_sw) use the is_visible callback from the same structure.
>>
>> Moreover, now the hotkey_init_tablet_mode() function never returns a
>> negative number. So, the check after the call can be safely removed.
>>
>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>>
>> Suggested-by: Greg Kroah-Hartman <[email protected]>
>> Signed-off-by: Len Baker <[email protected]>
>> ---
>> Hi,
>>
>> Following the suggestions made by Greg I have switch the code to common
>> use of attributes. However this code is untested. If someone could test
>> it would be great.
>
> Much better, thanks.
This indeed is much better and a great cleanup, thanks.
>
> But, I have a few questions here:
>
>> @@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
>> hotkey_poll_stop_sync();
>> mutex_unlock(&hotkey_mutex);
>> #endif
>> -
>> - if (hotkey_dev_attributes)
>> - delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
>> + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
>
> Why do you have to manually add/remove these groups still?
>
> A huge hint that something is going wrong is when you have to call a
> sysfs_*() call from within a driver. There should be proper driver_*()
> calls for you instead to get the job done.
>
> As this is a platform device, why not set the dev_groups variable in the
> platform_driver field so that these attribute groups get added and
> removed automatically?
The thinkpad_acpi code talks to the ACPI object representing the
ThinkPad embedded-controller and that has a lot of different sub-functionalities
which may or may not be present depending on the model laptop as well
as on the hw-configuration of the model.
The code is organized around all the different sub-functions with there
being a separate init + exit function for each sub-function, including
with first detecting in the init function if the functionality is present
(e.g. don't register SW_TABLETMODE_SW evdev reporting when the device
is not convertible / don register a WWAN rfkill if there is no WWAN modem).
Many (but not all) of the sub-functions come with a few sysfs-attributes
under /sys/bus/platform/devices/thinkpad_acpi/ many of the separate
function_init functions therefor call sysfs_create_group() for their own
set of sysfs-attributes, if the function is present on the machine.
> An example commit to look at that shows how this was converted for one
> driver is 5bd08a4ae3d0 ("platform: x86: hp-wmi: convert platform driver
> to use dev_groups"). See if that helps here as well.
Right, that results in a very nice cleanup. But there all the attributes
were always registered before the patch so throwing them together in a
ATTRIBUTE_GROUPS(hp_wmi) makes a ton of sense.
Here however we have all the separate function_init() blocks each
conditionally adding their own attributes if the function is present,
so that is different.
Currently there already are 8 separate sysfs_create_group() calls in
the thinkpad_acpi code, so even if we want to refactor this (I'm not
sure that we do) then doing so would fall outside of the scope of this
patch.
Greg, since this resolves / defers your remark and since this otherwise
is a nice cleanup I'm going to merge this version of this patch now.
Regards,
Hans
On Tue, Sep 28, 2021 at 04:55:25PM +0200, Hans de Goede wrote:
> Hi All,
>
> On 9/26/21 1:32 PM, Greg Kroah-Hartman wrote:
> > On Sun, Sep 26, 2021 at 01:19:08PM +0200, Len Baker wrote:
> >> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> >> and Conventions" documentation [1], size calculations (especially
> >> multiplication) should not be performed in memory allocator (or similar)
> >> function arguments due to the risk of them overflowing. This could lead
> >> to values wrapping around and a smaller allocation being made than the
> >> caller was expecting. Using those allocations could lead to linear
> >> overflows of heap memory and other misbehaviors.
> >>
> >> So, to avoid open-coded arithmetic in the kzalloc() call inside the
> >> create_attr_set() function the code must be refactored. Using the
> >> struct_size() helper is the fast solution but it is better to switch
> >> this code to common use of attributes.
> >>
> >> Then, remove all the custom code to manage hotkey attributes and use the
> >> attribute_group structure instead, refactoring the code accordingly.
> >> Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
> >> hotkey_radio_sw) use the is_visible callback from the same structure.
> >>
> >> Moreover, now the hotkey_init_tablet_mode() function never returns a
> >> negative number. So, the check after the call can be safely removed.
> >>
> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> >>
> >> Suggested-by: Greg Kroah-Hartman <[email protected]>
> >> Signed-off-by: Len Baker <[email protected]>
> >> ---
> >> Hi,
> >>
> >> Following the suggestions made by Greg I have switch the code to common
> >> use of attributes. However this code is untested. If someone could test
> >> it would be great.
> >
> > Much better, thanks.
>
> This indeed is much better and a great cleanup, thanks.
>
> >
> > But, I have a few questions here:
> >
> >> @@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
> >> hotkey_poll_stop_sync();
> >> mutex_unlock(&hotkey_mutex);
> >> #endif
> >> -
> >> - if (hotkey_dev_attributes)
> >> - delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
> >> + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
> >
> > Why do you have to manually add/remove these groups still?
> >
> > A huge hint that something is going wrong is when you have to call a
> > sysfs_*() call from within a driver. There should be proper driver_*()
> > calls for you instead to get the job done.
> >
> > As this is a platform device, why not set the dev_groups variable in the
> > platform_driver field so that these attribute groups get added and
> > removed automatically?
>
> The thinkpad_acpi code talks to the ACPI object representing the
> ThinkPad embedded-controller and that has a lot of different sub-functionalities
> which may or may not be present depending on the model laptop as well
> as on the hw-configuration of the model.
>
> The code is organized around all the different sub-functions with there
> being a separate init + exit function for each sub-function, including
> with first detecting in the init function if the functionality is present
> (e.g. don't register SW_TABLETMODE_SW evdev reporting when the device
> is not convertible / don register a WWAN rfkill if there is no WWAN modem).
>
> Many (but not all) of the sub-functions come with a few sysfs-attributes
> under /sys/bus/platform/devices/thinkpad_acpi/ many of the separate
> function_init functions therefor call sysfs_create_group() for their own
> set of sysfs-attributes, if the function is present on the machine.
>
> > An example commit to look at that shows how this was converted for one
> > driver is 5bd08a4ae3d0 ("platform: x86: hp-wmi: convert platform driver
> > to use dev_groups"). See if that helps here as well.
>
> Right, that results in a very nice cleanup. But there all the attributes
> were always registered before the patch so throwing them together in a
> ATTRIBUTE_GROUPS(hp_wmi) makes a ton of sense.
>
> Here however we have all the separate function_init() blocks each
> conditionally adding their own attributes if the function is present,
> so that is different.
>
> Currently there already are 8 separate sysfs_create_group() calls in
> the thinkpad_acpi code, so even if we want to refactor this (I'm not
> sure that we do) then doing so would fall outside of the scope of this
> patch.
>
> Greg, since this resolves / defers your remark and since this otherwise
> is a nice cleanup I'm going to merge this version of this patch now.
Ok, but having this all in one big list of attributes does work. You
can have multiple attribute groups listed together (that's why it's a
list of attribute groups, not just one attribute group that the driver
core is expecting.)
You just put the logic of "is this group needed or not?" in the
is_visible() callback for that group. You then don't need the
function_init() blocks to do anything with sysfs except maybe set a
device variable of "I have type foo" for the is_visible() callback to
check.
Yes, it's not obvious, but should clean up a lot of code in the end.
thanks,
greg k-h
Hi,
On 9/28/21 6:04 PM, Greg Kroah-Hartman wrote:
> On Tue, Sep 28, 2021 at 04:55:25PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> On 9/26/21 1:32 PM, Greg Kroah-Hartman wrote:
>>> On Sun, Sep 26, 2021 at 01:19:08PM +0200, Len Baker wrote:
>>>> As noted in the "Deprecated Interfaces, Language Features, Attributes,
>>>> and Conventions" documentation [1], size calculations (especially
>>>> multiplication) should not be performed in memory allocator (or similar)
>>>> function arguments due to the risk of them overflowing. This could lead
>>>> to values wrapping around and a smaller allocation being made than the
>>>> caller was expecting. Using those allocations could lead to linear
>>>> overflows of heap memory and other misbehaviors.
>>>>
>>>> So, to avoid open-coded arithmetic in the kzalloc() call inside the
>>>> create_attr_set() function the code must be refactored. Using the
>>>> struct_size() helper is the fast solution but it is better to switch
>>>> this code to common use of attributes.
>>>>
>>>> Then, remove all the custom code to manage hotkey attributes and use the
>>>> attribute_group structure instead, refactoring the code accordingly.
>>>> Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
>>>> hotkey_radio_sw) use the is_visible callback from the same structure.
>>>>
>>>> Moreover, now the hotkey_init_tablet_mode() function never returns a
>>>> negative number. So, the check after the call can be safely removed.
>>>>
>>>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>>>>
>>>> Suggested-by: Greg Kroah-Hartman <[email protected]>
>>>> Signed-off-by: Len Baker <[email protected]>
>>>> ---
>>>> Hi,
>>>>
>>>> Following the suggestions made by Greg I have switch the code to common
>>>> use of attributes. However this code is untested. If someone could test
>>>> it would be great.
>>>
>>> Much better, thanks.
>>
>> This indeed is much better and a great cleanup, thanks.
>>
>>>
>>> But, I have a few questions here:
>>>
>>>> @@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
>>>> hotkey_poll_stop_sync();
>>>> mutex_unlock(&hotkey_mutex);
>>>> #endif
>>>> -
>>>> - if (hotkey_dev_attributes)
>>>> - delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
>>>> + sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
>>>
>>> Why do you have to manually add/remove these groups still?
>>>
>>> A huge hint that something is going wrong is when you have to call a
>>> sysfs_*() call from within a driver. There should be proper driver_*()
>>> calls for you instead to get the job done.
>>>
>>> As this is a platform device, why not set the dev_groups variable in the
>>> platform_driver field so that these attribute groups get added and
>>> removed automatically?
>>
>> The thinkpad_acpi code talks to the ACPI object representing the
>> ThinkPad embedded-controller and that has a lot of different sub-functionalities
>> which may or may not be present depending on the model laptop as well
>> as on the hw-configuration of the model.
>>
>> The code is organized around all the different sub-functions with there
>> being a separate init + exit function for each sub-function, including
>> with first detecting in the init function if the functionality is present
>> (e.g. don't register SW_TABLETMODE_SW evdev reporting when the device
>> is not convertible / don register a WWAN rfkill if there is no WWAN modem).
>>
>> Many (but not all) of the sub-functions come with a few sysfs-attributes
>> under /sys/bus/platform/devices/thinkpad_acpi/ many of the separate
>> function_init functions therefor call sysfs_create_group() for their own
>> set of sysfs-attributes, if the function is present on the machine.
>>
>>> An example commit to look at that shows how this was converted for one
>>> driver is 5bd08a4ae3d0 ("platform: x86: hp-wmi: convert platform driver
>>> to use dev_groups"). See if that helps here as well.
>>
>> Right, that results in a very nice cleanup. But there all the attributes
>> were always registered before the patch so throwing them together in a
>> ATTRIBUTE_GROUPS(hp_wmi) makes a ton of sense.
>>
>> Here however we have all the separate function_init() blocks each
>> conditionally adding their own attributes if the function is present,
>> so that is different.
>>
>> Currently there already are 8 separate sysfs_create_group() calls in
>> the thinkpad_acpi code, so even if we want to refactor this (I'm not
>> sure that we do) then doing so would fall outside of the scope of this
>> patch.
>>
>> Greg, since this resolves / defers your remark and since this otherwise
>> is a nice cleanup I'm going to merge this version of this patch now.
>
> Ok, but having this all in one big list of attributes does work. You
> can have multiple attribute groups listed together (that's why it's a
> list of attribute groups, not just one attribute group that the driver
> core is expecting.)
>
> You just put the logic of "is this group needed or not?" in the
> is_visible() callback for that group. You then don't need the
> function_init() blocks to do anything with sysfs except maybe set a
> device variable of "I have type foo" for the is_visible() callback to
> check.
>
> Yes, it's not obvious, but should clean up a lot of code in the end.
That is an interesting suggestion, if someone feels up to giving this
a try I wonder what the end-result will look like.
Regards,
Hans