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
