| Message ID | bfd893701ac3d239fef856d2f589063983422100.1616574973.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Fixes to device-managed work-queue series | expand |
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.vaittinen@fi.rohmeurope.com> > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > --- > 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, &data->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 >
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.vaittinen@fi.rohmeurope.com> > > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > > --- > > 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, &data->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
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.vaittinen@fi.rohmeurope.com> >>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >>> --- >>> 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, &data->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 >
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.vaittinen@fi.rohmeurope.com> > > > > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > > > > --- > > > > 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, &data->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 > > > >
On Thu, Mar 25, 2021 at 04:52:12AM +0000, 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.vaittinen@fi.rohmeurope.com> > > > > > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > > > > > --- > > > > > 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, &data->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
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, &data->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
Add error print for probe failure when resource managed work-queue initialization fails. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/extcon/extcon-gpio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)