* [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe
@ 2023-10-12 3:47 Chunyan Zhang
2023-10-12 7:29 ` Baolin Wang
0 siblings, 1 reply; 5+ messages in thread
From: Chunyan Zhang @ 2023-10-12 3:47 UTC (permalink / raw)
To: Pavel Machek, Lee Jones
Cc: linux-leds, Baolin Wang, Orson Zhai, Chunyan Zhang, Chunyan Zhang, LKML
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);
+
+ return 0;
}
static void sc27xx_led_remove(struct platform_device *pdev)
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe
2023-10-12 3:47 [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe Chunyan Zhang
@ 2023-10-12 7:29 ` Baolin Wang
2023-10-12 9:16 ` Lee Jones
0 siblings, 1 reply; 5+ messages in thread
From: Baolin Wang @ 2023-10-12 7:29 UTC (permalink / raw)
To: Chunyan Zhang, Pavel Machek, Lee Jones
Cc: linux-leds, Orson Zhai, Chunyan Zhang, LKML
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().
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe
2023-10-12 7:29 ` Baolin Wang
@ 2023-10-12 9:16 ` Lee Jones
2023-10-12 11:43 ` Baolin Wang
0 siblings, 1 reply; 5+ messages in thread
From: Lee Jones @ 2023-10-12 9:16 UTC (permalink / raw)
To: Baolin Wang
Cc: Chunyan Zhang, Pavel Machek, linux-leds, Orson Zhai, Chunyan Zhang, LKML
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.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe
2023-10-12 9:16 ` Lee Jones
@ 2023-10-12 11:43 ` Baolin Wang
2023-10-13 10:19 ` Lee Jones
0 siblings, 1 reply; 5+ messages in thread
From: Baolin Wang @ 2023-10-12 11:43 UTC (permalink / raw)
To: Lee Jones
Cc: Chunyan Zhang, Pavel Machek, linux-leds, Orson Zhai, Chunyan Zhang, LKML
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe
2023-10-12 11:43 ` Baolin Wang
@ 2023-10-13 10:19 ` Lee Jones
0 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2023-10-13 10:19 UTC (permalink / raw)
To: Baolin Wang
Cc: Chunyan Zhang, Pavel Machek, linux-leds, Orson Zhai, Chunyan Zhang, LKML
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.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-13 10:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-12 3:47 [RESEND PATCH V2] leds: sc27xx: Move mutex_init() to the end of probe Chunyan Zhang
2023-10-12 7:29 ` Baolin Wang
2023-10-12 9:16 ` Lee Jones
2023-10-12 11:43 ` Baolin Wang
2023-10-13 10:19 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).