[char-misc-next V3] mei: add reference counting for me clients
To support dynamic addition and removal of
me clients we add reference counter.
Update kdoc with locking requirements.
Signed-off-by: Tomas Winkler
---
V2: add locking requirements to kdoc
V3: fix warning: 'cb/me_cl' may be used uninitialized in this function
drivers/misc/mei/amthif.c | 14 +++--
drivers/misc/mei/bus.c | 39 +++-
drivers/misc/mei/client.c | 152 ++---
drivers/misc/mei/client.h | 17 +++--
drivers/misc/mei/debugfs.c | 32 ++
drivers/misc/mei/hbm.c | 34 +-
drivers/misc/mei/main.c| 22 +++
drivers/misc/mei/mei_dev.h | 2 +
drivers/misc/mei/nfc.c | 2 +
drivers/misc/mei/wd.c | 1 +
10 files changed, 221 insertions(+), 94 deletions(-)
diff --git a/drivers/misc/mei/amthif.c b/drivers/misc/mei/amthif.c
index 79f53941779d..c4cb9a984a5f 100644
--- a/drivers/misc/mei/amthif.c
+++ b/drivers/misc/mei/amthif.c
@@ -97,23 +97,25 @@ int mei_amthif_host_init(struct mei_device *dev)
/* allocate storage for ME message buffer */
msg_buf = kcalloc(dev->iamthif_mtu,
sizeof(unsigned char), GFP_KERNEL);
- if (!msg_buf)
- return -ENOMEM;
+ if (!msg_buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
dev->iamthif_msg_buf = msg_buf;
ret = mei_cl_link(cl, MEI_IAMTHIF_HOST_CLIENT_ID);
-
if (ret < 0) {
- dev_err(dev->dev,
- "amthif: failed link client %d\n", ret);
- return ret;
+ dev_err(dev->dev, "amthif: failed cl_link %d\n", ret);
+ goto out;
}
ret = mei_cl_connect(cl, NULL);
dev->iamthif_state = MEI_IAMTHIF_IDLE;
+out:
+ mei_me_cl_put(me_cl);
return ret;
}
diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index 31164dd14bd0..be767f4db26a 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -228,8 +228,8 @@ static ssize_t ___mei_cl_send(struct mei_cl *cl, u8 *buf,
size_t length,
bool blocking)
{
struct mei_device *dev;
- struct mei_me_client *me_cl;
- struct mei_cl_cb *cb;
+ struct mei_me_client *me_cl = NULL;
+ struct mei_cl_cb *cb = NULL;
ssize_t rets;
if (WARN_ON(!cl || !cl->dev))
@@ -237,33 +237,40 @@ static ssize_t ___mei_cl_send(struct mei_cl *cl, u8 *buf,
size_t length,
dev = cl->dev;
- if (cl->state != MEI_FILE_CONNECTED)
- return -ENODEV;
+ mutex_lock(&dev->device_lock);
+ if (cl->state != MEI_FILE_CONNECTED) {
+ rets = -ENODEV;
+ goto out;
+ }
/* Check if we have an ME client device */
me_cl = mei_me_cl_by_uuid_id(dev, &cl->cl_uuid, cl->me_client_id);
- if (!me_cl)
- return -ENOTTY;
+ if (!me_cl) {
+ rets = -ENOTTY;
+ goto out;
+ }
- if (length > me_cl->props.max_msg_length)
- return -EFBIG;
+ if (length > me_cl->props.max_msg_length) {
+ rets = -EFBIG;
+ goto out;
+ }
cb = mei_io_cb_init(cl, NULL);
- if (!cb)
- return -ENOMEM;
+ if (!cb) {
+ rets = -ENOMEM;
+ goto out;
+ }
rets = mei_io_cb_alloc_req_buf(cb, length);
- if (rets < 0) {
- mei_io_cb_free(cb);
- return rets;
- }
+ if (rets < 0)
+ goto out;
memcpy(cb->request_buffer.data, buf, length);
- mutex_lock(&dev->device_lock);
-
rets = mei_cl_write(cl, cb, blocking);
+out:
+ mei_me_cl_put(me_cl);
mutex_unlock(&dev->device_lock);
if (rets < 0)
mei_io_cb_free(cb);
diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 28e004b52dd2..9541218c10eb 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -27,7 +27,63 @@
#include "client.h"
/**
+ * mei_me_cl_init - initialize me client
+ *
+ * @me_cl: me client
+ */
+void mei_me_cl_init(struct mei_me_client *me_cl)
+{
+ INIT_LIST_HEAD(&me_cl->list);
+ kref_init(&me_cl->refcnt);
+}
+
+/**
+ * mei_me_cl_get - increases me client refcount
+ *
+ * @me_cl: me client
+ *
+ * Locking: called under "dev->device_lock" lock
+ *
+ * Return: me client or NULL
+ */
+struct mei_me_client *mei_me_cl_get(struct mei_me_client *me_cl)
+{
+ if (me_cl)
+ kref_get(&me_cl->refcnt);
+
+ return me_cl;
+}
+
+/**
+ * mei_me_cl_release - unlink and free me client
+ *
+ * Locking: called under "dev->device_lock" lock
+ *
+ * @ref: me_client refcount
+ */
+static void mei_me_cl_release(struct kref *ref)
+{
+ struct mei_me_client *me_cl =
+ container_of(ref, struct mei_me_client, refcnt);
+ list_del(&me_cl->list);
+ kfree(me_cl);
+}
+/**
+ * mei_me_cl_put - decrease me
RE: [char-misc-next V3] mei: add reference counting for me clients
> -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Thursday, November 06, 2014 17:58 > To: Winkler, Tomas > Cc: a...@arndb.de; linux-kernel@vger.kernel.org > Subject: Re: [char-misc-next V3] mei: add reference counting for me clients > > On Thu, Nov 06, 2014 at 08:40:21AM +, Winkler, Tomas wrote: > > > > > > On Tue, Nov 04, 2014 at 05:22:51AM +, Winkler, Tomas wrote: > > > > > > > > > > On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > > > > > > To support dynamic addition/remove we add reference > > > > > > counter. > > > > > > > > > > What is keeping two different threads / cpus from grabbing a reference > > > > > at the same time the other one is dropping it? > > > > > > > > > > You have a list here, with no locking, right? You also don't have any > > > > > locking for your kref, which isn't good at all... > > > > > > > > > > Please audit and fix up. > > > > > > > > Of course there is a lock, it wouldn't work at all. It is not explicit > > > > but we run under device_lock mutex. > > > > > > Please make it explicit :) > > > > Not sure what you mean by that? There is a lot of options in this laconic > sentence. > > In looking at that patch, it was not obvious to me that any locking was > happening at all, so that needs to be addressed somehow before I can > accept the change. I understand the concern though we are using a BIG driver mutex so no additional locking is required, we are already locked. Currently we didn't hit any performance issue that required more fine grained locking but let say this is something we aware and that might happen in the future. Thanks Tomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [char-misc-next V3] mei: add reference counting for me clients
On Thu, Nov 06, 2014 at 08:40:21AM +, Winkler, Tomas wrote: > > > > On Tue, Nov 04, 2014 at 05:22:51AM +, Winkler, Tomas wrote: > > > > > > > > On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > > > > > To support dynamic addition/remove we add reference > > > > > counter. > > > > > > > > What is keeping two different threads / cpus from grabbing a reference > > > > at the same time the other one is dropping it? > > > > > > > > You have a list here, with no locking, right? You also don't have any > > > > locking for your kref, which isn't good at all... > > > > > > > > Please audit and fix up. > > > > > > Of course there is a lock, it wouldn't work at all. It is not explicit > > > but we run under device_lock mutex. > > > > Please make it explicit :) > > Not sure what you mean by that? There is a lot of options in this laconic > sentence. In looking at that patch, it was not obvious to me that any locking was happening at all, so that needs to be addressed somehow before I can accept the change. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [char-misc-next V3] mei: add reference counting for me clients
> > On Tue, Nov 04, 2014 at 05:22:51AM +, Winkler, Tomas wrote: > > > > > > On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > > > > To support dynamic addition/remove we add reference > > > > counter. > > > > > > What is keeping two different threads / cpus from grabbing a reference > > > at the same time the other one is dropping it? > > > > > > You have a list here, with no locking, right? You also don't have any > > > locking for your kref, which isn't good at all... > > > > > > Please audit and fix up. > > > > Of course there is a lock, it wouldn't work at all. It is not explicit > > but we run under device_lock mutex. > > Please make it explicit :) Not sure what you mean by that? There is a lot of options in this laconic sentence. Thanks Tomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [char-misc-next V3] mei: add reference counting for me clients
On Tue, Nov 04, 2014 at 05:22:51AM +, Winkler, Tomas wrote: > > > > On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > > > To support dynamic addition/remove we add reference > > > counter. > > > > What is keeping two different threads / cpus from grabbing a reference > > at the same time the other one is dropping it? > > > > You have a list here, with no locking, right? You also don't have any > > locking for your kref, which isn't good at all... > > > > Please audit and fix up. > > Of course there is a lock, it wouldn't work at all. It is not explicit > but we run under device_lock mutex. Please make it explicit :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [char-misc-next V3] mei: add reference counting for me clients
> > On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > > To support dynamic addition/remove we add reference > > counter. > > What is keeping two different threads / cpus from grabbing a reference > at the same time the other one is dropping it? > > You have a list here, with no locking, right? You also don't have any > locking for your kref, which isn't good at all... > > Please audit and fix up. Of course there is a lock, it wouldn't work at all. It is not explicit but we run under device_lock mutex. Tomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [char-misc-next V3] mei: add reference counting for me clients
On Mon, Nov 03, 2014 at 10:42:05AM +0200, Tomas Winkler wrote: > To support dynamic addition/remove we add reference > counter. What is keeping two different threads / cpus from grabbing a reference at the same time the other one is dropping it? You have a list here, with no locking, right? You also don't have any locking for your kref, which isn't good at all... Please audit and fix up. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[char-misc-next V3] mei: add reference counting for me clients
To support dynamic addition/remove we add reference
counter.
Signed-off-by: Tomas Winkler
---
V2: code style and documentation fixes
V3: 1. Use refcounter also in mei_me_cl_rm_all,
2. Replace mei_me_cl_rm_by_id
with mei_me_cl_rm_by_uuid_id
3. Few more doc fixes
drivers/misc/mei/amthif.c | 14 +++--
drivers/misc/mei/bus.c | 39 +++--
drivers/misc/mei/client.c | 140 +
drivers/misc/mei/client.h | 17 --
drivers/misc/mei/debugfs.c | 32 +++
drivers/misc/mei/hbm.c | 34 +--
drivers/misc/mei/main.c| 22 +++
drivers/misc/mei/mei_dev.h | 2 +
drivers/misc/mei/nfc.c | 2 +
drivers/misc/mei/wd.c | 1 +
10 files changed, 209 insertions(+), 94 deletions(-)
diff --git a/drivers/misc/mei/amthif.c b/drivers/misc/mei/amthif.c
index 56255ffe5a84..cafd9470a856 100644
--- a/drivers/misc/mei/amthif.c
+++ b/drivers/misc/mei/amthif.c
@@ -97,23 +97,25 @@ int mei_amthif_host_init(struct mei_device *dev)
/* allocate storage for ME message buffer */
msg_buf = kcalloc(dev->iamthif_mtu,
sizeof(unsigned char), GFP_KERNEL);
- if (!msg_buf)
- return -ENOMEM;
+ if (!msg_buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
dev->iamthif_msg_buf = msg_buf;
ret = mei_cl_link(cl, MEI_IAMTHIF_HOST_CLIENT_ID);
-
if (ret < 0) {
- dev_err(dev->dev,
- "amthif: failed link client %d\n", ret);
- return ret;
+ dev_err(dev->dev, "amthif: failed cl_link %d\n", ret);
+ goto out;
}
ret = mei_cl_connect(cl, NULL);
dev->iamthif_state = MEI_IAMTHIF_IDLE;
+out:
+ mei_me_cl_put(me_cl);
return ret;
}
diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index b3a72bca5242..b2de93724f5b 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -228,8 +228,8 @@ static int ___mei_cl_send(struct mei_cl *cl, u8 *buf,
size_t length,
bool blocking)
{
struct mei_device *dev;
- struct mei_me_client *me_cl;
- struct mei_cl_cb *cb;
+ struct mei_me_client *me_cl = NULL;
+ struct mei_cl_cb *cb = NULL;
int rets;
if (WARN_ON(!cl || !cl->dev))
@@ -237,33 +237,40 @@ static int ___mei_cl_send(struct mei_cl *cl, u8 *buf,
size_t length,
dev = cl->dev;
- if (cl->state != MEI_FILE_CONNECTED)
- return -ENODEV;
+ mutex_lock(&dev->device_lock);
+ if (cl->state != MEI_FILE_CONNECTED) {
+ rets = -ENODEV;
+ goto out;
+ }
/* Check if we have an ME client device */
me_cl = mei_me_cl_by_uuid_id(dev, &cl->cl_uuid, cl->me_client_id);
- if (!me_cl)
- return -ENOTTY;
+ if (!me_cl) {
+ rets = -ENOTTY;
+ goto out;
+ }
- if (length > me_cl->props.max_msg_length)
- return -EFBIG;
+ if (length > me_cl->props.max_msg_length) {
+ rets = -EFBIG;
+ goto out;
+ }
cb = mei_io_cb_init(cl, NULL);
- if (!cb)
- return -ENOMEM;
+ if (!cb) {
+ rets = -ENOMEM;
+ goto out;
+ }
rets = mei_io_cb_alloc_req_buf(cb, length);
- if (rets < 0) {
- mei_io_cb_free(cb);
- return rets;
- }
+ if (rets < 0)
+ goto out;
memcpy(cb->request_buffer.data, buf, length);
- mutex_lock(&dev->device_lock);
-
rets = mei_cl_write(cl, cb, blocking);
+out:
+ mei_me_cl_put(me_cl);
mutex_unlock(&dev->device_lock);
if (rets < 0)
mei_io_cb_free(cb);
diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 03c95e06ed1b..3a0bac150c73 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -27,7 +27,57 @@
#include "client.h"
/**
+ * mei_me_cl_init - initialize me client
+ *
+ * @me_cl: me client
+ */
+void mei_me_cl_init(struct mei_me_client *me_cl)
+{
+ INIT_LIST_HEAD(&me_cl->list);
+ kref_init(&me_cl->refcnt);
+}
+
+/**
+ * mei_me_cl_get - increases me client refcount
+ *
+ * @me_cl: me client
+ *
+ * Return: me client or NULL
+ */
+struct mei_me_client *mei_me_cl_get(struct mei_me_client *me_cl)
+{
+ if (me_cl)
+ kref_get(&me_cl->refcnt);
+
+ return me_cl;
+}
+
+/**
+ * mei_me_cl_release - unlink and free me client
+ *
+ * @ref: me_client refcount
+ */
+static void mei_me_cl_release(struct kref *ref)
+{
+ struct mei_me_client *me_cl =
+ container_of(ref, struct mei_me_client, refcnt);
+ list_del(&me_cl->list);
+ kfree(me_cl);
+}
+/**
+ * mei_me_cl_put - decrease me client refcount and free client if necessary
+ *
+ * @me_cl: me client
+ */
+void mei_me_cl_put(str
