Re: [PATCH v2] platform/x86: thinkpad_acpi: Switch to common use of attributes

[Hans de Goede]

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 <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Len Baker <len.baker@xxxxxxx>
>>>> ---
>>>> 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