| Message ID | 20210510095045.3299382-1-andy.shevchenko@gmail.com |
|---|---|
| Headers | show |
| Series | leds: cleanups and fwnode refcounting bug fixes | expand |
On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > When analyzing the current state of affairs with fwnode reference counting > I found that a lot of core doesn't take it right. Here is a bunch of > corresponding fixes against LED drivers. > > The series includes some cleanups and a few other fixes grouped by a driver. > > First two patches are taking care of -ENOTSUPP error code too prevent its > appearance in the user space. Pavel, any comments on this bug fix series? -- With Best Regards, Andy Shevchenko
On Mon, May 17, 2021 at 10:30:08AM +0300, Andy Shevchenko wrote: > On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > > When analyzing the current state of affairs with fwnode reference counting > > I found that a lot of core doesn't take it right. Here is a bunch of > > corresponding fixes against LED drivers. > > > > The series includes some cleanups and a few other fixes grouped by a driver. > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > appearance in the user space. > > Pavel, any comments on this bug fix series? Pavel, we are at rc-3 already and this is kinda a big series that needs more time to be sit in Linux-next, unfortunately while I see your activities here and there, it is kept uncommented and unapplied. Can you shed a light what's going on here? Do I need something to be amended? -- With Best Regards, Andy Shevchenko
On Mon 2021-05-24 17:56:13, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:30:08AM +0300, Andy Shevchenko wrote: > > On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > > > When analyzing the current state of affairs with fwnode reference counting > > > I found that a lot of core doesn't take it right. Here is a bunch of > > > corresponding fixes against LED drivers. > > > > > > The series includes some cleanups and a few other fixes grouped by a driver. > > > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > > appearance in the user space. > > > > Pavel, any comments on this bug fix series? > > Pavel, we are at rc-3 already and this is kinda a big series that needs more > time to be sit in Linux-next, unfortunately while I see your activities here > and there, it is kept uncommented and unapplied. Can you shed a light what's > going on here? Do I need something to be amended? I'm busy, sorry. Series looks kind of okay on quick look. Pavel -- http://www.livejournal.com/~pavelmachek
On Mon, May 24, 2021 at 07:49:03PM +0200, Pavel Machek wrote: > On Mon 2021-05-24 17:56:13, Andy Shevchenko wrote: > > On Mon, May 17, 2021 at 10:30:08AM +0300, Andy Shevchenko wrote: > > > On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > > > > When analyzing the current state of affairs with fwnode reference counting > > > > I found that a lot of core doesn't take it right. Here is a bunch of > > > > corresponding fixes against LED drivers. > > > > > > > > The series includes some cleanups and a few other fixes grouped by a driver. > > > > > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > > > appearance in the user space. > > > > > > Pavel, any comments on this bug fix series? > > > > Pavel, we are at rc-3 already and this is kinda a big series that needs more > > time to be sit in Linux-next, unfortunately while I see your activities here > > and there, it is kept uncommented and unapplied. Can you shed a light what's > > going on here? Do I need something to be amended? > > I'm busy, sorry. Oh, I see. Good you eventually answered! > Series looks kind of okay on quick look. I'll look forward to see it applied at some point in the future, thanks! -- With Best Regards, Andy Shevchenko
On Mon 2021-05-17 10:30:08, Andy Shevchenko wrote: > On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > > When analyzing the current state of affairs with fwnode reference counting > > I found that a lot of core doesn't take it right. Here is a bunch of > > corresponding fixes against LED drivers. > > > > The series includes some cleanups and a few other fixes grouped by a driver. > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > appearance in the user space. > > Pavel, any comments on this bug fix series? I took these: 95138e01275e1af3f1fc2780fe1d9c6138b29c7a leds: pwm: Make error handling more robust d33e98a1f3ee76f38668304f9ffce49af07da77a leds: lt3593: Make use of device properties f1e1d532da7e6ef355528a22fb97d9a8fbf76c4e leds: lp50xx: Put fwnode in error case during ->probe() 807553f8bf4afa673750e52905e0f9488179112f leds: lm3697: Don't spam logs when probe is deferred f55db1c7fadc2a29c9fa4ff3aec98dbb111f2206 leds: lm3692x: Put fwnode in any case during ->probe() e2e8e4e8187509a77cb6328d876d9c09c07c2e82 leds: lm36274: Correct headers (of*.h -> mod_devicetable.h) 3c5f655c44bb65cb7e3c219d08c130ce5fa45d7f leds: lm36274: Put fwnode in error case during ->probe() 2f39f68cec0a19c0371c1e7cb149159810e87f64 leds: lm3532: Make error handling more robust 99be74f61cb0292b518f5e6d7e5c6611555c2ec7 leds: lm3532: select regmap I2C API f3e2b3825ffb034b001fbe283d7a32a56e41aca7 leds: lgm-sso: Drop duplicate NULL check for GPIO operations 2cbbe9c50d13b6417e0baf8e8475ed73d4d12c2d leds: lgm-sso: Remove unneeded of_match_ptr() fba8a6f2263bc54683cf3fd75df4dbd5d041c195 leds: lgm-sso: Fix clock handling a43a4e588e72bafc81924d61bf1167cd6810ecbb leds: el15203000: Introduce to_el15203000_led() helper 0ac40af86077982a5346dbc9655172d2775d6b08 leds: class: The -ENOTSUPP should never be seen by user space For the "remove depends on OF"... I'd preffer not to take those. We don't need to ask the user for configurations that never happen. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
On Mon 2021-05-10 12:50:20, Andy Shevchenko wrote: > 1 microsecond with 20 millisecond parameter is too low margin for > usleep_range(). Give 100 to make scheduler happier. > > While at it, fix indentation in cases where EL_FW_DELAY_USEC is in use. > In the loop, move it to the end to avoid a conditional. Its not like unhappy schedulers are problem... Pavel -- http://www.livejournal.com/~pavelmachek
Hi! > @@ -734,10 +736,15 @@ static int sso_led_dt_parse(struct sso_led_priv *priv) > if (fw_ssoled) { > ret = __sso_led_dt_parse(priv, fw_ssoled); > if (ret) > - return ret; > + goto err_child_out; > } > > + fwnode_handle_put(fw_ssoled); > return 0; > + > +err_child_out: > + fwnode_handle_put(fw_ssoled); > + return ret; > } Just delete the return and you get the same effect, no? No need to have two exits here. BR, Pavel -- http://www.livejournal.com/~pavelmachek
On Mon 2021-05-10 12:50:28, Andy Shevchenko wrote: > The idea of managed resources that they will be cleaned up automatically > and in the proper order. Remove explicit cleanups. Are you really sure this is good idea with the regmap_update_bits in between? BR, Pavel > --- > drivers/leds/blink/leds-lgm-sso.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/leds/blink/leds-lgm-sso.c b/drivers/leds/blink/leds-lgm-sso.c > index e76be25480b4..a7f2e5436ba2 100644 > --- a/drivers/leds/blink/leds-lgm-sso.c > +++ b/drivers/leds/blink/leds-lgm-sso.c > @@ -606,16 +606,10 @@ static void sso_led_shutdown(struct sso_led *led) > { > struct sso_led_priv *priv = led->priv; > > - /* unregister led */ > - devm_led_classdev_unregister(priv->dev, &led->cdev); > - > /* clear HW control bit */ > if (led->desc.hw_trig) > regmap_update_bits(priv->mmap, SSO_CON3, BIT(led->desc.pin), 0); > > - if (led->gpiod) > - devm_gpiod_put(priv->dev, led->gpiod); > - > led->priv = NULL; > } > -- http://www.livejournal.com/~pavelmachek
On Mon 2021-05-10 12:50:38, Andy Shevchenko wrote: > It's easy to miss necessary clean up, e.g. firmware node reference counting, > during error path in ->probe(). Make it more robust by moving to a single > point of return. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> You are now putting the handle even in the success case. Is that right? Pavel > --- > drivers/leds/leds-lm3697.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c > index 9d35dd2a9bf0..6262ae69591e 100644 > --- a/drivers/leds/leds-lm3697.c > +++ b/drivers/leds/leds-lm3697.c > @@ -224,14 +224,12 @@ static int lm3697_probe_dt(struct lm3697 *priv) > ret = fwnode_property_read_u32(child, "reg", &control_bank); > if (ret) { > dev_err(dev, "reg property missing\n"); > - fwnode_handle_put(child); > goto child_out; > } > > if (control_bank > LM3697_CONTROL_B) { > dev_err(dev, "reg property is invalid\n"); > ret = -EINVAL; > - fwnode_handle_put(child); > goto child_out; > } > > @@ -262,7 +260,6 @@ static int lm3697_probe_dt(struct lm3697 *priv) > led->num_leds); > if (ret) { > dev_err(dev, "led-sources property missing\n"); > - fwnode_handle_put(child); > goto child_out; > } > > @@ -287,7 +284,6 @@ static int lm3697_probe_dt(struct lm3697 *priv) > &init_data); > if (ret) { > dev_err(dev, "led register err: %d\n", ret); > - fwnode_handle_put(child); > goto child_out; > } > > @@ -295,6 +291,7 @@ static int lm3697_probe_dt(struct lm3697 *priv) > } > > child_out: > + fwnode_handle_put(child); > return ret; > } > -- http://www.livejournal.com/~pavelmachek
On Fri, May 28, 2021 at 12:04:40PM +0200, Pavel Machek wrote: > On Mon 2021-05-10 12:50:20, Andy Shevchenko wrote: > > 1 microsecond with 20 millisecond parameter is too low margin for > > usleep_range(). Give 100 to make scheduler happier. > > > > While at it, fix indentation in cases where EL_FW_DELAY_USEC is in use. > > In the loop, move it to the end to avoid a conditional. > > Its not like unhappy schedulers are problem... Any hints then? To me it sounds like torturing scheduler is the real problem and that's why scheduler is unhappy. -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 12:08:00PM +0200, Pavel Machek wrote: > > @@ -734,10 +736,15 @@ static int sso_led_dt_parse(struct sso_led_priv *priv) > > if (fw_ssoled) { > > ret = __sso_led_dt_parse(priv, fw_ssoled); > > if (ret) > > - return ret; > > + goto err_child_out; > > } > > > > + fwnode_handle_put(fw_ssoled); > > return 0; > > + > > +err_child_out: > > + fwnode_handle_put(fw_ssoled); > > + return ret; > > } > > Just delete the return and you get the same effect, no? No need to > have two exits here. I prefer to see it clear and follow the same pattern, but if you insist, I can change in proposed way. Thanks for review! -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 12:09:06PM +0200, Pavel Machek wrote: > On Mon 2021-05-10 12:50:28, Andy Shevchenko wrote: > > The idea of managed resources that they will be cleaned up automatically > > and in the proper order. Remove explicit cleanups. > > Are you really sure this is good idea with the regmap_update_bits in > between? Good question. I will check it. > > - /* unregister led */ > > - devm_led_classdev_unregister(priv->dev, &led->cdev); > > - > > /* clear HW control bit */ > > if (led->desc.hw_trig) > > regmap_update_bits(priv->mmap, SSO_CON3, BIT(led->desc.pin), 0); > > > > - if (led->gpiod) > > - devm_gpiod_put(priv->dev, led->gpiod); > > - > > led->priv = NULL; -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 12:10:57PM +0200, Pavel Machek wrote: > On Mon 2021-05-10 12:50:38, Andy Shevchenko wrote: > > It's easy to miss necessary clean up, e.g. firmware node reference counting, > > during error path in ->probe(). Make it more robust by moving to a single > > point of return. > > > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > You are now putting the handle even in the success case. Is that > right? Let's put it this way: it's no-op in successful case. But yeah, I would prefer to have a separate case for error, I'll revisit this. -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 12:02:54PM +0200, Pavel Machek wrote: > On Mon 2021-05-17 10:30:08, Andy Shevchenko wrote: > > On Mon, May 10, 2021 at 12:50:17PM +0300, Andy Shevchenko wrote: > > > When analyzing the current state of affairs with fwnode reference counting > > > I found that a lot of core doesn't take it right. Here is a bunch of > > > corresponding fixes against LED drivers. > > > > > > The series includes some cleanups and a few other fixes grouped by a driver. > > > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > > appearance in the user space. > > > > Pavel, any comments on this bug fix series? > > I took these: Thanks! What branch/tree should I rebase the rest on? > For the "remove depends on OF"... I'd preffer not to take those. We > don't need to ask the user for configurations that never happen. What do you mean by this? ACPI is quite a good configuration to make use of it on the corresponding platforms. By default any discrete LED driver (in hardware term here) IC should be considered independent from the type of the platform description. Do you agree? If so, it means that dropping OF dependency is a right thing to do to allow users of those ICs to be happy even on ACPI based platforms. Note, entire IIO subsystem is a good example of this activity. All the sensors can be used now in ACPI environment without explicit requirement to have an ACPI ID, although it's highly recommended to acquire for the real products (not DIY ones). -- With Best Regards, Andy Shevchenko
Hi! > > > > First two patches are taking care of -ENOTSUPP error code too prevent its > > > > appearance in the user space. > > > > > > Pavel, any comments on this bug fix series? > > > > I took these: > > Thanks! > > What branch/tree should I rebase the rest on? git@gitolite.kernel.org:pub/scm/linux/kernel/git/pavel/linux-leds.git for-next would do the trick. As would linux-next, I guess. This area should not be changing. > > For the "remove depends on OF"... I'd preffer not to take those. We > > don't need to ask the user for configurations that never happen. > > What do you mean by this? ACPI is quite a good configuration to make use > of it on the corresponding platforms. By default any discrete LED driver > (in hardware term here) IC should be considered independent from the type > of the platform description. Do you agree? If so, it means that The drivers are independend, I guess. But I'm also very sure you will not find some of the chips in a ACPI based machine. el15203000 is such example. I don't want people configuring for normal PCs to be asked if they want el15203000 support. If you know particular chip is present in ACPI-based machine, I'm okay with removing the dependency. (Maybe some of these chould depend on ARM || COMPILE_TEST instead?) > > dropping > OF dependency is a right thing to do to allow users of those ICs to be happy > even on ACPI based platforms. > > Note, entire IIO subsystem is a good example of this activity. All the sensors > can be used now in ACPI environment without explicit requirement to have an > ACPI ID, although it's highly recommended to acquire for the real products > (not DIY ones). Well. I'm not sure that is good step forward. It will result in useless questions being asked. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
10.05.21 12:50, Andy Shevchenko пише: > There is no user of of*.h headers, but mod_devicetable.h. > Update header block accordingly. > > While at it, drop unneeded dependency to OF. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/leds/Kconfig | 1 - > drivers/leds/leds-el15203000.c | 3 ++- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index dcbfcd491fd0..531c79155717 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -167,7 +167,6 @@ config LEDS_EL15203000 > tristate "LED Support for Crane EL15203000" > depends on LEDS_CLASS > depends on SPI > - depends on OF > help > This option enables support for EL15203000 LED Board > (aka RED LED board) which is widely used in coffee vending > diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c > index bcdbbbc9c187..fcb90d7cd42f 100644 > --- a/drivers/leds/leds-el15203000.c > +++ b/drivers/leds/leds-el15203000.c > @@ -4,8 +4,9 @@ > > #include <linux/delay.h> > #include <linux/leds.h> > +#include <linux/mod_devicetable.h> > #include <linux/module.h> > -#include <linux/of_device.h> > +#include <linux/property.h> > #include <linux/spi/spi.h> > > /* > Reviewed-by: Oleh Kravchenko <oleg@kaa.org.ua>
On Fri, May 28, 2021 at 1:08 PM Pavel Machek <pavel@ucw.cz> wrote: > > @@ -734,10 +736,15 @@ static int sso_led_dt_parse(struct sso_led_priv *priv) > > if (fw_ssoled) { > > ret = __sso_led_dt_parse(priv, fw_ssoled); > > if (ret) > > - return ret; > > + goto err_child_out; > > } > > > > + fwnode_handle_put(fw_ssoled); > > return 0; > > + > > +err_child_out: > > + fwnode_handle_put(fw_ssoled); > > + return ret; > > } > > Just delete the return and you get the same effect, no? No need to > have two exits here. Okay, I have tried and neither result is better: option 1. Add ret = 0, but keep the label option 2. Assign 0 to ret at the definition stage and replace return with break and remove return 0 (I don't like that ret assigned to 0 in the definition block. It usually may lead to subtle errors) option 3+. Something I missed which you see can be done? Which one do you prefer? -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 1:46 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, May 28, 2021 at 12:04:40PM +0200, Pavel Machek wrote: > > On Mon 2021-05-10 12:50:20, Andy Shevchenko wrote: > > > 1 microsecond with 20 millisecond parameter is too low margin for > > > usleep_range(). Give 100 to make scheduler happier. > > > > > > While at it, fix indentation in cases where EL_FW_DELAY_USEC is in use. > > > In the loop, move it to the end to avoid a conditional. > > > > Its not like unhappy schedulers are problem... > > Any hints then? To me it sounds like torturing scheduler is the real problem > and that's why scheduler is unhappy. Okay, I have rephrased the commit message. -- With Best Regards, Andy Shevchenko
On Sat, May 29, 2021 at 12:00 AM Oleh Kravchenko <oleg@kaa.org.ua> wrote: > 10.05.21 12:50, Andy Shevchenko пише: > > There is no user of of*.h headers, but mod_devicetable.h. > > Update header block accordingly. > > While at it, drop unneeded dependency to OF. > > depends on LEDS_CLASS > > depends on SPI > > - depends on OF > Reviewed-by: Oleh Kravchenko <oleg@kaa.org.ua> Thanks! I have dropped the OF dependency removal, while keeping your Rb. I think we may figure out later what is the best course of action regarding it. -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 1:50 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, May 28, 2021 at 12:09:06PM +0200, Pavel Machek wrote: > > On Mon 2021-05-10 12:50:28, Andy Shevchenko wrote: > > > The idea of managed resources that they will be cleaned up automatically > > > and in the proper order. Remove explicit cleanups. > > > > Are you really sure this is good idea with the regmap_update_bits in > > between? > > Good question. I will check it. > > > > - /* unregister led */ > > > - devm_led_classdev_unregister(priv->dev, &led->cdev); I left this untouched, i.o.w. no removal in v2. -- With Best Regards, Andy Shevchenko
On Fri, May 28, 2021 at 1:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, May 28, 2021 at 12:10:57PM +0200, Pavel Machek wrote: > > On Mon 2021-05-10 12:50:38, Andy Shevchenko wrote: > > > It's easy to miss necessary clean up, e.g. firmware node reference counting, > > > during error path in ->probe(). Make it more robust by moving to a single > > > point of return. > > > > > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > You are now putting the handle even in the success case. Is that > > right? > > Let's put it this way: it's no-op in successful case. > > But yeah, I would prefer to have a separate case for error, I'll revisit this. I have added return 0; for a successful case. -- With Best Regards, Andy Shevchenko
On Sat, May 29, 2021 at 12:28 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, May 28, 2021 at 1:08 PM Pavel Machek <pavel@ucw.cz> wrote: > > > @@ -734,10 +736,15 @@ static int sso_led_dt_parse(struct sso_led_priv *priv) > > > if (fw_ssoled) { > > > ret = __sso_led_dt_parse(priv, fw_ssoled); > > > if (ret) > > > - return ret; > > > + goto err_child_out; > > > } > > > > > > + fwnode_handle_put(fw_ssoled); > > > return 0; > > > + > > > +err_child_out: > > > + fwnode_handle_put(fw_ssoled); > > > + return ret; > > > } > > > > Just delete the return and you get the same effect, no? No need to > > have two exits here. > > Okay, I have tried and neither result is better: > option 1. Add ret = 0, but keep the label > option 2. Assign 0 to ret at the definition stage and replace return > with break and remove return 0 (I don't like that ret assigned to 0 in > the definition block. It usually may lead to subtle errors) > option 3+. Something I missed which you see can be done? > > Which one do you prefer? I found option 3 which is better and follows your suggestion (I suppose). -- With Best Regards, Andy Shevchenko