* [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
@ 2020-11-18 10:37 ` Wang Hai
0 siblings, 0 replies; 4+ messages in thread
From: Wang Hai @ 2020-11-18 10:37 UTC (permalink / raw)
To: mchehab+huawei, gregkh, mayulong1; +Cc: devel, linux-kernel
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 4524ac56cdca ("staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 64b30d263c8d..4f34a5282970 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -262,8 +262,10 @@ static int hi6421_spmi_pmic_probe(struct spmi_device *pdev)
hi6421_spmi_pmic_irq_prc(pmic);
pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int), GFP_KERNEL);
- if (!pmic->irqs)
+ if (!pmic->irqs) {
+ ret = -ENOMEM;
goto irq_malloc;
+ }
pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
&hi6421_spmi_domain_ops, pmic);
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
@ 2020-11-18 10:37 ` Wang Hai
0 siblings, 0 replies; 4+ messages in thread
From: Wang Hai @ 2020-11-18 10:37 UTC (permalink / raw)
To: mchehab+huawei, gregkh, mayulong1; +Cc: devel, linux-kernel
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 4524ac56cdca ("staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 64b30d263c8d..4f34a5282970 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -262,8 +262,10 @@ static int hi6421_spmi_pmic_probe(struct spmi_device *pdev)
hi6421_spmi_pmic_irq_prc(pmic);
pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int), GFP_KERNEL);
- if (!pmic->irqs)
+ if (!pmic->irqs) {
+ ret = -ENOMEM;
goto irq_malloc;
+ }
pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
&hi6421_spmi_domain_ops, pmic);
--
2.17.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
2020-11-18 10:37 ` Wang Hai
@ 2020-11-18 11:12 ` Dan Carpenter
-1 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-11-18 11:12 UTC (permalink / raw)
To: Wang Hai; +Cc: mchehab+huawei, gregkh, mayulong1, devel, linux-kernel
Not necessarily related to your patch but it should just return -ENOMEM
instead of the "goto irq_malloc;".
drivers/staging/hikey9xx/hi6421-spmi-pmic.c
251 if (!gpio_is_valid(pmic->gpio))
252 return -EINVAL;
253
254 ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN, "pmic");
255 if (ret < 0) {
256 dev_err(dev, "failed to request gpio%d\n", pmic->gpio);
257 return ret;
This is a direct return.
258 }
259
260 pmic->irq = gpio_to_irq(pmic->gpio);
[ Edit. Actually I can see that the original author must have thought
that this needed to be released but it doesn't. ]
261
262 hi6421_spmi_pmic_irq_prc(pmic);
263
264 pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int), GFP_KERNEL);
265 if (!pmic->irqs) {
266 ret = -ENOMEM;
267 goto irq_malloc;
This is a goto with a ComeFrom style label name, which says where it
is called from (The goto is at the place where irq_malloc fails). This
is a useless label name because we can see from the line before that
the alloc failed. What we want to know is what the goto does!
268 }
269
270 pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
271 &hi6421_spmi_domain_ops, pmic);
272 if (!pmic->domain) {
273 dev_err(dev, "failed irq domain add simple!\n");
274 ret = -ENODEV;
275 goto irq_malloc;
Here the label name is even more useless here because "irq_malloc"
didn't fail on the line before. #Confusing But we still don't know
what the goto does.
If we scroll down then we see that "goto irq_malloc" releases the IRQ.
A better name would be "goto err_irq;"
276 }
277
278 for (i = 0; i < HISI_IRQ_NUM; i++) {
279 virq = irq_create_mapping(pmic->domain, i);
280 if (!virq) {
281 dev_err(dev, "Failed mapping hwirq\n");
282 ret = -ENOSPC;
283 goto irq_malloc;
284 }
285 pmic->irqs[i] = virq;
286 dev_dbg(dev, "%s: pmic->irqs[%d] = %d\n",
287 __func__, i, pmic->irqs[i]);
288 }
289
290 ret = request_threaded_irq(pmic->irq, hi6421_spmi_irq_handler, NULL,
291 IRQF_TRIGGER_LOW | IRQF_SHARED | IRQF_NO_SUSPEND,
292 "pmic", pmic);
Except it turns out that we don't actually request the IRQ until this
line. So those earlier "goto err_irq;" things are bogus.
293 if (ret < 0) {
294 dev_err(dev, "could not claim pmic IRQ: error %d\n", ret);
295 goto irq_malloc;
296 }
297
298 dev_set_drvdata(&pdev->dev, pmic);
299
300 /*
301 * The logic below will rely that the pmic is already stored at
302 * drvdata.
303 */
304 dev_dbg(&pdev->dev, "SPMI-PMIC: adding children for %pOF\n",
305 pdev->dev.of_node);
306 ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
307 hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs),
308 NULL, 0, NULL);
309 if (!ret)
310 return 0;
This is "success handling" anti-pattern and "last condition is weird"
anti-pattern. We should always do failure handling. The code should
look like:
success();
success();
success();
success();
if () {
failure();
failure();
failure();
}
success();
success();
if () {
failure();
failure();
failure();
}
Failure is indented twice and success once.
311
312 dev_err(dev, "Failed to add child devices: %d\n", ret);
313
314 irq_malloc:
315 free_irq(pmic->irq, pmic);
This free should only be done if devm_mfd_add_devices() fails. I don't
know what happens if you free an IRQ which has not been requested. I
think it triggers a WARN().
316
317 return ret;
318 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
@ 2020-11-18 11:12 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-11-18 11:12 UTC (permalink / raw)
To: Wang Hai; +Cc: mchehab+huawei, gregkh, devel, linux-kernel, mayulong1
Not necessarily related to your patch but it should just return -ENOMEM
instead of the "goto irq_malloc;".
drivers/staging/hikey9xx/hi6421-spmi-pmic.c
251 if (!gpio_is_valid(pmic->gpio))
252 return -EINVAL;
253
254 ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN, "pmic");
255 if (ret < 0) {
256 dev_err(dev, "failed to request gpio%d\n", pmic->gpio);
257 return ret;
This is a direct return.
258 }
259
260 pmic->irq = gpio_to_irq(pmic->gpio);
[ Edit. Actually I can see that the original author must have thought
that this needed to be released but it doesn't. ]
261
262 hi6421_spmi_pmic_irq_prc(pmic);
263
264 pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int), GFP_KERNEL);
265 if (!pmic->irqs) {
266 ret = -ENOMEM;
267 goto irq_malloc;
This is a goto with a ComeFrom style label name, which says where it
is called from (The goto is at the place where irq_malloc fails). This
is a useless label name because we can see from the line before that
the alloc failed. What we want to know is what the goto does!
268 }
269
270 pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
271 &hi6421_spmi_domain_ops, pmic);
272 if (!pmic->domain) {
273 dev_err(dev, "failed irq domain add simple!\n");
274 ret = -ENODEV;
275 goto irq_malloc;
Here the label name is even more useless here because "irq_malloc"
didn't fail on the line before. #Confusing But we still don't know
what the goto does.
If we scroll down then we see that "goto irq_malloc" releases the IRQ.
A better name would be "goto err_irq;"
276 }
277
278 for (i = 0; i < HISI_IRQ_NUM; i++) {
279 virq = irq_create_mapping(pmic->domain, i);
280 if (!virq) {
281 dev_err(dev, "Failed mapping hwirq\n");
282 ret = -ENOSPC;
283 goto irq_malloc;
284 }
285 pmic->irqs[i] = virq;
286 dev_dbg(dev, "%s: pmic->irqs[%d] = %d\n",
287 __func__, i, pmic->irqs[i]);
288 }
289
290 ret = request_threaded_irq(pmic->irq, hi6421_spmi_irq_handler, NULL,
291 IRQF_TRIGGER_LOW | IRQF_SHARED | IRQF_NO_SUSPEND,
292 "pmic", pmic);
Except it turns out that we don't actually request the IRQ until this
line. So those earlier "goto err_irq;" things are bogus.
293 if (ret < 0) {
294 dev_err(dev, "could not claim pmic IRQ: error %d\n", ret);
295 goto irq_malloc;
296 }
297
298 dev_set_drvdata(&pdev->dev, pmic);
299
300 /*
301 * The logic below will rely that the pmic is already stored at
302 * drvdata.
303 */
304 dev_dbg(&pdev->dev, "SPMI-PMIC: adding children for %pOF\n",
305 pdev->dev.of_node);
306 ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
307 hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs),
308 NULL, 0, NULL);
309 if (!ret)
310 return 0;
This is "success handling" anti-pattern and "last condition is weird"
anti-pattern. We should always do failure handling. The code should
look like:
success();
success();
success();
success();
if () {
failure();
failure();
failure();
}
success();
success();
if () {
failure();
failure();
failure();
}
Failure is indented twice and success once.
311
312 dev_err(dev, "Failed to add child devices: %d\n", ret);
313
314 irq_malloc:
315 free_irq(pmic->irq, pmic);
This free should only be done if devm_mfd_add_devices() fails. I don't
know what happens if you free an IRQ which has not been requested. I
think it triggers a WARN().
316
317 return ret;
318 }
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-18 11:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 10:37 [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe() Wang Hai
2020-11-18 10:37 ` Wang Hai
2020-11-18 11:12 ` Dan Carpenter
2020-11-18 11:12 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.