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
