| Message ID | CWLP123MB547377D20905AF224E682BFBC5EB2@CWLP123MB5473.GBRP123.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | LED1202 / leds-st1202 fixes and improvements | expand |
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", ®); > 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 >
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", ®); > 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 >
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
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
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.
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
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.
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", ®); 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;
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(-)