Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
On Thu, Mar 25, 2021 at 04:52:12AM +, Vaittinen, Matti wrote: > > On Thu, 2021-03-25 at 09:49 +0900, Chanwoo Choi wrote: > > On 3/24/21 6:51 PM, Vaittinen, Matti wrote: > > > Hello Hans, Chanwoo, Greg, > > > > > > On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 3/24/21 10:21 AM, Matti Vaittinen wrote: > > > > > Add error print for probe failure when resource managed work- > > > > > queue > > > > > initialization fails. > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > matti.vaitti...@fi.rohmeurope.com> > > > > > Suggested-by: Chanwoo Choi > > > > > --- > > > > > drivers/extcon/extcon-gpio.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/extcon/extcon-gpio.c > > > > > b/drivers/extcon/extcon- > > > > > gpio.c > > > > > index 4105df74f2b0..8ea2cda8f7f3 100644 > > > > > --- a/drivers/extcon/extcon-gpio.c > > > > > +++ b/drivers/extcon/extcon-gpio.c > > > > > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct > > > > > platform_device *pdev) > > > > > return ret; > > > > > > > > > > ret = devm_delayed_work_autocancel(dev, >work, > > > > > gpio_extcon_work); > > > > > - if (ret) > > > > > + if (ret) { > > > > > + dev_err(dev, "Failed to initialize > > > > > delayed_work"); > > > > > return ret; > > > > > + } > > > > > > > > The only ret which we can have here is -ENOMEM and as a rule we > > > > don't > > > > log > > > > errors for those, because the kernel memory-management code > > > > already > > > > complains > > > > loudly when this happens. > > > > > > I know. This is why I originally omitted the print. Besides, if the > > > memory is so low that devres adding fails - then we probably have > > > plenty of other complaints as well... But as Chanwoo maintains the > > > driver and wanted to have the print - I do not have objections to > > > that > > > either. Maybe someone some-day adds another error path to wq > > > initialization in which case seeing it failed could make sense. > > > > > > > So IMHO this patch should be dropped. > > > Fine for me - as well as keeping it. I have no strong opinion on > > > this. > > > > If it is the same handling way for -ENOMEM, don't need to add log ss > > Hans said. > > Thanks for Hans. > > So be it :) > Greg, can you just apply the patch 2/2 and drop this one? (There should > be no dependency between these) or do you want me to resend 2/2 alone? I'll just take the 2/2 patch, thanks. greg k-h
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
On Thu, 2021-03-25 at 09:49 +0900, Chanwoo Choi wrote: > On 3/24/21 6:51 PM, Vaittinen, Matti wrote: > > Hello Hans, Chanwoo, Greg, > > > > On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 3/24/21 10:21 AM, Matti Vaittinen wrote: > > > > Add error print for probe failure when resource managed work- > > > > queue > > > > initialization fails. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaitti...@fi.rohmeurope.com> > > > > Suggested-by: Chanwoo Choi > > > > --- > > > > drivers/extcon/extcon-gpio.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/extcon/extcon-gpio.c > > > > b/drivers/extcon/extcon- > > > > gpio.c > > > > index 4105df74f2b0..8ea2cda8f7f3 100644 > > > > --- a/drivers/extcon/extcon-gpio.c > > > > +++ b/drivers/extcon/extcon-gpio.c > > > > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct > > > > platform_device *pdev) > > > > return ret; > > > > > > > > ret = devm_delayed_work_autocancel(dev, >work, > > > > gpio_extcon_work); > > > > - if (ret) > > > > + if (ret) { > > > > + dev_err(dev, "Failed to initialize > > > > delayed_work"); > > > > return ret; > > > > + } > > > > > > The only ret which we can have here is -ENOMEM and as a rule we > > > don't > > > log > > > errors for those, because the kernel memory-management code > > > already > > > complains > > > loudly when this happens. > > > > I know. This is why I originally omitted the print. Besides, if the > > memory is so low that devres adding fails - then we probably have > > plenty of other complaints as well... But as Chanwoo maintains the > > driver and wanted to have the print - I do not have objections to > > that > > either. Maybe someone some-day adds another error path to wq > > initialization in which case seeing it failed could make sense. > > > > > So IMHO this patch should be dropped. > > Fine for me - as well as keeping it. I have no strong opinion on > > this. > > If it is the same handling way for -ENOMEM, don't need to add log ss > Hans said. > Thanks for Hans. So be it :) Greg, can you just apply the patch 2/2 and drop this one? (There should be no dependency between these) or do you want me to resend 2/2 alone? > > Br, > > Matti > > > >
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
On 3/24/21 6:51 PM, Vaittinen, Matti wrote: > Hello Hans, Chanwoo, Greg, > > On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote: >> Hi, >> >> On 3/24/21 10:21 AM, Matti Vaittinen wrote: >>> Add error print for probe failure when resource managed work-queue >>> initialization fails. >>> >>> Signed-off-by: Matti Vaittinen >>> Suggested-by: Chanwoo Choi >>> --- >>> drivers/extcon/extcon-gpio.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon- >>> gpio.c >>> index 4105df74f2b0..8ea2cda8f7f3 100644 >>> --- a/drivers/extcon/extcon-gpio.c >>> +++ b/drivers/extcon/extcon-gpio.c >>> @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct >>> platform_device *pdev) >>> return ret; >>> >>> ret = devm_delayed_work_autocancel(dev, >work, >>> gpio_extcon_work); >>> - if (ret) >>> + if (ret) { >>> + dev_err(dev, "Failed to initialize delayed_work"); >>> return ret; >>> + } >> >> The only ret which we can have here is -ENOMEM and as a rule we don't >> log >> errors for those, because the kernel memory-management code already >> complains >> loudly when this happens. > > I know. This is why I originally omitted the print. Besides, if the > memory is so low that devres adding fails - then we probably have > plenty of other complaints as well... But as Chanwoo maintains the > driver and wanted to have the print - I do not have objections to that > either. Maybe someone some-day adds another error path to wq > initialization in which case seeing it failed could make sense. > >> So IMHO this patch should be dropped. > Fine for me - as well as keeping it. I have no strong opinion on this. If it is the same handling way for -ENOMEM, don't need to add log ss Hans said. Thanks for Hans. > > Br, > Matti > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
Hello Hans, Chanwoo, Greg, On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote: > Hi, > > On 3/24/21 10:21 AM, Matti Vaittinen wrote: > > Add error print for probe failure when resource managed work-queue > > initialization fails. > > > > Signed-off-by: Matti Vaittinen > > Suggested-by: Chanwoo Choi > > --- > > drivers/extcon/extcon-gpio.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon- > > gpio.c > > index 4105df74f2b0..8ea2cda8f7f3 100644 > > --- a/drivers/extcon/extcon-gpio.c > > +++ b/drivers/extcon/extcon-gpio.c > > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct > > platform_device *pdev) > > return ret; > > > > ret = devm_delayed_work_autocancel(dev, >work, > > gpio_extcon_work); > > - if (ret) > > + if (ret) { > > + dev_err(dev, "Failed to initialize delayed_work"); > > return ret; > > + } > > The only ret which we can have here is -ENOMEM and as a rule we don't > log > errors for those, because the kernel memory-management code already > complains > loudly when this happens. I know. This is why I originally omitted the print. Besides, if the memory is so low that devres adding fails - then we probably have plenty of other complaints as well... But as Chanwoo maintains the driver and wanted to have the print - I do not have objections to that either. Maybe someone some-day adds another error path to wq initialization in which case seeing it failed could make sense. > So IMHO this patch should be dropped. Fine for me - as well as keeping it. I have no strong opinion on this. Br, Matti
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
Hi, On 3/24/21 10:21 AM, Matti Vaittinen wrote: > Add error print for probe failure when resource managed work-queue > initialization fails. > > Signed-off-by: Matti Vaittinen > Suggested-by: Chanwoo Choi > --- > drivers/extcon/extcon-gpio.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c > index 4105df74f2b0..8ea2cda8f7f3 100644 > --- a/drivers/extcon/extcon-gpio.c > +++ b/drivers/extcon/extcon-gpio.c > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct platform_device > *pdev) > return ret; > > ret = devm_delayed_work_autocancel(dev, >work, gpio_extcon_work); > - if (ret) > + if (ret) { > + dev_err(dev, "Failed to initialize delayed_work"); > return ret; > + } The only ret which we can have here is -ENOMEM and as a rule we don't log errors for those, because the kernel memory-management code already complains loudly when this happens. So IMHO this patch should be dropped. Regards, Hans > > /* >* Request the interrupt of gpio to detect whether external connector >
[PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
Add error print for probe failure when resource managed work-queue initialization fails. Signed-off-by: Matti Vaittinen Suggested-by: Chanwoo Choi --- drivers/extcon/extcon-gpio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c index 4105df74f2b0..8ea2cda8f7f3 100644 --- a/drivers/extcon/extcon-gpio.c +++ b/drivers/extcon/extcon-gpio.c @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct platform_device *pdev) return ret; ret = devm_delayed_work_autocancel(dev, >work, gpio_extcon_work); - if (ret) + if (ret) { + dev_err(dev, "Failed to initialize delayed_work"); return ret; + } /* * Request the interrupt of gpio to detect whether external connector -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]