linux-kernel - Re: [PATCH 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Jun 2022 18:05:55 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jean-Jacques Hiblot <jjhiblot@...phandler.com>
Cc:     Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Sven Schwermer <sven.schwermer@...ruptive-technologies.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        johan+linaro@...nel.org,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] leds: Add a multicolor LED driver to group
 monochromatic LEDs

On Wed, Jun 15, 2022 at 5:49 PM Jean-Jacques Hiblot
<jjhiblot@...phandler.com> wrote:
>
> By allowing to group multiple monochrome LED into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.

...

> +#include <linux/err.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>

Missed math.h

...

> +static int iterate_subleds(struct device *dev, struct led_mcg_priv *priv,
> +                          struct fwnode_handle *mcnode)

Use namespace even for static functions (think about tracing, for example).

led_mcg_iterave_subleds

> +{
> +       struct mc_subled *subled = priv->mc_cdev.subled_info;
> +       struct fwnode_handle *fwnode;
> +       int ret;
> +
> +       /* iterate over the nodes inside the multi-led node */
> +       fwnode_for_each_child_node(mcnode, fwnode) {
> +               u32 color;
> +               struct led_classdev *led_cdev;
> +
> +               led_cdev = devm_fwnode_led_get(dev, fwnode, 0);
> +               if (IS_ERR(led_cdev)) {

> +                       ret = PTR_ERR(led_cdev);
> +                       dev_err(dev, "unable to request LED: %d\n", ret);

ret = dev_err_probe(...);

> +                       goto release_fwnode;
> +               }
> +               priv->monochromatics[priv->mc_cdev.num_colors] = led_cdev;
> +
> +               ret = fwnode_property_read_u32(fwnode, "color", &color);
> +               if (ret) {
> +                       dev_err(dev, "cannot read color: %d\n", ret);
> +                       goto release_fwnode;
> +               }
> +               subled[priv->mc_cdev.num_colors].color_index = color;
> +
> +               /* Make the sysfs of the monochromatic LED read-only */
> +               led_cdev->flags |= LED_SYSFS_DISABLE;
> +
> +               priv->mc_cdev.num_colors++;
> +       }
> +
> +       return 0;
> +
> +release_fwnode:
> +       fwnode_handle_put(fwnode);
> +       return ret;
> +}

...

> +       /* count the nodes inside the multi-led node */
> +       fwnode_for_each_child_node(mcnode, fwnode)
> +               count++;

Don't we have a _count API? Hmm... Indeed, we have it only for a
device and not for fwnode...

...

> +       priv = devm_kzalloc(&pdev->dev,
> +                           struct_size(priv, monochromatics, count),
> +                           GFP_KERNEL);
> +       if (!priv) {
> +               ret = -ENOMEM;
> +               goto release_mcnode;

This is the wrong order. You shouldn't mix non-devm_ APIs with devm_
like this. devm_ calls always should be first. You have two options
(at least?): 1) drop devm_ and switch to plain error handling and
->remove(); 2) make devm_ wrappers for the certain calls.

> +       }

...

> +       if (ret) {
> +               dev_err(&pdev->dev,
> +                       "failed to register multicolor led for %s: %d\n",
> +                       cdev->name, ret);

Taking into account above,
return dev_err_probe(...);

> +               goto release_mcnode;
> +       }

-- 
With Best Regards,
Andy Shevchenko