| Message ID | 20231012034735.804157-1-chunyan.zhang@unisoc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND,V2] leds: sc27xx: Move mutex_init() to the end of probe | expand |
On 10/12/2023 11:47 AM, Chunyan Zhang wrote: > Move the mutex_init() to avoid redundant mutex_destroy() calls after > that for each time the probe fails. > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > --- > Rebased onto linux-next. > > V2: > - Move the mutex_init() to the end of .probe() instead of adding > mutex_destroy() according to Lee's comments. > --- > drivers/leds/leds-sc27xx-bltc.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > index af1f00a2f328..ef57e57ecf07 100644 > --- a/drivers/leds/leds-sc27xx-bltc.c > +++ b/drivers/leds/leds-sc27xx-bltc.c > @@ -296,7 +296,6 @@ static int sc27xx_led_probe(struct platform_device *pdev) > return -ENOMEM; > > platform_set_drvdata(pdev, priv); > - mutex_init(&priv->lock); > priv->base = base; > priv->regmap = dev_get_regmap(dev->parent, NULL); > if (!priv->regmap) { > @@ -309,13 +308,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > err = of_property_read_u32(child, "reg", ®); > if (err) { > of_node_put(child); > - mutex_destroy(&priv->lock); > return err; > } > > if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) { > of_node_put(child); > - mutex_destroy(&priv->lock); > return -EINVAL; > } > > @@ -325,9 +322,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > err = sc27xx_led_register(dev, priv); > if (err) > - mutex_destroy(&priv->lock); > + return err; > > - return err; > + mutex_init(&priv->lock); I think it is better to prepare all the required resources before registering the led device, what I mean is moving mutex_init() before calling sc27xx_led_register().
On Thu, 12 Oct 2023, Baolin Wang wrote: > > > On 10/12/2023 11:47 AM, Chunyan Zhang wrote: > > Move the mutex_init() to avoid redundant mutex_destroy() calls after > > that for each time the probe fails. > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > --- > > Rebased onto linux-next. > > > > V2: > > - Move the mutex_init() to the end of .probe() instead of adding > > mutex_destroy() according to Lee's comments. > > --- > > drivers/leds/leds-sc27xx-bltc.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > > index af1f00a2f328..ef57e57ecf07 100644 > > --- a/drivers/leds/leds-sc27xx-bltc.c > > +++ b/drivers/leds/leds-sc27xx-bltc.c > > @@ -296,7 +296,6 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > return -ENOMEM; > > platform_set_drvdata(pdev, priv); > > - mutex_init(&priv->lock); > > priv->base = base; > > priv->regmap = dev_get_regmap(dev->parent, NULL); > > if (!priv->regmap) { > > @@ -309,13 +308,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > err = of_property_read_u32(child, "reg", ®); > > if (err) { > > of_node_put(child); > > - mutex_destroy(&priv->lock); > > return err; > > } > > if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) { > > of_node_put(child); > > - mutex_destroy(&priv->lock); > > return -EINVAL; > > } > > @@ -325,9 +322,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > err = sc27xx_led_register(dev, priv); > > if (err) > > - mutex_destroy(&priv->lock); > > + return err; > > - return err; > > + mutex_init(&priv->lock); > > I think it is better to prepare all the required resources before > registering the led device, what I mean is moving mutex_init() before > calling sc27xx_led_register(). Is the mutex used before this point? If not, I don't see any reason to initialise it sooner.
On Thu, 12 Oct 2023, Baolin Wang wrote: > > > On 10/12/2023 5:16 PM, Lee Jones wrote: > > On Thu, 12 Oct 2023, Baolin Wang wrote: > > > > > > > > > > > On 10/12/2023 11:47 AM, Chunyan Zhang wrote: > > > > Move the mutex_init() to avoid redundant mutex_destroy() calls after > > > > that for each time the probe fails. > > > > > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > --- > > > > Rebased onto linux-next. > > > > > > > > V2: > > > > - Move the mutex_init() to the end of .probe() instead of adding > > > > mutex_destroy() according to Lee's comments. > > > > --- > > > > drivers/leds/leds-sc27xx-bltc.c | 9 ++++----- > > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > > > > index af1f00a2f328..ef57e57ecf07 100644 > > > > --- a/drivers/leds/leds-sc27xx-bltc.c > > > > +++ b/drivers/leds/leds-sc27xx-bltc.c > > > > @@ -296,7 +296,6 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > > > return -ENOMEM; > > > > platform_set_drvdata(pdev, priv); > > > > - mutex_init(&priv->lock); > > > > priv->base = base; > > > > priv->regmap = dev_get_regmap(dev->parent, NULL); > > > > if (!priv->regmap) { > > > > @@ -309,13 +308,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > > > err = of_property_read_u32(child, "reg", ®); > > > > if (err) { > > > > of_node_put(child); > > > > - mutex_destroy(&priv->lock); > > > > return err; > > > > } > > > > if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) { > > > > of_node_put(child); > > > > - mutex_destroy(&priv->lock); > > > > return -EINVAL; > > > > } > > > > @@ -325,9 +322,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) > > > > err = sc27xx_led_register(dev, priv); > > > > if (err) > > > > - mutex_destroy(&priv->lock); > > > > + return err; > > > > - return err; > > > > + mutex_init(&priv->lock); > > > > > > I think it is better to prepare all the required resources before > > > registering the led device, what I mean is moving mutex_init() before > > > calling sc27xx_led_register(). > > > > Is the mutex used before this point? > > > > If not, I don't see any reason to initialise it sooner. > > When inserting the led module, after registering the led device, users can > set the led brightness or pattern trigger before initializing the mutex, > which will crash the system. I know this may not be an actual scenario, but > this patch opens a small race window, that's what I concerned. If there's a good technical reason to move it, then we should.
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c index af1f00a2f328..ef57e57ecf07 100644 --- a/drivers/leds/leds-sc27xx-bltc.c +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -296,7 +296,6 @@ static int sc27xx_led_probe(struct platform_device *pdev) return -ENOMEM; platform_set_drvdata(pdev, priv); - mutex_init(&priv->lock); priv->base = base; priv->regmap = dev_get_regmap(dev->parent, NULL); if (!priv->regmap) { @@ -309,13 +308,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) err = of_property_read_u32(child, "reg", ®); if (err) { of_node_put(child); - mutex_destroy(&priv->lock); return err; } if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) { of_node_put(child); - mutex_destroy(&priv->lock); return -EINVAL; } @@ -325,9 +322,11 @@ static int sc27xx_led_probe(struct platform_device *pdev) err = sc27xx_led_register(dev, priv); if (err) - mutex_destroy(&priv->lock); + return err; - return err; + mutex_init(&priv->lock); + + return 0; } static void sc27xx_led_remove(struct platform_device *pdev)
Move the mutex_init() to avoid redundant mutex_destroy() calls after that for each time the probe fails. Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> --- Rebased onto linux-next. V2: - Move the mutex_init() to the end of .probe() instead of adding mutex_destroy() according to Lee's comments. --- drivers/leds/leds-sc27xx-bltc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)