linux-kernel - Re: [PATCH v4 1/3] driver core: add probe

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9f7cb75-62a0-20f6-0e5b-1553852d81b2@samsung.com>
Date:   Thu, 20 Dec 2018 12:37:58 +0100
From:   Andrzej Hajda <a.hajda@...sung.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org,
        Javier Martinez Canillas <javierm@...hat.com>,
        linux-arm-kernel@...ts.infradead.org, andy.shevchenko@...il.com,
        Mark Brown <broonie@...nel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH v4 1/3] driver core: add probe_err log helper

On 20.12.2018 12:14, Greg Kroah-Hartman wrote:
> On Thu, Dec 20, 2018 at 11:22:45AM +0100, Andrzej Hajda wrote:
>> During probe every time driver gets resource it should usually check for error
>> printk some message if it is not -EPROBE_DEFER and return the error. This
>> pattern is simple but requires adding few lines after any resource acquisition
>> code, as a result it is often omited or implemented only partially.
>> probe_err helps to replace such code sequences with simple call, so code:
>> 	if (err != -EPROBE_DEFER)
>> 		dev_err(dev, ...);
>> 	return err;
>> becomes:
>> 	return probe_err(dev, err, ...);
> Can you show a driver being converted to use this to show if it really
> will save a bunch of lines and make things simpler?  Usually you are
> requesting lots of resources so you need to do more than just return,
> you need to clean stuff up first.


I have posted sample conversion patch (generated by cocci) in previous
version of this patchset [1].

I did not re-posted it again as it is quite big patch and it will not be
applied without prior splitting it per subsystem.

Regarding stuff cleaning: devm_* usually makes it unnecessary, but also
even with necessary cleaning you can profit from probe_err, you just
calls it without leaving probe - you have still handled correctly probe
deferring.

Here is sample usage (taken from beginning of the mentioned patch):

---
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 4b900fc659f7..52e891fe1586 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -581,11 +581,8 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	int i, irq, n_ports, rc;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		if (irq != -EPROBE_DEFER)
-			dev_err(dev, "no irq\n");
-		return irq;
-	}
+	if (irq <= 0)
+		return probe_err(dev, irq, "no irq\n");

---

And there is plenty of such places, with new version of cocci script I can locate about 2700 such cases, and it is still far from completeness.

[1]:
https://lore.kernel.org/lkml/20181016072244.1216-4-a.hajda@samsung.com/T/#u


Regards

Andrzej