linux-kernel - Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201118111224.GC29398@kadam>
Date:   Wed, 18 Nov 2020 14:12:24 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Wang Hai <wanghai38@...wei.com>
Cc:     mchehab+huawei@...nel.org, gregkh@...uxfoundation.org,
        mayulong1@...wei.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in
 hi6421_spmi_pmic_probe()

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