| 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: <8c6bd40f-164d-46ba-80de-36ea349e1437@wanadoo.fr>
Date: Mon, 5 Aug 2024 16:12:30 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Joseph Strauss <jstrauss@...lbox.org>, lee@...nel.org
Cc: pavel@....cz, jansimon.moeller@....de, conor@...nel.org,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8] Add multicolor support to BlinkM LED driver
Le 10/07/2024 à 20:48, Joseph Strauss a écrit :
> Add multicolor support to the BlinkM driver, making it easier to control
> from userspace. The BlinkM LED is a programmable RGB LED. The driver
> currently supports only the regular LED sysfs class, resulting in the
> creation of three distinct classes, one for red, green, and blue. The
> user then has to input three values into the three seperate brightness
> files within those classes. The multicolor LED framework makes the
> device easier to control with the multi_intensity file: the user can
> input three values at once to form a color, while still controlling the
> lightness with the brightness file.
>
> The main struct blinkm_led has changed slightly. The struct led_classdev
> for the regular sysfs classes remain. The blinkm_probe function checks
> CONFIG_LEDS_BLINKM_MULTICOLOR to decide whether to load the seperate
> sysfs classes or the single multicolor one, but never both. The
> blinkm_set_mc_brightness() function had to be added to calculate the
> three color components and then set the fields of the blinkm_data
> structure accordingly.
>
> Signed-off-by: Joseph Strauss <jstrauss@...lbox.org>
> ---
Hi,
this patch has already been applied, but I have 2 questions/remarks below:
...
> -static int blinkm_probe(struct i2c_client *client)
> +static int register_separate_colors(struct i2c_client *client, struct blinkm_data *data)
> {
> - struct blinkm_data *data;
> - struct blinkm_led *led[3];
> - int err, i;
> + /* 3 separate classes for red, green, and blue respectively */
> + struct blinkm_led *leds[NUM_LEDS];
> + int err;
> char blinkm_led_name[28];
> -
> - data = devm_kzalloc(&client->dev,
> - sizeof(struct blinkm_data), GFP_KERNEL);
> - if (!data) {
> - err = -ENOMEM;
> - goto exit;
> - }
> -
> - data->i2c_addr = 0x08;
> - /* i2c addr - use fake addr of 0x08 initially (real is 0x09) */
> - data->fw_ver = 0xfe;
> - /* firmware version - use fake until we read real value
> - * (currently broken - BlinkM confused!) */
> - data->script_id = 0x01;
> - data->i2c_client = client;
> -
> - i2c_set_clientdata(client, data);
> - mutex_init(&data->update_lock);
> -
> - /* Register sysfs hooks */
> - err = sysfs_create_group(&client->dev.kobj, &blinkm_group);
> - if (err < 0) {
> - dev_err(&client->dev, "couldn't register sysfs group\n");
> - goto exit;
> - }
> -
> - for (i = 0; i < 3; i++) {
> + /* Register red, green, and blue sysfs classes */
> + for (int i = 0; i < NUM_LEDS; i++) {
> /* RED = 0, GREEN = 1, BLUE = 2 */
> - led[i] = &data->blinkm_leds[i];
> - led[i]->i2c_client = client;
> - led[i]->id = i;
> - led[i]->led_cdev.max_brightness = 255;
> - led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
> + leds[i] = &data->blinkm_leds[i];
> + leds[i]->i2c_client = client;
> + leds[i]->id = i;
> + leds[i]->cdev.led_cdev.max_brightness = 255;
> + leds[i]->cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
> switch (i) {
> case RED:
> - snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> + scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
> "blinkm-%d-%d-red",
> client->adapter->nr,
> client->addr);
> - led[i]->led_cdev.name = blinkm_led_name;
> - led[i]->led_cdev.brightness_set_blocking =
> + leds[i]->cdev.led_cdev.name = blinkm_led_name;
> + leds[i]->cdev.led_cdev.brightness_set_blocking =
> blinkm_led_red_set;
> err = led_classdev_register(&client->dev,
> - &led[i]->led_cdev);
> + &leds[i]->cdev.led_cdev);
> if (err < 0) {
> dev_err(&client->dev,
> "couldn't register LED %s\n",
> - led[i]->led_cdev.name);
> + leds[i]->cdev.led_cdev.name);
> goto failred;
> }
> break;
> case GREEN:
> - snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> + scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
> "blinkm-%d-%d-green",
> client->adapter->nr,
> client->addr);
> - led[i]->led_cdev.name = blinkm_led_name;
> - led[i]->led_cdev.brightness_set_blocking =
> + leds[i]->cdev.led_cdev.name = blinkm_led_name;
> + leds[i]->cdev.led_cdev.brightness_set_blocking =
> blinkm_led_green_set;
> err = led_classdev_register(&client->dev,
> - &led[i]->led_cdev);
> + &leds[i]->cdev.led_cdev);
> if (err < 0) {
> dev_err(&client->dev,
> "couldn't register LED %s\n",
> - led[i]->led_cdev.name);
> + leds[i]->cdev.led_cdev.name);
> goto failgreen;
> }
> break;
> case BLUE:
> - snprintf(blinkm_led_name, sizeof(blinkm_led_name),
> + scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
> "blinkm-%d-%d-blue",
> client->adapter->nr,
> client->addr);
> - led[i]->led_cdev.name = blinkm_led_name;
> - led[i]->led_cdev.brightness_set_blocking =
> + leds[i]->cdev.led_cdev.name = blinkm_led_name;
> + leds[i]->cdev.led_cdev.brightness_set_blocking =
> blinkm_led_blue_set;
> err = led_classdev_register(&client->dev,
> - &led[i]->led_cdev);
> + &leds[i]->cdev.led_cdev);
> if (err < 0) {
> dev_err(&client->dev,
> "couldn't register LED %s\n",
> - led[i]->led_cdev.name);
> + leds[i]->cdev.led_cdev.name);
> goto failblue;
> }
> break;
> + default:
> + break;
> } /* end switch */
> } /* end for */
> -
> - /* Initialize the blinkm */
> - blinkm_init_hw(client);
> -
> return 0;
>
> failblue:
> - led_classdev_unregister(&led[GREEN]->led_cdev);
> -
> + led_classdev_unregister(&leds[GREEN]->cdev.led_cdev);
> failgreen:
> - led_classdev_unregister(&led[RED]->led_cdev);
> -
> + led_classdev_unregister(&leds[RED]->cdev.led_cdev);
> failred:
> sysfs_remove_group(&client->dev.kobj, &blinkm_group);
I think that this would be more logical to have it in an error handling
path in blinkm_probe(), as it was before.
This is strange to un-do sysfs_create_group() here in
register_separate_colors() and...
> -exit:
> +
> return err;
> }
>
> +static int register_multicolor(struct i2c_client *client, struct blinkm_data *data)
> +{
> + struct blinkm_led *mc_led;
> + struct mc_subled *mc_led_info;
> + char blinkm_led_name[28];
> + int err;
> +
> + /* Register multicolor sysfs class */
> + /* The first element of leds is used for multicolor facilities */
> + mc_led = &data->blinkm_leds[RED];
> + mc_led->i2c_client = client;
> +
> + mc_led_info = devm_kcalloc(&client->dev, NUM_LEDS, sizeof(*mc_led_info),
> + GFP_KERNEL);
> + if (!mc_led_info)
> + return -ENOMEM;
> +
> + mc_led_info[RED].color_index = LED_COLOR_ID_RED;
> + mc_led_info[GREEN].color_index = LED_COLOR_ID_GREEN;
> + mc_led_info[BLUE].color_index = LED_COLOR_ID_BLUE;
> +
> + mc_led->cdev.mcled_cdev.subled_info = mc_led_info;
> + mc_led->cdev.mcled_cdev.num_colors = NUM_LEDS;
> + mc_led->cdev.mcled_cdev.led_cdev.brightness = 255;
> + mc_led->cdev.mcled_cdev.led_cdev.max_brightness = 255;
> + mc_led->cdev.mcled_cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> + scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
> + "blinkm-%d-%d:rgb:indicator",
> + client->adapter->nr,
> + client->addr);
> + mc_led->cdev.mcled_cdev.led_cdev.name = blinkm_led_name;
> + mc_led->cdev.mcled_cdev.led_cdev.brightness_set_blocking = blinkm_set_mc_brightness;
> +
> + err = led_classdev_multicolor_register(&client->dev, &mc_led->cdev.mcled_cdev);
> + if (err < 0) {
> + dev_err(&client->dev, "couldn't register LED %s\n",
> + mc_led->cdev.led_cdev.name);
> + sysfs_remove_group(&client->dev.kobj, &blinkm_group);
... here in register_multicolor().
Also, is it on pourpose that err is *not* propagated?
If led_classdev_multicolor_register() fails, we still return 0.
> + }
> + return 0;
> +}
> +
> +static int blinkm_probe(struct i2c_client *client)
> +{
> + struct blinkm_data *data;
> + int err;
> +
> + data = devm_kzalloc(&client->dev,
> + sizeof(struct blinkm_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->i2c_addr = 0x08;
> + /* i2c addr - use fake addr of 0x08 initially (real is 0x09) */
> + data->fw_ver = 0xfe;
> + /* firmware version - use fake until we read real value
> + * (currently broken - BlinkM confused!)
> + */
> + data->script_id = 0x01;
> + data->i2c_client = client;
> +
> + i2c_set_clientdata(client, data);
> + mutex_init(&data->update_lock);
> +
> + /* Register sysfs hooks */
> + err = sysfs_create_group(&client->dev.kobj, &blinkm_group);
> + if (err < 0) {
> + dev_err(&client->dev, "couldn't register sysfs group\n");
> + return err;
> + }
> +
> + if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR)) {
> + err = register_separate_colors(client, data);
> + if (err < 0)
> + return err;
> + } else {
> + err = register_multicolor(client, data);
> + if (err < 0)
> + return err;
> + }
> +
> + blinkm_init_hw(client);
> +
> + return 0;
> +}
...
CJ
Powered by blists - more mailing lists