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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox