[PATCH RESEND 0/5] LED1202 / leds-st1202 fixes and improvements

[PATCH RESEND 0/5] LED1202 / leds-st1202 fixes and improvements

[Manuel Fombuena]

Collection of patches for the recently added leds-st1202 driver. It is the
same patchset sent on 2025-01-17. The cover letter wasn't properly threaded
with the patches that time, so apologies for that.

Obvious from the individual descriptions, but as a summary:

- 0001: fix a NULL pointer access error that occurs when LEDs are
registered but the LED driver is not fully initialized
- 0002: initialize the LED driver before any DT LED initialization is done
- 0003: some spacing and typo edits
- 0004: include the appropriate select in Kconfig to make sure the
required Pattern trigger driver is available.
- 0005: remove .rst extension on Documentation/leds/index.rst

Manuel Fombuena (5):
  leds: leds-st1202: fix NULL pointer access on race condition
  leds: leds-st1202: initialize hardware before DT node child operations
  leds: leds-st1202: spacing and proofreading editing
  leds: Kconfig: leds-st1202: add select for required
    LEDS_TRIGGER_PATTERN
  Documentation: leds: remove .rst extension for leds-st1202 on index

 Documentation/leds/index.rst |  2 +-
 drivers/leds/Kconfig         |  1 +
 drivers/leds/leds-st1202.c   | 73 ++++++++++++++++++------------------
 3 files changed, 38 insertions(+), 38 deletions(-)

-- 
2.48.1

[PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Manuel Fombuena]

st1202_dt_init() calls devm_led_classdev_register_ext() before the
internal data structures are properly setup, so the leds become visible
to user space while being partially initialized, leading to a window
where trying to access them causes a NULL pointer access.

This change moves devm_led_classdev_register_ext() to the last thing to
happen during initialization to eliminate it.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/leds-st1202.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index b691c4886993..e894b3f9a0f4 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -261,8 +261,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
 	int err, reg;
 
 	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
-		struct led_init_data init_data = {};
-
 		err = of_property_read_u32(child, "reg", &reg);
 		if (err)
 			return dev_err_probe(dev, err, "Invalid register\n");
@@ -276,15 +274,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
 		led->led_cdev.pattern_set = st1202_led_pattern_set;
 		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
 		led->led_cdev.default_trigger = "pattern";
-
-		init_data.fwnode = led->fwnode;
-		init_data.devicename = "st1202";
-		init_data.default_label = ":";
-
-		err = devm_led_classdev_register_ext(dev, &led->led_cdev, &init_data);
-		if (err < 0)
-			return dev_err_probe(dev, err, "Failed to register LED class device\n");
-
 		led->led_cdev.brightness_set = st1202_brightness_set;
 		led->led_cdev.brightness_get = st1202_brightness_get;
 	}
@@ -368,6 +357,7 @@ static int st1202_probe(struct i2c_client *client)
 		return ret;
 
 	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
+		struct led_init_data init_data = {};
 		led = &chip->leds[i];
 		led->chip = chip;
 		led->led_num = i;
@@ -384,6 +374,15 @@ static int st1202_probe(struct i2c_client *client)
 		if (ret < 0)
 			return dev_err_probe(&client->dev, ret,
 					"Failed to clear LED pattern\n");
+
+		init_data.fwnode = led->fwnode;
+		init_data.devicename = "st1202";
+		init_data.default_label = ":";
+
+		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
+		if (ret < 0)
+			return dev_err_probe(&client->dev, ret,
+					"Failed to register LED class device\n");
 	}
 
 	return 0;
-- 
2.48.1

[PATCH RESEND 2/5] leds: leds-st1202: initialize hardware before DT node child operations

[Manuel Fombuena]

Arguably, there are more chances of errors occurring during the
initialization of the hardware, so this should complete successfully
before the DT node childreen are initialized.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/leds-st1202.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index e894b3f9a0f4..927874f20839 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -348,11 +348,11 @@ static int st1202_probe(struct i2c_client *client)
 	devm_mutex_init(&client->dev, &chip->lock);
 	chip->client = client;
 
-	ret = st1202_dt_init(chip);
+	ret = st1202_setup(chip);
 	if (ret < 0)
 		return ret;
 
-	ret = st1202_setup(chip);
+	ret = st1202_dt_init(chip);
 	if (ret < 0)
 		return ret;
 
-- 
2.48.1

[PATCH RESEND 3/5] leds: leds-st1202: spacing and proofreading editing

[Manuel Fombuena]

Minor edits regarding use of spacing and proofreading.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/leds-st1202.c | 48 +++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 927874f20839..cb4797ea8f3a 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -16,27 +16,27 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
-#define ST1202_CHAN_DISABLE_ALL            0x00
-#define ST1202_CHAN_ENABLE_HIGH            0x03
-#define ST1202_CHAN_ENABLE_LOW             0x02
-#define ST1202_CONFIG_REG                  0x04
+#define ST1202_CHAN_DISABLE_ALL				0x00
+#define ST1202_CHAN_ENABLE_HIGH				0x03
+#define ST1202_CHAN_ENABLE_LOW				0x02
+#define ST1202_CONFIG_REG					0x04
 /* PATS: Pattern sequence feature enable */
-#define ST1202_CONFIG_REG_PATS             BIT(7)
+#define ST1202_CONFIG_REG_PATS				BIT(7)
 /* PATSR: Pattern sequence runs (self-clear when sequence is finished) */
-#define ST1202_CONFIG_REG_PATSR            BIT(6)
-#define ST1202_CONFIG_REG_SHFT             BIT(3)
-#define ST1202_DEV_ENABLE                  0x01
-#define ST1202_DEV_ENABLE_ON               BIT(0)
-#define ST1202_DEV_ENABLE_RESET            BIT(7)
-#define ST1202_DEVICE_ID                   0x00
-#define ST1202_ILED_REG0                   0x09
-#define ST1202_MAX_LEDS                    12
-#define ST1202_MAX_PATTERNS                8
-#define ST1202_MILLIS_PATTERN_DUR_MAX      5660
-#define ST1202_MILLIS_PATTERN_DUR_MIN      22
-#define ST1202_PATTERN_DUR                 0x16
-#define ST1202_PATTERN_PWM                 0x1E
-#define ST1202_PATTERN_REP                 0x15
+#define ST1202_CONFIG_REG_PATSR				BIT(6)
+#define ST1202_CONFIG_REG_SHFT				BIT(3)
+#define ST1202_DEV_ENABLE					0x01
+#define ST1202_DEV_ENABLE_ON				BIT(0)
+#define ST1202_DEV_ENABLE_RESET				BIT(7)
+#define ST1202_DEVICE_ID					0x00
+#define ST1202_ILED_REG0					0x09
+#define ST1202_MAX_LEDS						12
+#define ST1202_MAX_PATTERNS					8
+#define ST1202_MILLIS_PATTERN_DUR_MAX		5660
+#define ST1202_MILLIS_PATTERN_DUR_MIN		22
+#define ST1202_PATTERN_DUR					0x16
+#define ST1202_PATTERN_PWM					0x1E
+#define ST1202_PATTERN_REP					0x15
 
 struct st1202_led {
 	struct fwnode_handle *fwnode;
@@ -99,9 +99,9 @@ static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,
 	value_h = (u8)(value >> 8);
 
 	/*
-	 *  Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
-	 *  where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
-	 *  and y is the pattern number in hexadecimal (y = 00h .. 07h)
+	 * Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
+	 * where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
+	 * and y is the pattern number in hexadecimal (y = 00h .. 07h)
 	 */
 	ret = st1202_write_reg(chip, (ST1202_PATTERN_PWM + (led_num * 2) + 0x18 * pattern),
 				value_l);
@@ -288,8 +288,8 @@ static int st1202_setup(struct st1202_chip *chip)
 	guard(mutex)(&chip->lock);
 
 	/*
-	 * Once the supply voltage is applied, the LED1202 executes some internal checks,
-	 * afterwords it stops the oscillator and puts the internal LDO in quiescent mode.
+	 * Once the supply voltage is applied, the LED1202 executes some internal checks.
+	 * Afterwards, it stops the oscillator and puts the internal LDO in quiescent mode.
 	 * To start the device, EN bit must be set inside the “Device Enable” register at
 	 * address 01h. As soon as EN is set, the LED1202 loads the adjustment parameters
 	 * from the internal non-volatile memory and performs an auto-calibration procedure
-- 
2.48.1

[PATCH RESEND 4/5] leds: Kconfig: leds-st1202: add select for required LEDS_TRIGGER_PATTERN

[Manuel Fombuena]

leds-st1202 requires the LED Pattern Trigger (LEDS_TRIGGER_PATTERN), which
is not selected when LED Trigger support is (LEDS_TRIGGERS).

To reproduce this:

- make menuconfig KCONFIG_CONFIG=
- select LEDS_ST1202 dependencies OF, I2C and LEDS_CLASS.
- select LEDS_ST1202
- LEDS_TRIGGERS is selected but LEDS_TRIGGER_PATTERN isn't.

Add select LEDS_TRIGGER_PATTERN to Kconfig to meet the requirement and
indirectly document it as well.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6efd514bfb48..b585548c51cb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -960,6 +960,7 @@ config LEDS_ST1202
 	depends on I2C
 	depends on OF
 	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_PATTERN
 	help
 	  Say Y to enable support for LEDs connected to LED1202
 	  LED driver chips accessed via the I2C bus.
-- 
2.48.1

[PATCH RESEND 5/5] Documentation: leds: remove .rst extension for leds-st1202 on index

[Manuel Fombuena]

No other LED driver is listed on index.rst with the .rst extension.
Remove it.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 Documentation/leds/index.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index 0ab0a2128a11..76fae171039c 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -28,5 +28,5 @@ LEDs
    leds-mlxcpld
    leds-mt6370-rgb
    leds-sc27xx
-   leds-st1202.rst
+   leds-st1202
    leds-qcom-lpg
-- 
2.48.1

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Lee Jones]

On Sat, 01 Feb 2025, Manuel Fombuena wrote:

> st1202_dt_init() calls devm_led_classdev_register_ext() before the
> internal data structures are properly setup, so the leds become visible
> to user space while being partially initialized, leading to a window
> where trying to access them causes a NULL pointer access.

If this is true, you need to provide a Fixes: tag and to Cc: Stable.

Documentation/process/stable-kernel-rules.rst

> This change moves devm_led_classdev_register_ext() to the last thing to
> happen during initialization to eliminate it.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/leds-st1202.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b691c4886993..e894b3f9a0f4 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -261,8 +261,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  	int err, reg;
>  
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> -		struct led_init_data init_data = {};
> -
>  		err = of_property_read_u32(child, "reg", &reg);
>  		if (err)
>  			return dev_err_probe(dev, err, "Invalid register\n");
> @@ -276,15 +274,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> -
> -		init_data.fwnode = led->fwnode;
> -		init_data.devicename = "st1202";
> -		init_data.default_label = ":";
> -
> -		err = devm_led_classdev_register_ext(dev, &led->led_cdev, &init_data);
> -		if (err < 0)
> -			return dev_err_probe(dev, err, "Failed to register LED class device\n");
> -
>  		led->led_cdev.brightness_set = st1202_brightness_set;
>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}
> @@ -368,6 +357,7 @@ static int st1202_probe(struct i2c_client *client)
>  		return ret;
>  
>  	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		struct led_init_data init_data = {};
>  		led = &chip->leds[i];
>  		led->chip = chip;
>  		led->led_num = i;
> @@ -384,6 +374,15 @@ static int st1202_probe(struct i2c_client *client)
>  		if (ret < 0)
>  			return dev_err_probe(&client->dev, ret,
>  					"Failed to clear LED pattern\n");
> +
> +		init_data.fwnode = led->fwnode;
> +		init_data.devicename = "st1202";
> +		init_data.default_label = ":";
> +
> +		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> +		if (ret < 0)
> +			return dev_err_probe(&client->dev, ret,
> +					"Failed to register LED class device\n");
>  	}
>  
>  	return 0;
> -- 
> 2.48.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 2/5] leds: leds-st1202: initialize hardware before DT node child operations

[Lee Jones]

On Sat, 01 Feb 2025, Manuel Fombuena wrote:

> Arguably, there are more chances of errors occurring during the
> initialization of the hardware, so this should complete successfully
> before the DT node childreen are initialized.

Okay.  And you're sure nothing in Setup needs the DT info?

> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/leds-st1202.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index e894b3f9a0f4..927874f20839 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -348,11 +348,11 @@ static int st1202_probe(struct i2c_client *client)
>  	devm_mutex_init(&client->dev, &chip->lock);
>  	chip->client = client;
>  
> -	ret = st1202_dt_init(chip);
> +	ret = st1202_setup(chip);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = st1202_setup(chip);
> +	ret = st1202_dt_init(chip);
>  	if (ret < 0)
>  		return ret;
>  
> -- 
> 2.48.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 3/5] leds: leds-st1202: spacing and proofreading editing

[Lee Jones]

On Sat, 01 Feb 2025, Manuel Fombuena wrote:

> Minor edits regarding use of spacing and proofreading.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/leds-st1202.c | 48 +++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 927874f20839..cb4797ea8f3a 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -16,27 +16,27 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  
> -#define ST1202_CHAN_DISABLE_ALL            0x00
> -#define ST1202_CHAN_ENABLE_HIGH            0x03
> -#define ST1202_CHAN_ENABLE_LOW             0x02
> -#define ST1202_CONFIG_REG                  0x04
> +#define ST1202_CHAN_DISABLE_ALL				0x00
> +#define ST1202_CHAN_ENABLE_HIGH				0x03
> +#define ST1202_CHAN_ENABLE_LOW				0x02
> +#define ST1202_CONFIG_REG					0x04
>  /* PATS: Pattern sequence feature enable */
> -#define ST1202_CONFIG_REG_PATS             BIT(7)
> +#define ST1202_CONFIG_REG_PATS				BIT(7)
>  /* PATSR: Pattern sequence runs (self-clear when sequence is finished) */
> -#define ST1202_CONFIG_REG_PATSR            BIT(6)
> -#define ST1202_CONFIG_REG_SHFT             BIT(3)
> -#define ST1202_DEV_ENABLE                  0x01
> -#define ST1202_DEV_ENABLE_ON               BIT(0)
> -#define ST1202_DEV_ENABLE_RESET            BIT(7)
> -#define ST1202_DEVICE_ID                   0x00
> -#define ST1202_ILED_REG0                   0x09
> -#define ST1202_MAX_LEDS                    12
> -#define ST1202_MAX_PATTERNS                8
> -#define ST1202_MILLIS_PATTERN_DUR_MAX      5660
> -#define ST1202_MILLIS_PATTERN_DUR_MIN      22
> -#define ST1202_PATTERN_DUR                 0x16
> -#define ST1202_PATTERN_PWM                 0x1E
> -#define ST1202_PATTERN_REP                 0x15
> +#define ST1202_CONFIG_REG_PATSR				BIT(6)
> +#define ST1202_CONFIG_REG_SHFT				BIT(3)
> +#define ST1202_DEV_ENABLE					0x01
> +#define ST1202_DEV_ENABLE_ON				BIT(0)
> +#define ST1202_DEV_ENABLE_RESET				BIT(7)
> +#define ST1202_DEVICE_ID					0x00
> +#define ST1202_ILED_REG0					0x09
> +#define ST1202_MAX_LEDS						12
> +#define ST1202_MAX_PATTERNS					8
> +#define ST1202_MILLIS_PATTERN_DUR_MAX		5660
> +#define ST1202_MILLIS_PATTERN_DUR_MIN		22
> +#define ST1202_PATTERN_DUR					0x16
> +#define ST1202_PATTERN_PWM					0x1E
> +#define ST1202_PATTERN_REP					0x15

Why are you making this change?

The spacings went from being okay to too large.

>  struct st1202_led {
>  	struct fwnode_handle *fwnode;
> @@ -99,9 +99,9 @@ static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,
>  	value_h = (u8)(value >> 8);
>  
>  	/*
> -	 *  Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
> -	 *  where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
> -	 *  and y is the pattern number in hexadecimal (y = 00h .. 07h)
> +	 * Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
> +	 * where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
> +	 * and y is the pattern number in hexadecimal (y = 00h .. 07h)
>  	 */
>  	ret = st1202_write_reg(chip, (ST1202_PATTERN_PWM + (led_num * 2) + 0x18 * pattern),
>  				value_l);
> @@ -288,8 +288,8 @@ static int st1202_setup(struct st1202_chip *chip)
>  	guard(mutex)(&chip->lock);
>  
>  	/*
> -	 * Once the supply voltage is applied, the LED1202 executes some internal checks,
> -	 * afterwords it stops the oscillator and puts the internal LDO in quiescent mode.
> +	 * Once the supply voltage is applied, the LED1202 executes some internal checks.
> +	 * Afterwards, it stops the oscillator and puts the internal LDO in quiescent mode.
>  	 * To start the device, EN bit must be set inside the “Device Enable” register at
>  	 * address 01h. As soon as EN is set, the LED1202 loads the adjustment parameters
>  	 * from the internal non-volatile memory and performs an auto-calibration procedure
> -- 
> 2.48.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Lee Jones]

On Sat, 01 Feb 2025, Manuel Fombuena wrote:

> st1202_dt_init() calls devm_led_classdev_register_ext() before the
> internal data structures are properly setup, so the leds become visible

Always "LEDs".

> to user space while being partially initialized, leading to a window
> where trying to access them causes a NULL pointer access.
> 
> This change moves devm_led_classdev_register_ext() to the last thing to
> happen during initialization to eliminate it.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/leds-st1202.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b691c4886993..e894b3f9a0f4 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -261,8 +261,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  	int err, reg;
>  
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> -		struct led_init_data init_data = {};
> -
>  		err = of_property_read_u32(child, "reg", &reg);
>  		if (err)
>  			return dev_err_probe(dev, err, "Invalid register\n");
> @@ -276,15 +274,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> -
> -		init_data.fwnode = led->fwnode;
> -		init_data.devicename = "st1202";
> -		init_data.default_label = ":";
> -
> -		err = devm_led_classdev_register_ext(dev, &led->led_cdev, &init_data);
> -		if (err < 0)
> -			return dev_err_probe(dev, err, "Failed to register LED class device\n");
> -
>  		led->led_cdev.brightness_set = st1202_brightness_set;
>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}
> @@ -368,6 +357,7 @@ static int st1202_probe(struct i2c_client *client)
>  		return ret;
>  
>  	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		struct led_init_data init_data = {};
>  		led = &chip->leds[i];
>  		led->chip = chip;
>  		led->led_num = i;
> @@ -384,6 +374,15 @@ static int st1202_probe(struct i2c_client *client)
>  		if (ret < 0)
>  			return dev_err_probe(&client->dev, ret,
>  					"Failed to clear LED pattern\n");
> +
> +		init_data.fwnode = led->fwnode;
> +		init_data.devicename = "st1202";
> +		init_data.default_label = ":";
> +
> +		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> +		if (ret < 0)
> +			return dev_err_probe(&client->dev, ret,
> +					"Failed to register LED class device\n");
>  	}
>  
>  	return 0;
> -- 
> 2.48.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Manuel Fombuena]

On Tue, 11 Feb 2025, Lee Jones wrote:

> On Sat, 01 Feb 2025, Manuel Fombuena wrote:
> 
> > st1202_dt_init() calls devm_led_classdev_register_ext() before the
> > internal data structures are properly setup, so the leds become visible
> 
> Always "LEDs".

Noted, thank you.

--
Manuel Fombuena

Re: [PATCH RESEND 3/5] leds: leds-st1202: spacing and proofreading editing

[Manuel Fombuena]

On Tue, 11 Feb 2025, Lee Jones wrote:

> > +#define ST1202_MILLIS_PATTERN_DUR_MAX		5660
> > +#define ST1202_MILLIS_PATTERN_DUR_MIN		22
> > +#define ST1202_PATTERN_DUR					0x16
> > +#define ST1202_PATTERN_PWM					0x1E
> > +#define ST1202_PATTERN_REP					0x15
> 
> Why are you making this change?
> 
> The spacings went from being okay to too large.

Originally the code uses blank spaces for alignment, but changing it to 
tabs leads to inconsistency across viewers / editors so I will just drop 
it. 

--
Manuel Fombuena <fombuena@outlook.com>

Re: [PATCH RESEND 2/5] leds: leds-st1202: initialize hardware before DT node child operations

[Manuel Fombuena]

On Tue, 11 Feb 2025, Lee Jones wrote:

> On Sat, 01 Feb 2025, Manuel Fombuena wrote:
> 
> > Arguably, there are more chances of errors occurring during the
> > initialization of the hardware, so this should complete successfully
> > before the DT node childreen are initialized.
> 
> Okay.  And you're sure nothing in Setup needs the DT info?

Yes, st1202_setup() doesn't require anything previously done in  
st1202_dt_init() to do its thing.

Additionally, I am not just relying on reviewing the code. I am also 
carrying out real-world testing on a device I use daily and it works 
either way.

--
Manuel Fombuena

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Manuel Fombuena]

On Tue, 11 Feb 2025, Lee Jones wrote:

> On Sat, 01 Feb 2025, Manuel Fombuena wrote:
> 
> > st1202_dt_init() calls devm_led_classdev_register_ext() before the
> > internal data structures are properly setup, so the leds become visible
> > to user space while being partially initialized, leading to a window
> > where trying to access them causes a NULL pointer access.
> 
> If this is true, you need to provide a Fixes: tag and to Cc: Stable.
>
> Documentation/process/stable-kernel-rules.rst
> 

Yes, some circumstances have to confluence but it is reproducible under 
normal use. I can send panic logs if you want to see the details.

Since this driver has been added in 6.14-rc1, I thought that, if applied,    
this patch would be included in fixes before the final release and there 
would be no need to apply it to -stable trees, as it is nowhere else at 
the moment. But of course I will review the documentation and make changes as 
suggested.

-- Manuel Fombuena

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Lee Jones]

On Wed, 12 Feb 2025, Manuel Fombuena wrote:

> On Tue, 11 Feb 2025, Lee Jones wrote:
> 
> > On Sat, 01 Feb 2025, Manuel Fombuena wrote:
> > 
> > > st1202_dt_init() calls devm_led_classdev_register_ext() before the
> > > internal data structures are properly setup, so the leds become visible
> > > to user space while being partially initialized, leading to a window
> > > where trying to access them causes a NULL pointer access.
> > 
> > If this is true, you need to provide a Fixes: tag and to Cc: Stable.
> >
> > Documentation/process/stable-kernel-rules.rst
> > 
> 
> Yes, some circumstances have to confluence but it is reproducible under 
> normal use. I can send panic logs if you want to see the details.
> 
> Since this driver has been added in 6.14-rc1, I thought that, if applied,    
> this patch would be included in fixes before the final release and there 
> would be no need to apply it to -stable trees, as it is nowhere else at 
> the moment. But of course I will review the documentation and make changes as 
> suggested.

Then you need to separate the set into patches you expect to be
submitted to the -rcs and ones which can be applied during the next
cycle, then go to lengths to explain that either in the diff section of
each patch (preferred) or in the cover-letter.

Either way, you need Fixes: tags.

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 0/5] LED1202 / leds-st1202 fixes and improvements

[Lee Jones]

On Sat, 01 Feb 2025, Manuel Fombuena wrote:

> Collection of patches for the recently added leds-st1202 driver. It is the
> same patchset sent on 2025-01-17. The cover letter wasn't properly threaded
> with the patches that time, so apologies for that.
> 
> Obvious from the individual descriptions, but as a summary:
> 
> - 0001: fix a NULL pointer access error that occurs when LEDs are
> registered but the LED driver is not fully initialized
> - 0002: initialize the LED driver before any DT LED initialization is done
> - 0003: some spacing and typo edits
> - 0004: include the appropriate select in Kconfig to make sure the
> required Pattern trigger driver is available.
> - 0005: remove .rst extension on Documentation/leds/index.rst

Stripping the separators from patch file names and pasting them
culminates in a terrible summary.  In no way does this cover-letter
describe what you're trying to achieve, why you're trying to achieve it
and the consequences for not applying the set.  Nor does it communicate
any merge intentions (which is required due to the assumptions made, as
described in our previous conversation).

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Manuel Fombuena]

On Thu, 13 Feb 2025, Lee Jones wrote:

> Then you need to separate the set into patches you expect to be
> submitted to the -rcs and ones which can be applied during the next
> cycle, then go to lengths to explain that either in the diff section of
> each patch (preferred) or in the cover-letter.

One question so I don't take more of your time later on on this. Should I 
continue the set with 5 patches as v2, applying the above and the other 
comments, or would it be preferable to send this patch with its 
cover letter separately and drop it from this set?

--
Manuel Fombuena

Re: [PATCH RESEND 0/5] LED1202 / leds-st1202 fixes and improvements

[Manuel Fombuena]

On Thu, 13 Feb 2025, Lee Jones wrote:

> Stripping the separators from patch file names and pasting them
> culminates in a terrible summary.  In no way does this cover-letter
> describe what you're trying to achieve, why you're trying to achieve it
> and the consequences for not applying the set.  Nor does it communicate
> any merge intentions (which is required due to the assumptions made, as
> described in our previous conversation).

Do the messages in the diff section of the patches need similar 
improvements?

--
Manuel Fombuena

Re: [PATCH RESEND 1/5] leds: leds-st1202: fix NULL pointer access on race condition

[Lee Jones]

On Thu, 13 Feb 2025, Manuel Fombuena wrote:

> On Thu, 13 Feb 2025, Lee Jones wrote:
> 
> > Then you need to separate the set into patches you expect to be
> > submitted to the -rcs and ones which can be applied during the next
> > cycle, then go to lengths to explain that either in the diff section of
> > each patch (preferred) or in the cover-letter.
> 
> One question so I don't take more of your time later on on this. Should I 
> continue the set with 5 patches as v2, applying the above and the other 
> comments, or would it be preferable to send this patch with its 
> cover letter separately and drop it from this set?

This and any other patches due for the -rcs need splitting out.

Please submit them all again.

-- 
Lee Jones [李琼斯]

Re: [PATCH RESEND 0/5] LED1202 / leds-st1202 fixes and improvements

[Lee Jones]

On Thu, 13 Feb 2025, Manuel Fombuena wrote:

> On Thu, 13 Feb 2025, Lee Jones wrote:
> 
> > Stripping the separators from patch file names and pasting them
> > culminates in a terrible summary.  In no way does this cover-letter
> > describe what you're trying to achieve, why you're trying to achieve it
> > and the consequences for not applying the set.  Nor does it communicate
> > any merge intentions (which is required due to the assumptions made, as
> > described in our previous conversation).
> 
> Do the messages in the diff section of the patches need similar 
> improvements?

You can use the patch-level diff sections to add per-patch changelogs.

-- 
Lee Jones [李琼斯]