Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 12/16/22 18:14, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote:
>> On 12/16/22 15:16, Andy Shevchenko wrote:
>>> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>> +	if (IS_ERR(int3472->pled.gpio)) {
>>>> +		ret = PTR_ERR(int3472->pled.gpio);
>>>> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");
>>>
>>> 	return dev_err_probe(...);
>>
>> That goes over 100 chars.
> 
> The point is you don't need ret to be initialized. Moreover, no-one prevents
> you to split the line to two.

The compiler is perfectly capable of optimizing away the store
in ret if that is not necessary; and splitting the line instead
of doing it above will just make the code harder to read.

Also this really is bikeshedding...

> 
>>>> +	}
> 
> ...
> 
>>>> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>>>> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
>>>> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
>>>
>>>> +	for (i = 0; int3472->pled.name[i]; i++) {
>>>> +		if (int3472->pled.name[i] == ':') {
>>>> +			int3472->pled.name[i] = '_';
>>>> +			break;
>>>> +		}
>>>> +	}
>>>
>>> NIH strreplace().
>>
>> Please look more careful, quoting from the strreplace() docs:
>>
>>  * strreplace - Replace all occurrences of character in string.
>>
>> Notice the *all* and we only want to replace the first ':' here,
>> because the ':' char has a special meaning in LED class-device-names.
> 
> It's still possible to use that, but anyway, the above is still
> something NIH.
> 
> 	char *p;
> 
> 	p = strchr(name, ':');
> 	*p = '_';

Ok, In will switch to this for the next version.

> But either code has an issue if by some reason you need to check if : is ever
> present in acpi_dev_name().

acpi device names are set by this code:

        result = ida_alloc(instance_ida, GFP_KERNEL);
        if (result < 0)
                return result;

        device->pnp.instance_no = result;
        dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, result);

And the bus_id cannot have a : in it, so there always is a single :.


> 
> The more robust is either to copy acpi_dev_name(), call strreplace(), so you
> will be sure that _all_ : from ACPI device name will be covered and then attach
> the rest.
> 
> ...
> 
>>>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
>>>> +{
>>>> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
>>>> +		return;
>>>
>>> This dups the check inside the _unregister() below, right?
>>
>> Right.
>>
>>>> +	led_remove_lookup(&int3472->pled.lookup);
>>>
>>> With list_del_init() I believe the above check can be droped.
>>
>> No it cannot, list_del_init() inside led_remove_lookup() would
>> protect against double led_remove_lookup() calls.
>>
>> But here we may have a completely uninitialized list_head on
>> devices without an INT3472 privacy-led, which will trigger
>> either __list_del_entry_valid() errors or lead to NULL
>> pointer derefs.
> 
> But we can initialize that as well...

The standard pattern in the kernel is that INIT_LIST_HEAD()
is only used for list_head-s which are actually used as the head
of the list. list_head-s used to track members of the list are
usually not initialized until they are added to the list.

Doing multiple list-init-s in multiple cases, including
one in *subsystem core code* just to drop an if here seems
counter productive.

Also checking that we can move forward with the unregister
is a good idea regardless of all the called functions being
able to run safely if the register never happened, because
future changes to the unregister function might end up
doing something which is unsafe when the LED was never
registered in the first place.

Regards,

Hans




> 
>>>> +	led_classdev_unregister(&int3472->pled.classdev);
>>>> +	gpiod_put(int3472->pled.gpio);
>>>> +}
>