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

[Andy Shevchenko <andy@xxxxxxxxxx>]

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.

> >> +	}

...

> >> +	/* 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 = '_';

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

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

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

-- 
With Best Regards,
Andy Shevchenko