* Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
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 11:12 ` Dan Carpenter
0 siblings, 0 replies; 2+ 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] 2+ messages in thread