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

[Hans de Goede]

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.

Regards,

Hans