| Message ID | 20240830063228.3519385-3-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: ti: Simplify with scoped for each OF child loop and dev_err_probe() | expand |
On 14:32-20240830, Jinjie Ruan wrote: > Use the dev_err_probe() helper to simplify error handling > during probe. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > v2: > - Split into 2 patches. > --- > drivers/soc/ti/knav_dma.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c > index 15e41d3a5e22..eeec422a46f0 100644 > --- a/drivers/soc/ti/knav_dma.c > +++ b/drivers/soc/ti/knav_dma.c > @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) > struct device_node *node = pdev->dev.of_node; > int ret = 0; > > - if (!node) { > - dev_err(&pdev->dev, "could not find device info\n"); > - return -EINVAL; > - } > + if (!node) > + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); > > kdev = devm_kzalloc(dev, > sizeof(struct knav_dma_pool_device), GFP_KERNEL); > - if (!kdev) { > - dev_err(dev, "could not allocate driver mem\n"); > - return -ENOMEM; > - } > + if (!kdev) > + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); These make no sense to me :( -> just using dev_err_probe when there is no chance of -EPROBE_DEFER ? > > kdev->dev = dev; > INIT_LIST_HEAD(&kdev->list); > -- > 2.34.1 >
On 30/08/2024 12:31, Nishanth Menon wrote: > On 14:32-20240830, Jinjie Ruan wrote: >> Use the dev_err_probe() helper to simplify error handling >> during probe. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> v2: >> - Split into 2 patches. >> --- >> drivers/soc/ti/knav_dma.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c >> index 15e41d3a5e22..eeec422a46f0 100644 >> --- a/drivers/soc/ti/knav_dma.c >> +++ b/drivers/soc/ti/knav_dma.c >> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) >> struct device_node *node = pdev->dev.of_node; >> int ret = 0; >> >> - if (!node) { >> - dev_err(&pdev->dev, "could not find device info\n"); >> - return -EINVAL; >> - } >> + if (!node) >> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); >> >> kdev = devm_kzalloc(dev, >> sizeof(struct knav_dma_pool_device), GFP_KERNEL); >> - if (!kdev) { >> - dev_err(dev, "could not allocate driver mem\n"); >> - return -ENOMEM; >> - } >> + if (!kdev) >> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); > > These make no sense to me :( -> just using dev_err_probe when there is > no chance of -EPROBE_DEFER ? Well, this does not make sense from other point of view - memory allocation errors should have any printks... This patchset - like several others from Jinjie - is some sort of automation without careful consideration and without thinking whether code makes sense. Therefore I suggest to review it thoroughly and do not trust the "innocent" look of such changes. Best regards, Krzysztof
On 30/08/2024 12:36, Krzysztof Kozlowski wrote: > On 30/08/2024 12:31, Nishanth Menon wrote: >> On 14:32-20240830, Jinjie Ruan wrote: >>> Use the dev_err_probe() helper to simplify error handling >>> during probe. >>> >>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>> --- >>> v2: >>> - Split into 2 patches. >>> --- >>> drivers/soc/ti/knav_dma.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c >>> index 15e41d3a5e22..eeec422a46f0 100644 >>> --- a/drivers/soc/ti/knav_dma.c >>> +++ b/drivers/soc/ti/knav_dma.c >>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) >>> struct device_node *node = pdev->dev.of_node; >>> int ret = 0; >>> >>> - if (!node) { >>> - dev_err(&pdev->dev, "could not find device info\n"); >>> - return -EINVAL; >>> - } >>> + if (!node) >>> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); >>> >>> kdev = devm_kzalloc(dev, >>> sizeof(struct knav_dma_pool_device), GFP_KERNEL); >>> - if (!kdev) { >>> - dev_err(dev, "could not allocate driver mem\n"); >>> - return -ENOMEM; >>> - } >>> + if (!kdev) >>> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); >> >> These make no sense to me :( -> just using dev_err_probe when there is >> no chance of -EPROBE_DEFER ? > > Well, this does not make sense from other point of view - memory > allocation errors should have any printks... s/should/should not/ obviously :) > > This patchset - like several others from Jinjie - is some sort of > automation without careful consideration and without thinking whether > code makes sense. > > Therefore I suggest to review it thoroughly and do not trust the > "innocent" look of such changes. Best regards, Krzysztof
On 2024/8/30 18:31, Nishanth Menon wrote: > On 14:32-20240830, Jinjie Ruan wrote: >> Use the dev_err_probe() helper to simplify error handling >> during probe. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> v2: >> - Split into 2 patches. >> --- >> drivers/soc/ti/knav_dma.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c >> index 15e41d3a5e22..eeec422a46f0 100644 >> --- a/drivers/soc/ti/knav_dma.c >> +++ b/drivers/soc/ti/knav_dma.c >> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) >> struct device_node *node = pdev->dev.of_node; >> int ret = 0; >> >> - if (!node) { >> - dev_err(&pdev->dev, "could not find device info\n"); >> - return -EINVAL; >> - } >> + if (!node) >> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); >> >> kdev = devm_kzalloc(dev, >> sizeof(struct knav_dma_pool_device), GFP_KERNEL); >> - if (!kdev) { >> - dev_err(dev, "could not allocate driver mem\n"); >> - return -ENOMEM; >> - } >> + if (!kdev) >> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); > > These make no sense to me :( -> just using dev_err_probe when there is > no chance of -EPROBE_DEFER ? I noticed a change in dev_err_probe() this year, which is described in this patch: For an out-of-memory error there should be no additional output. Adapt dev_err_probe() to not emit the error message when err is -ENOMEM. This simplifies handling errors that might among others be -ENOMEM. And the comment of dev_err_probe() said below: * Using this helper in your probe function is totally fine even if @err ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ is ~~ * known to never be -EPROBE_DEFER. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ https://lore.kernel.org/all/3d1e308d45cddf67749522ca42d83f5b4f0b9634.1718311756.git.u.kleine-koenig@baylibre.com/ > >> >> kdev->dev = dev; >> INIT_LIST_HEAD(&kdev->list); >> -- >> 2.34.1 >> >
On 31/08/2024 03:59, Jinjie Ruan wrote: > > > On 2024/8/30 18:31, Nishanth Menon wrote: >> On 14:32-20240830, Jinjie Ruan wrote: >>> Use the dev_err_probe() helper to simplify error handling >>> during probe. >>> >>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>> --- >>> v2: >>> - Split into 2 patches. >>> --- >>> drivers/soc/ti/knav_dma.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c >>> index 15e41d3a5e22..eeec422a46f0 100644 >>> --- a/drivers/soc/ti/knav_dma.c >>> +++ b/drivers/soc/ti/knav_dma.c >>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) >>> struct device_node *node = pdev->dev.of_node; >>> int ret = 0; >>> >>> - if (!node) { >>> - dev_err(&pdev->dev, "could not find device info\n"); >>> - return -EINVAL; >>> - } >>> + if (!node) >>> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); >>> >>> kdev = devm_kzalloc(dev, >>> sizeof(struct knav_dma_pool_device), GFP_KERNEL); >>> - if (!kdev) { >>> - dev_err(dev, "could not allocate driver mem\n"); >>> - return -ENOMEM; >>> - } >>> + if (!kdev) >>> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); >> >> These make no sense to me :( -> just using dev_err_probe when there is >> no chance of -EPROBE_DEFER ? > > I noticed a change in dev_err_probe() this year, which is described in > this patch: > > For an out-of-memory error there should be no additional output. Adapt > dev_err_probe() to not emit the error message when err is -ENOMEM. > This simplifies handling errors that might among others be -ENOMEM. > > > And the comment of dev_err_probe() said below: > > * Using this helper in your probe function is totally fine even if @err Fine but not much useful and at the same time huge churn from @vivo.com. Best regards, Krzysztof
On 2024/8/31 13:21, Krzysztof Kozlowski wrote: > On 31/08/2024 03:59, Jinjie Ruan wrote: >> >> >> On 2024/8/30 18:31, Nishanth Menon wrote: >>> On 14:32-20240830, Jinjie Ruan wrote: >>>> Use the dev_err_probe() helper to simplify error handling >>>> during probe. >>>> >>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>>> --- >>>> v2: >>>> - Split into 2 patches. >>>> --- >>>> drivers/soc/ti/knav_dma.c | 12 ++++-------- >>>> 1 file changed, 4 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c >>>> index 15e41d3a5e22..eeec422a46f0 100644 >>>> --- a/drivers/soc/ti/knav_dma.c >>>> +++ b/drivers/soc/ti/knav_dma.c >>>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) >>>> struct device_node *node = pdev->dev.of_node; >>>> int ret = 0; >>>> >>>> - if (!node) { >>>> - dev_err(&pdev->dev, "could not find device info\n"); >>>> - return -EINVAL; >>>> - } >>>> + if (!node) >>>> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); >>>> >>>> kdev = devm_kzalloc(dev, >>>> sizeof(struct knav_dma_pool_device), GFP_KERNEL); >>>> - if (!kdev) { >>>> - dev_err(dev, "could not allocate driver mem\n"); >>>> - return -ENOMEM; >>>> - } >>>> + if (!kdev) >>>> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); >>> >>> These make no sense to me :( -> just using dev_err_probe when there is >>> no chance of -EPROBE_DEFER ? >> >> I noticed a change in dev_err_probe() this year, which is described in >> this patch: >> >> For an out-of-memory error there should be no additional output. Adapt >> dev_err_probe() to not emit the error message when err is -ENOMEM. >> This simplifies handling errors that might among others be -ENOMEM. >> >> >> And the comment of dev_err_probe() said below: >> >> * Using this helper in your probe function is totally fine even if @err > > Fine but not much useful and at the same time huge churn from @vivo.com. I see, it is fine with these dev_err_probe() dropped, the main is the scoped child iterate. > > Best regards, > Krzysztof >
> > Fine but not much useful and at the same time huge churn from @vivo.com. > > I see, it is fine with these dev_err_probe() dropped, the main is the > scoped child iterate. Which is also a lot of churn. How about you start with those that are actually broken, missing a put. Those changes are useful. Changing code which is already correct has a lot less value. Andrew
Hello, On Sat, Aug 31, 2024 at 09:59:33AM +0800, Jinjie Ruan wrote: > On 2024/8/30 18:31, Nishanth Menon wrote: > > On 14:32-20240830, Jinjie Ruan wrote: > >> Use the dev_err_probe() helper to simplify error handling > >> during probe. > >> > >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > >> --- > >> v2: > >> - Split into 2 patches. > >> --- > >> drivers/soc/ti/knav_dma.c | 12 ++++-------- > >> 1 file changed, 4 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c > >> index 15e41d3a5e22..eeec422a46f0 100644 > >> --- a/drivers/soc/ti/knav_dma.c > >> +++ b/drivers/soc/ti/knav_dma.c > >> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) > >> struct device_node *node = pdev->dev.of_node; > >> int ret = 0; > >> > >> - if (!node) { > >> - dev_err(&pdev->dev, "could not find device info\n"); > >> - return -EINVAL; > >> - } > >> + if (!node) > >> + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); > >> > >> kdev = devm_kzalloc(dev, > >> sizeof(struct knav_dma_pool_device), GFP_KERNEL); > >> - if (!kdev) { > >> - dev_err(dev, "could not allocate driver mem\n"); > >> - return -ENOMEM; > >> - } > >> + if (!kdev) > >> + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); > > > > These make no sense to me :( -> just using dev_err_probe when there is > > no chance of -EPROBE_DEFER ? > > I noticed a change in dev_err_probe() this year, which is described in > this patch: > > For an out-of-memory error there should be no additional output. Adapt > dev_err_probe() to not emit the error message when err is -ENOMEM. > This simplifies handling errors that might among others be -ENOMEM. Notice this was carefully worded. Calling dev_err_probe() if you know that the error is ENOMEM isn't helpful. The change was introduced to simplify ret = some_function_that_might_return_ENOMEM_and_other_errors() if (ret == -ENOMEM) return ret; else if (ret) return dev_err_probe(dev, ret, "...."); to ret = some_function_that_might_return_ENOMEM_and_other_errors() if (ret) return dev_err_probe(dev, ret, "...."); But adding a dev_err_probe() if you know in the ret != 0 branch that ret must be -ENOMEM, actively adding a dev_err_probe() gives very little improvement. The main effect is to increase the size of the resulting kernel image (or module). Back when I made dev_err_probe() silent for -ENOMEM, it was suggested to even make it fail to compile if ret is ENOMEM. I wouldn't necessarily object to new code using dev_err_probe() to check the return value of a function that can only return (success or) -ENOMEM, but I agree to Krzysztof that changing existing code is little helpful. Best regards Uwe
diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c index 15e41d3a5e22..eeec422a46f0 100644 --- a/drivers/soc/ti/knav_dma.c +++ b/drivers/soc/ti/knav_dma.c @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev) struct device_node *node = pdev->dev.of_node; int ret = 0; - if (!node) { - dev_err(&pdev->dev, "could not find device info\n"); - return -EINVAL; - } + if (!node) + return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n"); kdev = devm_kzalloc(dev, sizeof(struct knav_dma_pool_device), GFP_KERNEL); - if (!kdev) { - dev_err(dev, "could not allocate driver mem\n"); - return -ENOMEM; - } + if (!kdev) + return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n"); kdev->dev = dev; INIT_LIST_HEAD(&kdev->list);
Use the dev_err_probe() helper to simplify error handling during probe. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- v2: - Split into 2 patches. --- drivers/soc/ti/knav_dma.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)