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 15:16, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
>> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
>> X1 Nano gen 2 there is no clock-enable pin, triggering the:
>> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
>> LED to not work.
>>
>> Fix this by modeling the privacy LED as a LED class device rather then
>> integrating it with the registered clock.
>>
>> Note this relies on media subsys changes to actually turn the LED on/off
>> when the sensor's v4l2_subdev's s_stream() operand gets called.
> 
> ...
> 
>> +	struct int3472_pled {
>> +		char name[INT3472_LED_MAX_NAME_LEN];
>> +		struct led_lookup_data lookup;
> 
>> +		struct led_classdev classdev;
> 
> Why not putting this as a first member in the struct, so any container_of()
> against it become no-op at compile time?

Ack will fix for v4.

> 
>> +		struct gpio_desc *gpio;
>> +	} pled;
> 
> ...
> 
>> +	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.


> 
>> +	}
> 
> ...
> 
>> +	/* 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.


> 
> ...
> 
>> +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.


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

Regards.

Hans