| 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
| ||
|
Message-ID: <82e9cffc-472a-b725-1a12-de8aade67189@wanadoo.fr>
Date: Sun, 1 Oct 2023 22:46:11 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: André Apitzsch <git@...tzsch.eu>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org,
krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
pavel@....cz, phone-devel@...r.kernel.org, robh+dt@...nel.org,
u.kleine-koenig@...gutronix.de,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v5 2/2] leds: add ktd202x driver
Le 01/10/2023 à 18:56, André Apitzsch a écrit :
> Hi Christophe,
>
> Am Sonntag, dem 01.10.2023 um 17:15 +0200 schrieb Christophe JAILLET:
>> Le 01/10/2023 à 15:52, André Apitzsch a écrit :
>>> This commit adds support for Kinetic KTD2026/7 RGB/White LED
>>> driver.
>>>
>>> Signed-off-by: André Apitzsch
>>> <git-AtRKszJ1oGPsq35pWSNszA@...lic.gmane.org>
>>
>> ...
>>
>>> +static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct
>>> device_node *np,
>>> + struct ktd202x_led *led, struct
>>> led_init_data *init_data)
>>> +{
>>> + struct led_classdev *cdev;
>>> + struct device_node *child;
>>> + struct mc_subled *info;
>>> + int num_channels;
>>> + int i = 0;
>>> + u32 reg;
>>> + int ret;
>>> +
>>> + num_channels = of_get_available_child_count(np);
>>> + if (!num_channels || num_channels > chip->num_leds)
>>> + return -EINVAL;
>>> +
>>> + info = devm_kcalloc(chip->dev, num_channels, sizeof(*info),
>>> GFP_KERNEL);
>>> + if (!info)
>>> + return -ENOMEM;
>>> +
>>> + for_each_available_child_of_node(np, child) {
>>> + u32 mono_color = 0;
>>
>> Un-needed init.
>> And, why is it defined here, while reg is defined out-side the loop?
>
> I'll move it out-side the loop (without initialization).
>
>>
>>> +
>>> + ret = of_property_read_u32(child, "reg", ®);
>>> + if (ret != 0 || reg >= chip->num_leds) {
>>> + dev_err(chip->dev, "invalid 'reg' of
>>> %pOFn\n", np);
>>
>> Mossing of_node_put(np);?
>
> It shouldn't be needed here if handled in the calling function, right?
How can the caller do this?
The goal of this of_node_put() is to release a reference taken by the
for_each_available_child_of_node() loop, in case of early exit.
The caller can't know if np needs to be released or not. An error code
is returned either if an error occurs within the for_each loop, or if
devm_led_classdev_multicolor_register_ext() fails.
More over, in your case the caller is ktd202x_add_led().
From there either ktd202x_setup_led_rgb() or ktd202x_setup_led_single()
is called.
ktd202x_setup_led_single() does not take any reference to np.
But if it fails, of_node_put() would still be called.
>
>>
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = of_property_read_u32(child, "color",
>>> &mono_color);
>>> + if (ret < 0 && ret != -EINVAL) {
>>> + dev_err(chip->dev, "failed to parse 'color'
>>> of %pOF\n", np);
>>
>> Mossing of_node_put(np);?
>>
>>> + return ret;
>>> + }
>>> +
>>> + info[i].color_index = mono_color;
>>> + info[i].channel = reg;
>>> + info[i].intensity = KTD202X_MAX_BRIGHTNESS;
>>> + i++;
>>> + }
>>> +
>>> + led->mcdev.subled_info = info;
>>> + led->mcdev.num_colors = num_channels;
>>> +
>>> + cdev = &led->mcdev.led_cdev;
>>> + cdev->brightness_set_blocking = ktd202x_brightness_mc_set;
>>> + cdev->blink_set = ktd202x_blink_mc_set;
>>> +
>>> + return devm_led_classdev_multicolor_register_ext(chip->dev,
>>> &led->mcdev, init_data);
>>> +}
>>> +
>>> +static int ktd202x_setup_led_single(struct ktd202x *chip, struct
>>> device_node *np,
>>> + struct ktd202x_led *led, struct
>>> led_init_data *init_data)
>>> +{
>>> + struct led_classdev *cdev;
>>> + u32 reg;
>>> + int ret;
>>> +
>>> + ret = of_property_read_u32(np, "reg", ®);
>>> + if (ret != 0 || reg >= chip->num_leds) {
>>> + dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
>>> + return -EINVAL;
>>> + }
>>> + led->index = reg;
>>> +
>>> + cdev = &led->cdev;
>>> + cdev->brightness_set_blocking =
>>> ktd202x_brightness_single_set;
>>> + cdev->blink_set = ktd202x_blink_single_set;
>>> +
>>> + return devm_led_classdev_register_ext(chip->dev, &led-
>>>> cdev, init_data);
>>> +}
>>> +
>>> +static int ktd202x_add_led(struct ktd202x *chip, struct
>>> device_node *np, unsigned int index)
>>> +{
>>> + struct ktd202x_led *led = &chip->leds[index];
>>> + struct led_init_data init_data = {};
>>> + struct led_classdev *cdev;
>>> + u32 color = 0;
>> Un-needed init.
>>
>>> + int ret;
>>> +
>>> + /* Color property is optional in single color case */
>>> + ret = of_property_read_u32(np, "color", &color);
>>> + if (ret < 0 && ret != -EINVAL) {
>>> + dev_err(chip->dev, "failed to parse 'color' of
>>> %pOF\n", np);
>>> + return ret;
>>> + }
>>> +
>>> + led->chip = chip;
>>> + init_data.fwnode = of_fwnode_handle(np);
>>> +
>>> + if (color == LED_COLOR_ID_RGB) {
>>> + cdev = &led->mcdev.led_cdev;
>>> + ret = ktd202x_setup_led_rgb(chip, np, led,
>>> &init_data);
>>> + } else {
>>> + cdev = &led->cdev;
>>> + ret = ktd202x_setup_led_single(chip, np, led,
>>> &init_data);
>>> + }
>>> +
>>> + if (ret) {
>>> + dev_err(chip->dev, "unable to register %s\n", cdev-
>>>> name);
>>> + of_node_put(np);
>>
>> This is strange to have it here.
>> Why not above after "if (ret < 0 && ret != -EINVAL) {"?
>>
>> It would look much more natural to have it a few lines below, ... [1]
>
> Good catch. I'll move of_node_put(np); to [1] and [2].
Why [2]?
It does not seem needed here.
of_get_available_child_count() does not keep any reference.
CJ
>
>>
>>> + return ret;
>>> + }
>>> +
>>> + cdev->max_brightness = KTD202X_MAX_BRIGHTNESS;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ktd202x_probe_dt(struct ktd202x *chip)
>>> +{
>>> + struct device_node *np = dev_of_node(chip->dev), *child;
>>> + unsigned int i;
>>> + int count, ret;
>>> +
>>> + chip->num_leds = (int)(unsigned
>>> long)of_device_get_match_data(chip->dev);
>>> +
>>> + count = of_get_available_child_count(np);
>>> + if (!count || count > chip->num_leds)
>
> [2].
>
>>> + return -EINVAL;
>>> +
>>> + regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL,
>>> KTD202X_RSTR_RESET);
>>> +
>>> + /* Allow the device to execute the complete reset */
>>> + usleep_range(200, 300);
>>> +
>>> + i = 0;
>>> + for_each_available_child_of_node(np, child) {
>>> + ret = ktd202x_add_led(chip, child, i);
>>> + if (ret)
>>
>> [1] ... here.
>>
>> Otherwise, it is likely that, thanks to a static checker, an
>> additionnal
>> of_node_put() will be added on early exit of the loop.
>>
>> CJ
>>
>>> + return ret;
>>> + i++;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> ...
>>
>
> Best regards,
> André
>
Powered by blists - more mailing lists