From: Lee Jones <lee@kernel.org>
To: Lorenzo Egidio <lorenzoegidioms@gmail.com>
Cc: pavel@ucw.cz, linux-leds@vger.kernel.org
Subject: Re: [PATCH v2] leds: tests: use a fresh instance for name conflict rejection
Date: Wed, 17 Jun 2026 09:48:21 +0100 [thread overview]
Message-ID: <20260617084821.GD10056@google.com> (raw)
In-Reply-To: <20260612230606.1438-1-lorenzoegidioms@gmail.com>
On Fri, 12 Jun 2026, Lorenzo Egidio wrote:
> The LED_REJECT_NAME_CONFLICT test currently re-registers an
> already registered led_classdev instance.
>
> Use a fresh copy instead so the test exercises the
> name-conflict rejection path directly.
>
> Signed-off-by: Lorenzo Egidio <lorenzoegidioms@gmail.com>
How did you author this patch? Honestly.
> ---
> drivers/leds/led-test.c | 102 ++++++++++++++++++++++++++--------------
> 1 file changed, 66 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/leds/led-test.c b/drivers/leds/led-test.c
> index ddf9aa967..36aef3e13 100644
> --- a/drivers/leds/led-test.c
> +++ b/drivers/leds/led-test.c
> @@ -1,4 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> +// leds: tests: clarify name conflict test
Why did you add this?
> /*
> * Copyright (C) 2025 Google LLC
> *
> @@ -10,77 +11,106 @@
> #include <linux/device.h>
> #include <linux/leds.h>
>
> -#define LED_TEST_POST_REG_BRIGHTNESS 10
> +enum {
> + LED_TEST_POST_REG_BRIGHTNESS = 10,
> +};
Why?
> -struct led_test_ddata {
> +struct led_test_data {
Why rename?
> struct led_classdev cdev;
> struct device *dev;
> };
>
> -static enum led_brightness led_test_brightness_get(struct led_classdev *cdev)
> +static enum led_brightness
> +led_test_brightness_get(struct led_classdev *cdev)
Why?
> {
> return LED_TEST_POST_REG_BRIGHTNESS;
> }
>
> -static void led_test_class_register(struct kunit *test)
> +static void led_test_init_cdev(struct led_classdev *cdev)
> {
> - struct led_test_ddata *ddata = test->priv;
> - struct led_classdev *cdev_clash, *cdev = &ddata->cdev;
> - struct device *dev = ddata->dev;
> - int ret;
> + memset(cdev, 0, sizeof(*cdev));
Why?
>
> - /* Register a LED class device */
> cdev->name = "led-test";
> - cdev->brightness_get = led_test_brightness_get;
> cdev->brightness = 0;
> + cdev->brightness_get = led_test_brightness_get;
> +}
> +
> +static void led_test_class_register(struct kunit *test)
> +{
> + struct led_test_data *data = test->priv;
> + struct led_classdev *cdev = &data->cdev;
> + struct led_classdev *cdev_clash;
> + struct led_classdev *cdev_reject;
> + struct device *dev = data->dev;
> + int ret;
> +
> + led_test_init_cdev(cdev);
>
> ret = devm_led_classdev_register(dev, cdev);
> KUNIT_ASSERT_EQ(test, ret, 0);
>
> + KUNIT_EXPECT_NOT_NULL(test, cdev->dev);
How could this happen?
> KUNIT_EXPECT_EQ(test, cdev->max_brightness, LED_FULL);
> - KUNIT_EXPECT_EQ(test, cdev->brightness, LED_TEST_POST_REG_BRIGHTNESS);
> + KUNIT_EXPECT_EQ(test, cdev->brightness,
> + LED_TEST_POST_REG_BRIGHTNESS);
Why?
> KUNIT_EXPECT_STREQ(test, cdev->dev->kobj.name, "led-test");
>
> - /* Register again with the same name - expect it to pass with the LED renamed */
> + /*
> + * Name collision should be resolved automatically by
> + * renaming the device instance while preserving the
> + * logical LED name.
> + */
> cdev_clash = devm_kmemdup(dev, cdev, sizeof(*cdev), GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_clash);
>
> ret = devm_led_classdev_register(dev, cdev_clash);
> KUNIT_ASSERT_EQ(test, ret, 0);
>
> - KUNIT_EXPECT_STREQ(test, cdev_clash->dev->kobj.name, "led-test_1");
> - KUNIT_EXPECT_STREQ(test, cdev_clash->name, "led-test");
> + KUNIT_EXPECT_STREQ(test,
> + cdev_clash->dev->kobj.name,
> + "led-test_1");
> + KUNIT_EXPECT_STREQ(test,
> + cdev_clash->name,
> + "led-test");
Why?
>
> - /* Enable name conflict rejection and register with the same name again - expect failure */
> - cdev_clash->flags |= LED_REJECT_NAME_CONFLICT;
> - ret = devm_led_classdev_register(dev, cdev_clash);
> + /*
> + * Verify that explicit conflict rejection fails.
> + */
Why did you write a single line comment like this?
> + cdev_reject = devm_kmemdup(dev, cdev, sizeof(*cdev), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_reject);
> +
> + cdev_reject->flags |= LED_REJECT_NAME_CONFLICT;
> +
> + ret = devm_led_classdev_register(dev, cdev_reject);
> KUNIT_EXPECT_EQ(test, ret, -EEXIST);
> }
>
> static void led_test_class_add_lookup_and_get(struct kunit *test)
> {
> - struct led_test_ddata *ddata = test->priv;
> - struct led_classdev *cdev = &ddata->cdev, *cdev_get;
> - struct device *dev = ddata->dev;
> - struct led_lookup_data lookup;
> + struct led_test_data *data = test->priv;
> + struct led_classdev *cdev = &data->cdev;
> + struct led_classdev *cdev_get;
> + struct device *dev = data->dev;
> + struct led_lookup_data lookup = { };
> int ret;
>
> - /* First, register a LED class device */
> - cdev->name = "led-test";
> + led_test_init_cdev(cdev);
> +
> ret = devm_led_classdev_register(dev, cdev);
> KUNIT_ASSERT_EQ(test, ret, 0);
>
> - /* Then make the LED available for lookup */
> lookup.provider = cdev->name;
> lookup.dev_id = dev_name(dev);
> lookup.con_id = "led-test-1";
> +
> led_add_lookup(&lookup);
>
> - /* Finally, attempt to look it up via the API - imagine this was an orthogonal driver */
You have removed all commentary - why?
> cdev_get = devm_led_get(dev, "led-test-1");
> - KUNIT_ASSERT_FALSE(test, IS_ERR(cdev_get));
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_get);
>
> - KUNIT_EXPECT_STREQ(test, cdev_get->name, cdev->name);
> + KUNIT_EXPECT_STREQ(test,
> + cdev_get->name,
> + cdev->name);
Why?
>
> led_remove_lookup(&lookup);
> }
> @@ -93,30 +123,29 @@ static struct kunit_case led_test_cases[] = {
>
> static int led_test_init(struct kunit *test)
> {
> - struct led_test_ddata *ddata;
> + struct led_test_data *data;
> struct device *dev;
>
> - ddata = kunit_kzalloc(test, sizeof(*ddata), GFP_KERNEL);
> - if (!ddata)
> + data = kunit_kzalloc(test, sizeof(*data), GFP_KERNEL);
> + if (!data)
> return -ENOMEM;
>
> - test->priv = ddata;
> -
> dev = kunit_device_register(test, "led_test");
> if (IS_ERR(dev))
> return PTR_ERR(dev);
>
> - ddata->dev = get_device(dev);
> + data->dev = get_device(dev);
> + test->priv = data;
>
> return 0;
> }
>
> static void led_test_exit(struct kunit *test)
> {
> - struct led_test_ddata *ddata = test->priv;
> + struct led_test_data *data = test->priv;
>
> - if (ddata && ddata->dev)
> - put_device(ddata->dev);
> + if (data && data->dev)
> + put_device(data->dev);
> }
>
> static struct kunit_suite led_test_suite = {
> @@ -125,6 +154,7 @@ static struct kunit_suite led_test_suite = {
> .exit = led_test_exit,
> .test_cases = led_test_cases,
> };
> +
Why?
> kunit_test_suite(led_test_suite);
>
> MODULE_AUTHOR("Lee Jones <lee@kernel.org>");
> --
> 2.51.0
>
--
Lee Jones
prev parent reply other threads:[~2026-06-17 8:48 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-12 23:06 [PATCH v2] leds: tests: use a fresh instance for name conflict rejection Lorenzo Egidio
2026-06-17 8:48 ` Lee Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260617084821.GD10056@google.com \
--to=lee@kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=lorenzoegidioms@gmail.com \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.