Skip to contentSkip to conversation

PNPACPI: check return value of pnp_add_device()

PATCH In review Dmitry Torokhov 2 participants 3 messages Nov 4, 2010 – Nov 8, 2010

Subsystems (2)
ACPI Pnp support
lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

PATCH 1/1 PNPACPI: check return value of pnp_add_device()

pnp_add_device() may fail so we need to handle errors and avoid leaking memory. Also, do not use ACPI-specific return codes (AE_OK) but rather standard ones.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
drivers/pnp/pnpacpi/core.c +10 -3
index 2d73dfc..06ee06c 100644
@@ -194,8 +194,9 @@ static char *pnpacpi_get_id(struct acpi_device *device)
 
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
-	acpi_handle temp = NULL;
+	acpi_handle temp;
 	acpi_status status;
+	int error;
 	struct pnp_dev *dev;
 	char *pnpid;
 	struct acpi_hardware_id *id;

@@ -256,10 +257,16 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	/* clear out the damaged flags */
 	if (!dev->active)
 		pnp_init_resources(dev);
-	pnp_add_device(dev);
+
+	error = pnp_add_device(dev);
+	if (error) {
+		put_device(&dev->dev);
+		return error;
+	}
+
 	num++;
 
-	return AE_OK;
+	return 0;
 }
 
 static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
Signature · 1 line
Dmitry
#1 lore raw
Quoted 3 lines · Dmitry Torokhov

pnp_add_device() may fail so we need to handle errors and avoid leaking memory. Also, do not use ACPI-specific return codes (AE_OK) but rather standard ones.

I think this is generally a good thing. You probably found this by inspection, but if there's a bug report, please include the URL.

Would you consider fixing the similar leaks and failure path issues in isapnp and pnpbios at the same time?

Quoted 28 lines
Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
drivers/pnp/pnpacpi/core.c +3 -2
index 2d73dfc..06ee06c 100644
@@ -194,8 +194,9 @@ static char *pnpacpi_get_id(struct acpi_device *device)
 
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
-	acpi_handle temp = NULL;
+	acpi_handle temp;
 	acpi_status status;
+	int error;
 	struct pnp_dev *dev;
 	char *pnpid;
 	struct acpi_hardware_id *id;

@@ -256,10 +257,16 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	/* clear out the damaged flags */
 	if (!dev->active)
 		pnp_init_resources(dev);
-	pnp_add_device(dev);
+

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device *device) · line 269

Quoted 10 lines
drivers/pnp/pnpacpi/core.c +7 -1
+	error = pnp_add_device(dev);
+	if (error) {
+		put_device(&dev->dev);
+		return error;
+	}
+
 	num++;
 
-	return AE_OK;
+	return 0;

The acpi_get_devices() usage of pnpacpi_add_device() expects an acpi_status return value, so maybe we should leave this same and change the pnpacpi_add_device() return type from
"int" to "acpi_status"?

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device *device) · line 272

Quoted 3 lines
drivers/pnp/pnpacpi/core.c +0 -0
 }
 
 static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,

Bjorn

author
#2 lore raw
Quoted 7 lines · Bjorn Helgaas
Quoted 3 lines · Dmitry Torokhov

pnp_add_device() may fail so we need to handle errors and avoid leaking memory. Also, do not use ACPI-specific return codes (AE_OK) but rather standard ones.

I think this is generally a good thing. You probably found this by inspection, but if there's a bug report, please include the URL.

I am not aware of an existing bug report,I just happened to look into the code in question...

Quoted 3 lines

Would you consider fixing the similar leaks and failure path issues in isapnp and pnpbios at the same time?

-ENOTIME at the moment ;( The only reason I happened to look into PNP code is that issue with HID strings exceeding 8 characters.

Quoted 37 lines
Quoted 37 lines
Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
drivers/pnp/pnpacpi/core.c +9 -2
index 2d73dfc..06ee06c 100644
@@ -194,8 +194,9 @@ static char *pnpacpi_get_id(struct acpi_device *device)
 
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
-	acpi_handle temp = NULL;
+	acpi_handle temp;
 	acpi_status status;
+	int error;
 	struct pnp_dev *dev;
 	char *pnpid;
 	struct acpi_hardware_id *id;

@@ -256,10 +257,16 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	/* clear out the damaged flags */
 	if (!dev->active)
 		pnp_init_resources(dev);
-	pnp_add_device(dev);
+
+	error = pnp_add_device(dev);
+	if (error) {
+		put_device(&dev->dev);
+		return error;
+	}
+
 	num++;
Quoted 7 lines

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device *device) · line 269

Quoted 2 lines
drivers/pnp/pnpacpi/core.c +1 -1
-	return AE_OK;
+	return 0;

The acpi_get_devices() usage of pnpacpi_add_device() expects an acpi_status return value, so maybe we should leave this same and change the pnpacpi_add_device() return type from
"int" to "acpi_status"?

PNPACPI calls acpi_get_devices() with pnpacpi_add_device_handler() which returns proper type (acpi_status).

Thanks.

Signature · 1 line
Dmitry