* [RFC][PATCH 0/2] PNP: Avoid potential error code path problems
@ 2015-03-12 1:20 Rafael J. Wysocki
2015-03-12 1:21 ` [RFC][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-12 1:20 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
Hi,
These two patches fix potential issues related to failing device registration
in the PNP subsystem.
Comments welcome!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 1/2] PNP: Convert pnp_lock into a mutex
2015-03-12 1:20 [RFC][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
@ 2015-03-12 1:21 ` Rafael J. Wysocki
2015-03-12 1:22 ` [RFC][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
2015-03-13 0:42 ` [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-12 1:21 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
pnp_lock is a spinlock, but it is only acquired from process context,
so it may be a mutex just fine and that will allow a couple of problems
to be avoided going forward.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/pnp/base.h | 2 +-
drivers/pnp/card.c | 25 +++++++++++++------------
drivers/pnp/core.c | 19 ++++++++++---------
3 files changed, 24 insertions(+), 22 deletions(-)
Index: linux-pm/drivers/pnp/core.c
===================================================================
--- linux-pm.orig/drivers/pnp/core.c
+++ linux-pm/drivers/pnp/core.c
@@ -9,6 +9,7 @@
#include <linux/list.h>
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/string.h>
#include <linux/slab.h>
@@ -19,7 +20,7 @@
static LIST_HEAD(pnp_protocols);
LIST_HEAD(pnp_global);
-DEFINE_SPINLOCK(pnp_lock);
+DEFINE_MUTEX(pnp_lock);
/*
* ACPI or PNPBIOS should tell us about all platform devices, so we can
@@ -55,7 +56,7 @@ int pnp_register_protocol(struct pnp_pro
INIT_LIST_HEAD(&protocol->devices);
INIT_LIST_HEAD(&protocol->cards);
nodenum = 0;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
/* assign the lowest unused number */
list_for_each(pos, &pnp_protocols) {
@@ -67,7 +68,7 @@ int pnp_register_protocol(struct pnp_pro
}
list_add_tail(&protocol->protocol_list, &pnp_protocols);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
protocol->number = nodenum;
dev_set_name(&protocol->dev, "pnp%d", nodenum);
@@ -80,9 +81,9 @@ int pnp_register_protocol(struct pnp_pro
*/
void pnp_unregister_protocol(struct pnp_protocol *protocol)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&protocol->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
device_unregister(&protocol->dev);
}
@@ -161,10 +162,10 @@ int __pnp_add_device(struct pnp_dev *dev
{
pnp_fixup_device(dev);
dev->status = PNP_READY;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&dev->global_list, &pnp_global);
list_add_tail(&dev->protocol_list, &dev->protocol->devices);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
if (dev->protocol->can_wakeup)
device_set_wakeup_capable(&dev->dev,
dev->protocol->can_wakeup(dev));
@@ -203,10 +204,10 @@ int pnp_add_device(struct pnp_dev *dev)
void __pnp_remove_device(struct pnp_dev *dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&dev->global_list);
list_del(&dev->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
device_unregister(&dev->dev);
}
Index: linux-pm/drivers/pnp/base.h
===================================================================
--- linux-pm.orig/drivers/pnp/base.h
+++ linux-pm/drivers/pnp/base.h
@@ -3,7 +3,7 @@
* Bjorn Helgaas <bjorn.helgaas@hp.com>
*/
-extern spinlock_t pnp_lock;
+extern struct mutex pnp_lock;
extern const struct attribute_group *pnp_dev_groups[];
void *pnp_alloc(long size);
Index: linux-pm/drivers/pnp/card.c
===================================================================
--- linux-pm.orig/drivers/pnp/card.c
+++ linux-pm/drivers/pnp/card.c
@@ -5,6 +5,7 @@
*/
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/ctype.h>
#include <linux/slab.h>
#include <linux/pnp.h>
@@ -244,10 +245,10 @@ int pnp_add_card(struct pnp_card *card)
}
pnp_interface_attach_card(card);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&card->global_list, &pnp_cards);
list_add_tail(&card->protocol_list, &card->protocol->cards);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
/* we wait until now to add devices in order to ensure the drivers
* will be able to use all of the related devices on the card
@@ -276,10 +277,10 @@ void pnp_remove_card(struct pnp_card *ca
struct list_head *pos, *temp;
device_unregister(&card->dev);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&card->global_list);
list_del(&card->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
list_for_each_safe(pos, temp, &card->devices) {
struct pnp_dev *dev = card_to_pnp_dev(pos);
pnp_remove_card_device(dev);
@@ -297,10 +298,10 @@ int pnp_add_card_device(struct pnp_card
dev->card_link = NULL;
dev_set_name(&dev->dev, "%02x:%02x.%02x",
dev->protocol->number, card->number, dev->number);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
dev->card = card;
list_add_tail(&dev->card_list, &card->devices);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
return 0;
}
@@ -310,10 +311,10 @@ int pnp_add_card_device(struct pnp_card
*/
void pnp_remove_card_device(struct pnp_dev *dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
dev->card = NULL;
list_del(&dev->card_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
__pnp_remove_device(dev);
}
@@ -426,9 +427,9 @@ int pnp_register_card_driver(struct pnp_
if (error < 0)
return error;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&drv->global_list, &pnp_card_drivers);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
list_for_each_safe(pos, temp, &pnp_cards) {
struct pnp_card *card =
@@ -444,9 +445,9 @@ int pnp_register_card_driver(struct pnp_
*/
void pnp_unregister_card_driver(struct pnp_card_driver *drv)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&drv->global_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
pnp_unregister_driver(&drv->link);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists
2015-03-12 1:20 [RFC][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2015-03-12 1:21 ` [RFC][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
@ 2015-03-12 1:22 ` Rafael J. Wysocki
2015-03-13 0:42 ` [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-12 1:22 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
pnp_register_protocol() and __pnp_add_device() both have the problem
that if device_register() fails, the objects they create will be left
in the lists they have been put on beforehand. Unfortunately, that
is not handled by the callers of those routines either, so in case
of a device registration errors the PNP bus type's data structures
will end up in an inconsistent state.
Make pnp_register_protocol() and __pnp_add_device() remove the
objects from the lists if device registration fails.
In addition to that, since pnp_lock is a mutex now, device registration
can be put under it too to avoid the gap in which the device is in the
PNP bus type's lists, but has not been registered yet (or the other
way around during removal).
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/pnp/core.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 11 deletions(-)
Index: linux-pm/drivers/pnp/core.c
===================================================================
--- linux-pm.orig/drivers/pnp/core.c
+++ linux-pm/drivers/pnp/core.c
@@ -42,6 +42,11 @@ void *pnp_alloc(long size)
return result;
}
+static void pnp_remove_protocol(struct pnp_protocol *protocol)
+{
+ list_del(&protocol->protocol_list);
+}
+
/**
* pnp_protocol_register - adds a pnp protocol to the pnp layer
* @protocol: pointer to the corresponding pnp_protocol structure
@@ -50,12 +55,13 @@ void *pnp_alloc(long size)
*/
int pnp_register_protocol(struct pnp_protocol *protocol)
{
- int nodenum;
struct list_head *pos;
+ int nodenum, ret = 0;
INIT_LIST_HEAD(&protocol->devices);
INIT_LIST_HEAD(&protocol->cards);
nodenum = 0;
+
mutex_lock(&pnp_lock);
/* assign the lowest unused number */
@@ -67,12 +73,17 @@ int pnp_register_protocol(struct pnp_pro
}
}
- list_add_tail(&protocol->protocol_list, &pnp_protocols);
- mutex_unlock(&pnp_lock);
-
protocol->number = nodenum;
dev_set_name(&protocol->dev, "pnp%d", nodenum);
- return device_register(&protocol->dev);
+
+ list_add_tail(&protocol->protocol_list, &pnp_protocols);
+
+ ret = device_register(&protocol->dev);
+ if (ret)
+ pnp_remove_protocol(protocol);
+
+ mutex_unlock(&pnp_lock);
+ return ret;
}
/**
@@ -82,9 +93,9 @@ int pnp_register_protocol(struct pnp_pro
void pnp_unregister_protocol(struct pnp_protocol *protocol)
{
mutex_lock(&pnp_lock);
- list_del(&protocol->protocol_list);
- mutex_unlock(&pnp_lock);
+ pnp_remove_protocol(protocol);
device_unregister(&protocol->dev);
+ mutex_unlock(&pnp_lock);
}
static void pnp_free_ids(struct pnp_dev *dev)
@@ -158,18 +169,38 @@ struct pnp_dev *pnp_alloc_dev(struct pnp
return dev;
}
+static void pnp_delist_device(struct pnp_dev *dev)
+{
+ list_del(&dev->global_list);
+ list_del(&dev->protocol_list);
+}
+
int __pnp_add_device(struct pnp_dev *dev)
{
+ int ret;
+
pnp_fixup_device(dev);
dev->status = PNP_READY;
+
mutex_lock(&pnp_lock);
+
list_add_tail(&dev->global_list, &pnp_global);
list_add_tail(&dev->protocol_list, &dev->protocol->devices);
+
+ ret = device_register(&dev->dev);
+ if (ret) {
+ pnp_delist_device(dev);
+ mutex_unlock(&pnp_lock);
+ return ret;
+ }
+
mutex_unlock(&pnp_lock);
+
if (dev->protocol->can_wakeup)
device_set_wakeup_capable(&dev->dev,
dev->protocol->can_wakeup(dev));
- return device_register(&dev->dev);
+
+ return 0;
}
/*
@@ -205,10 +236,9 @@ int pnp_add_device(struct pnp_dev *dev)
void __pnp_remove_device(struct pnp_dev *dev)
{
mutex_lock(&pnp_lock);
- list_del(&dev->global_list);
- list_del(&dev->protocol_list);
- mutex_unlock(&pnp_lock);
+ pnp_delist_device(dev);
device_unregister(&dev->dev);
+ mutex_unlock(&pnp_lock);
}
static int __init pnp_init(void)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems
2015-03-12 1:20 [RFC][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2015-03-12 1:21 ` [RFC][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
2015-03-12 1:22 ` [RFC][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
@ 2015-03-13 0:42 ` Rafael J. Wysocki
2015-03-13 0:43 ` [RFC v2][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
2015-03-13 0:44 ` [RFC v2][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
2 siblings, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-13 0:42 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
On Thursday, March 12, 2015 02:20:07 AM Rafael J. Wysocki wrote:
> Hi,
>
> These two patches fix potential issues related to failing device registration
> in the PNP subsystem.
I overlooked the fact that the registration of a PNP device cannot be put
under pnp_lock (as it will deadlock), so new version follows. Patch [1/2]
is the same as before, but with a shorter changelog, patch [2/2] has been
reworked.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC v2][PATCH 1/2] PNP: Convert pnp_lock into a mutex
2015-03-13 0:42 ` [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
@ 2015-03-13 0:43 ` Rafael J. Wysocki
2015-03-13 0:44 ` [RFC v2][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-13 0:43 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
pnp_lock is a spinlock, but it is only acquired from process context,
so it may be a mutex just fine.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/pnp/base.h | 2 +-
drivers/pnp/card.c | 25 +++++++++++++------------
drivers/pnp/core.c | 19 ++++++++++---------
drivers/pnp/driver.c | 10 +++++-----
4 files changed, 29 insertions(+), 27 deletions(-)
Index: linux-pm/drivers/pnp/core.c
===================================================================
--- linux-pm.orig/drivers/pnp/core.c
+++ linux-pm/drivers/pnp/core.c
@@ -9,6 +9,7 @@
#include <linux/list.h>
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/string.h>
#include <linux/slab.h>
@@ -19,7 +20,7 @@
static LIST_HEAD(pnp_protocols);
LIST_HEAD(pnp_global);
-DEFINE_SPINLOCK(pnp_lock);
+DEFINE_MUTEX(pnp_lock);
/*
* ACPI or PNPBIOS should tell us about all platform devices, so we can
@@ -55,7 +56,7 @@ int pnp_register_protocol(struct pnp_pro
INIT_LIST_HEAD(&protocol->devices);
INIT_LIST_HEAD(&protocol->cards);
nodenum = 0;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
/* assign the lowest unused number */
list_for_each(pos, &pnp_protocols) {
@@ -67,7 +68,7 @@ int pnp_register_protocol(struct pnp_pro
}
list_add_tail(&protocol->protocol_list, &pnp_protocols);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
protocol->number = nodenum;
dev_set_name(&protocol->dev, "pnp%d", nodenum);
@@ -80,9 +81,9 @@ int pnp_register_protocol(struct pnp_pro
*/
void pnp_unregister_protocol(struct pnp_protocol *protocol)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&protocol->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
device_unregister(&protocol->dev);
}
@@ -161,10 +162,10 @@ int __pnp_add_device(struct pnp_dev *dev
{
pnp_fixup_device(dev);
dev->status = PNP_READY;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&dev->global_list, &pnp_global);
list_add_tail(&dev->protocol_list, &dev->protocol->devices);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
if (dev->protocol->can_wakeup)
device_set_wakeup_capable(&dev->dev,
dev->protocol->can_wakeup(dev));
@@ -203,10 +204,10 @@ int pnp_add_device(struct pnp_dev *dev)
void __pnp_remove_device(struct pnp_dev *dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&dev->global_list);
list_del(&dev->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
device_unregister(&dev->dev);
}
Index: linux-pm/drivers/pnp/base.h
===================================================================
--- linux-pm.orig/drivers/pnp/base.h
+++ linux-pm/drivers/pnp/base.h
@@ -3,7 +3,7 @@
* Bjorn Helgaas <bjorn.helgaas@hp.com>
*/
-extern spinlock_t pnp_lock;
+extern struct mutex pnp_lock;
extern const struct attribute_group *pnp_dev_groups[];
void *pnp_alloc(long size);
Index: linux-pm/drivers/pnp/card.c
===================================================================
--- linux-pm.orig/drivers/pnp/card.c
+++ linux-pm/drivers/pnp/card.c
@@ -5,6 +5,7 @@
*/
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/ctype.h>
#include <linux/slab.h>
#include <linux/pnp.h>
@@ -244,10 +245,10 @@ int pnp_add_card(struct pnp_card *card)
}
pnp_interface_attach_card(card);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&card->global_list, &pnp_cards);
list_add_tail(&card->protocol_list, &card->protocol->cards);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
/* we wait until now to add devices in order to ensure the drivers
* will be able to use all of the related devices on the card
@@ -276,10 +277,10 @@ void pnp_remove_card(struct pnp_card *ca
struct list_head *pos, *temp;
device_unregister(&card->dev);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&card->global_list);
list_del(&card->protocol_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
list_for_each_safe(pos, temp, &card->devices) {
struct pnp_dev *dev = card_to_pnp_dev(pos);
pnp_remove_card_device(dev);
@@ -297,10 +298,10 @@ int pnp_add_card_device(struct pnp_card
dev->card_link = NULL;
dev_set_name(&dev->dev, "%02x:%02x.%02x",
dev->protocol->number, card->number, dev->number);
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
dev->card = card;
list_add_tail(&dev->card_list, &card->devices);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
return 0;
}
@@ -310,10 +311,10 @@ int pnp_add_card_device(struct pnp_card
*/
void pnp_remove_card_device(struct pnp_dev *dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
dev->card = NULL;
list_del(&dev->card_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
__pnp_remove_device(dev);
}
@@ -426,9 +427,9 @@ int pnp_register_card_driver(struct pnp_
if (error < 0)
return error;
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_add_tail(&drv->global_list, &pnp_card_drivers);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
list_for_each_safe(pos, temp, &pnp_cards) {
struct pnp_card *card =
@@ -444,9 +445,9 @@ int pnp_register_card_driver(struct pnp_
*/
void pnp_unregister_card_driver(struct pnp_card_driver *drv)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
list_del(&drv->global_list);
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
pnp_unregister_driver(&drv->link);
}
Index: linux-pm/drivers/pnp/driver.c
===================================================================
--- linux-pm.orig/drivers/pnp/driver.c
+++ linux-pm/drivers/pnp/driver.c
@@ -58,22 +58,22 @@ static const struct pnp_device_id *match
int pnp_device_attach(struct pnp_dev *pnp_dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
if (pnp_dev->status != PNP_READY) {
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
return -EBUSY;
}
pnp_dev->status = PNP_ATTACHED;
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
return 0;
}
void pnp_device_detach(struct pnp_dev *pnp_dev)
{
- spin_lock(&pnp_lock);
+ mutex_lock(&pnp_lock);
if (pnp_dev->status == PNP_ATTACHED)
pnp_dev->status = PNP_READY;
- spin_unlock(&pnp_lock);
+ mutex_unlock(&pnp_lock);
pnp_disable_dev(pnp_dev);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC v2][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists
2015-03-13 0:42 ` [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2015-03-13 0:43 ` [RFC v2][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
@ 2015-03-13 0:44 ` Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2015-03-13 0:44 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Bjorn Helgaas, ACPI Devel Maling List
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
pnp_register_protocol() and __pnp_add_device() both have a problem
that if device_register() fails, the objects they create will be left
in the lists they have been put one beforehand. Unfortunately, that
is not handled by the callers of those routines either, so in case
of a device registration errors the PNP bus type's data structures
will end up in an inconsistent state.
Make pnp_register_protocol() and __pnp_add_device() remove the
objects from the lists if device registration fails.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/pnp/core.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 40 insertions(+), 13 deletions(-)
Index: linux-pm/drivers/pnp/core.c
===================================================================
--- linux-pm.orig/drivers/pnp/core.c
+++ linux-pm/drivers/pnp/core.c
@@ -42,6 +42,13 @@ void *pnp_alloc(long size)
return result;
}
+static void pnp_remove_protocol(struct pnp_protocol *protocol)
+{
+ mutex_lock(&pnp_lock);
+ list_del(&protocol->protocol_list);
+ mutex_unlock(&pnp_lock);
+}
+
/**
* pnp_protocol_register - adds a pnp protocol to the pnp layer
* @protocol: pointer to the corresponding pnp_protocol structure
@@ -50,12 +57,13 @@ void *pnp_alloc(long size)
*/
int pnp_register_protocol(struct pnp_protocol *protocol)
{
- int nodenum;
struct list_head *pos;
+ int nodenum, ret;
INIT_LIST_HEAD(&protocol->devices);
INIT_LIST_HEAD(&protocol->cards);
nodenum = 0;
+
mutex_lock(&pnp_lock);
/* assign the lowest unused number */
@@ -67,12 +75,18 @@ int pnp_register_protocol(struct pnp_pro
}
}
+ protocol->number = nodenum;
+ dev_set_name(&protocol->dev, "pnp%d", nodenum);
+
list_add_tail(&protocol->protocol_list, &pnp_protocols);
+
mutex_unlock(&pnp_lock);
- protocol->number = nodenum;
- dev_set_name(&protocol->dev, "pnp%d", nodenum);
- return device_register(&protocol->dev);
+ ret = device_register(&protocol->dev);
+ if (ret)
+ pnp_remove_protocol(protocol);
+
+ return ret;
}
/**
@@ -81,9 +95,7 @@ int pnp_register_protocol(struct pnp_pro
*/
void pnp_unregister_protocol(struct pnp_protocol *protocol)
{
- mutex_lock(&pnp_lock);
- list_del(&protocol->protocol_list);
- mutex_unlock(&pnp_lock);
+ pnp_remove_protocol(protocol);
device_unregister(&protocol->dev);
}
@@ -158,18 +170,36 @@ struct pnp_dev *pnp_alloc_dev(struct pnp
return dev;
}
+static void pnp_delist_device(struct pnp_dev *dev)
+{
+ mutex_lock(&pnp_lock);
+ list_del(&dev->global_list);
+ list_del(&dev->protocol_list);
+ mutex_unlock(&pnp_lock);
+}
+
int __pnp_add_device(struct pnp_dev *dev)
{
+ int ret;
+
pnp_fixup_device(dev);
dev->status = PNP_READY;
+
mutex_lock(&pnp_lock);
+
list_add_tail(&dev->global_list, &pnp_global);
list_add_tail(&dev->protocol_list, &dev->protocol->devices);
+
mutex_unlock(&pnp_lock);
- if (dev->protocol->can_wakeup)
+
+ ret = device_register(&dev->dev);
+ if (ret)
+ pnp_delist_device(dev);
+ else if (dev->protocol->can_wakeup)
device_set_wakeup_capable(&dev->dev,
dev->protocol->can_wakeup(dev));
- return device_register(&dev->dev);
+
+ return ret;
}
/*
@@ -204,10 +234,7 @@ int pnp_add_device(struct pnp_dev *dev)
void __pnp_remove_device(struct pnp_dev *dev)
{
- mutex_lock(&pnp_lock);
- list_del(&dev->global_list);
- list_del(&dev->protocol_list);
- mutex_unlock(&pnp_lock);
+ pnp_delist_device(dev);
device_unregister(&dev->dev);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-13 0:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-12 1:20 [RFC][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2015-03-12 1:21 ` [RFC][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
2015-03-12 1:22 ` [RFC][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
2015-03-13 0:42 ` [RFC v2][PATCH 0/2] PNP: Avoid potential error code path problems Rafael J. Wysocki
2015-03-13 0:43 ` [RFC v2][PATCH 1/2] PNP: Convert pnp_lock into a mutex Rafael J. Wysocki
2015-03-13 0:44 ` [RFC v2][PATCH 2/2] PNP: Avoid leaving unregistered device objects in lists Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).