* [RFC] w1: fixes and bundling replies
@ 2014-03-23 1:27 David Fries
2014-03-23 1:27 ` [PATCH 1/3] w1: fix netlink refcnt leak on error path David Fries
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: David Fries @ 2014-03-23 1:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Evgeniy Polyakov
On Fri, Feb 21, 2014 at 01:07:28AM +0400, Evgeniy Polyakov wrote:
> Your approach and patch seem correct, but I worry about how old
> commands are processed. Do I get it right, that replies to old
> non-bundle commands are queued back instead of sending immediately,
> but since it is not bundle but single command, queued reply is being
> sent 'almost' immediately? Basically, nothing changed to old
> clients, but there is new way to send batch requests now?
Yes, with the previous patch set, if a client sent one command per
message they would get the replies in individual packets. However in
some cases clients had to bundle multiple commands in one message.
For instance when using a slave command to read a temperature sensor
it takes one W1_CMD_WRITE command to send a W1_READ_SCRATCHPAD, then
another W1_CMD_READ command to read the data back and they have to be
sent in the same W1_SLAVE_CMD because there can't be a reset between
the two write and read, and you can't do that with a slave command.
You can with master commands, but if you are going to issue individual
reset, write, and read commands why would you not bundle them?
Here is a new set of patches making bundling opt in.
In this version of the patch, W1_CN_BUNDLE set in the cn_msg.flags
enables the kernel to bundle the reply, otherwise replies aren't
bundled.
The previous patch separated the data replies and status replies
because they have a different ack value which is in the cn_msg and
cn_netlink_send could only send one cn_msg in a call. I didn't much
like that solution because now status and data replies were out of
order. This version creates cn_netlink_send_mult which takes a length
which allows multiple cn_msg structures to be sent at once (inside one
nlmsghdr, so clients can see there are more cn_msg structures to
read), so now status and data messages can be sent in order.
This is somewhat suboptimal as each command has a status reply and
possibly a data reply, each requiring a different ack, so even if a
client sent multiple commands in one w1_netlink_msg like,
cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [read], w1_netlink_cmd [read]
the response can't pack the commands into one w1_netlink_msg because
of the ack, so there's duplicate of cn_msg and w1_netlink_msg
structures,
cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data]
cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status]
cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data]
cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status]
cn_msg is 20 bytes, w1_netlink_msg 12, so compared to sending separate
packets with all the context switches, select overhead and such,
bundling is going to be well worth it.
Another alternative could be one cn_msg [seq+1] with the data, and
followed by cn_msg [ack ] with the status messages, but they are back
to out of order.
I wasn't completely sure what to call the new sending function, I
decided on mult for multiple cn_netlink_send_mult, cn_netlink_send_len
as another idea, or if there are any better ideas let me know.
I tested with my client program that will bundle up 14 temperature
sensor conversions, then after a delay, bundle up another set of
commands to read all 14 with the bundle bit set. I also tested with a
two year old version of the software that sends requests two one slave
at a time (bundling only the write/read to get the data out), and
doesn't have code to read the bundling the this patch adds. Both
operate correctly running at the same time.
Posted before, no changes
connector support for sending multiple cn_msg
bundling,
corrects ack value to previous ack or seq + 1
corrects the variables to be name consistently
Documentation/connector/connector.txt | 15 +-
Documentation/w1/w1.generic | 2 +-
Documentation/w1/w1.netlink | 13 +-
drivers/connector/connector.c | 17 +-
drivers/w1/w1.h | 8 -
drivers/w1/w1_netlink.c | 673 ++++++++++++++++++++-------------
drivers/w1/w1_netlink.h | 36 ++
include/linux/connector.h | 1 +
8 files changed, 489 insertions(+), 276 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] w1: fix netlink refcnt leak on error path
2014-03-23 1:27 [RFC] w1: fixes and bundling replies David Fries
@ 2014-03-23 1:27 ` David Fries
2014-03-23 1:27 ` [PATCH 2/3] connector: allow multiple messages to be sent in one packet David Fries
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: David Fries @ 2014-03-23 1:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Evgeniy Polyakov
If the message type is W1_MASTER_CMD or W1_SLAVE_CMD, then a reference
is taken when searching for the slave or master device. If there
isn't any following data m->len (mlen is a copy) is 0 and packing up
the message for later execution is skipped leaving nothing to
decrement the reference counts.
Way back when, m->len was checked before the search that increments the
reference count, but W1_LIST_MASTERS has no additional data, the check
was moved in 9be62e0b2fadaf5ff causing this bug.
This change reorders to put the check before the reference count is
incremented avoiding the problem.
Signed-off-by: David Fries <David@Fries.net>
---
drivers/w1/w1_netlink.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index 5234964..a02704a 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -300,12 +300,6 @@ static int w1_process_command_root(struct cn_msg *msg,
struct w1_netlink_msg *w;
u32 *id;
- if (mcmd->type != W1_LIST_MASTERS) {
- printk(KERN_NOTICE "%s: msg: %x.%x, wrong type: %u, len: %u.\n",
- __func__, msg->id.idx, msg->id.val, mcmd->type, mcmd->len);
- return -EPROTO;
- }
-
cn = kmalloc(PAGE_SIZE, GFP_KERNEL);
if (!cn)
return -ENOMEM;
@@ -441,6 +435,9 @@ static void w1_process_cb(struct w1_master *dev, struct w1_async_cmd *async_cmd)
w1_netlink_send_error(&node->block->msg, node->m, cmd,
node->block->portid, err);
+ /* ref taken in w1_search_slave or w1_search_master_id when building
+ * the block
+ */
if (sl)
w1_unref_slave(sl);
else
@@ -503,30 +500,42 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
msg_len = msg->len;
while (msg_len && !err) {
- struct w1_reg_num id;
- u16 mlen = m->len;
dev = NULL;
sl = NULL;
- memcpy(&id, m->id.id, sizeof(id));
-#if 0
- printk("%s: %02x.%012llx.%02x: type=%02x, len=%u.\n",
- __func__, id.family, (unsigned long long)id.id, id.crc, m->type, m->len);
-#endif
if (m->len + sizeof(struct w1_netlink_msg) > msg_len) {
err = -E2BIG;
break;
}
+ /* execute on this thread, no need to process later */
+ if (m->type == W1_LIST_MASTERS) {
+ err = w1_process_command_root(msg, m, nsp->portid);
+ goto out_cont;
+ }
+
+ /* All following message types require additional data,
+ * check here before references are taken.
+ */
+ if (!m->len) {
+ err = -EPROTO;
+ goto out_cont;
+ }
+
+ /* both search calls take reference counts */
if (m->type == W1_MASTER_CMD) {
dev = w1_search_master_id(m->id.mst.id);
} else if (m->type == W1_SLAVE_CMD) {
- sl = w1_search_slave(&id);
+ sl = w1_search_slave((struct w1_reg_num *)m->id.id);
if (sl)
dev = sl->master;
} else {
- err = w1_process_command_root(msg, m, nsp->portid);
+ printk(KERN_NOTICE
+ "%s: msg: %x.%x, wrong type: %u, len: %u.\n",
+ __func__, msg->id.idx, msg->id.val,
+ m->type, m->len);
+ err = -EPROTO;
goto out_cont;
}
@@ -536,8 +545,6 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
}
err = 0;
- if (!mlen)
- goto out_cont;
atomic_inc(&block->refcnt);
node->async.cb = w1_process_cb;
@@ -557,7 +564,8 @@ out_cont:
if (err)
w1_netlink_send_error(msg, m, NULL, nsp->portid, err);
msg_len -= sizeof(struct w1_netlink_msg) + m->len;
- m = (struct w1_netlink_msg *)(((u8 *)m) + sizeof(struct w1_netlink_msg) + m->len);
+ m = (struct w1_netlink_msg *)(((u8 *)m) +
+ sizeof(struct w1_netlink_msg) + m->len);
/*
* Let's allow requests for nonexisting devices.
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] connector: allow multiple messages to be sent in one packet
2014-03-23 1:27 [RFC] w1: fixes and bundling replies David Fries
2014-03-23 1:27 ` [PATCH 1/3] w1: fix netlink refcnt leak on error path David Fries
@ 2014-03-23 1:27 ` David Fries
2014-03-23 1:27 ` [PATCH 3/3] w1: optional bundling of netlink kernel replies David Fries
2014-04-07 14:13 ` [RFC] w1: fixes and bundling replies David Fries
3 siblings, 0 replies; 6+ messages in thread
From: David Fries @ 2014-03-23 1:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Evgeniy Polyakov
This increases the amount of bundling to reduce the number of packets
sent. For the one wire use there can be multiple struct
w1_netlink_cmd in a struct w1_netlink_msg and multiple of those in
struct cn_msg, and with this change multiple of those in a struct
nlmsghdr, and at each level the len identifies there being multiple of
the next.
Signed-off-by: David Fries <David@Fries.net>
---
Documentation/connector/connector.txt | 13 ++++++++++---
drivers/connector/connector.c | 17 +++++++++++++++--
include/linux/connector.h | 1 +
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/Documentation/connector/connector.txt b/Documentation/connector/connector.txt
index e5c5f5e..e56abdb 100644
--- a/Documentation/connector/connector.txt
+++ b/Documentation/connector/connector.txt
@@ -24,7 +24,8 @@ netlink based networking for inter-process communication in a significantly
easier way:
int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
-void cn_netlink_send(struct cn_msg *msg, u32 __group, int gfp_mask);
+void cn_netlink_send_multi(struct cn_msg *msg, u16 len, u32 portid, u32 __group, int gfp_mask);
+void cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group, int gfp_mask);
struct cb_id
{
@@ -71,15 +72,21 @@ void cn_del_callback(struct cb_id *id);
struct cb_id *id - unique connector's user identifier.
-int cn_netlink_send(struct cn_msg *msg, u32 __groups, int gfp_mask);
+int cn_netlink_send_multi(struct cn_msg *msg, u16 len, u32 portid, u32 __groups, int gfp_mask);
+int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __groups, int gfp_mask);
Sends message to the specified groups. It can be safely called from
softirq context, but may silently fail under strong memory pressure.
If there are no listeners for given group -ESRCH can be returned.
struct cn_msg * - message header(with attached data).
+ u16 len - for *_multi multiple cn_msg messages can be sent
+ u32 port - destination port.
+ If non-zero the message will be sent to the
+ given port, which should be set to the
+ original sender.
u32 __group - destination group.
- If __group is zero, then appropriate group will
+ If port and __group is zero, then appropriate group will
be searched through all registered connector users,
and message will be delivered to the group which was
created for user with the same ID as in msg.
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 77afe74..f4b0cdb 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -43,6 +43,8 @@ static struct cn_dev cdev;
static int cn_already_initialized;
/*
+ * Sends mult (multiple) cn_msg at a time.
+ *
* msg->seq and msg->ack are used to determine message genealogy.
* When someone sends message it puts there locally unique sequence
* and random acknowledge numbers. Sequence number may be copied into
@@ -62,10 +64,13 @@ static int cn_already_initialized;
* the acknowledgement number in the original message + 1, then it is
* a new message.
*
+ * If msg->len != len, then additional cn_msg messages are expected following
+ * the first msg.
+ *
* The message is sent to, the portid if given, the group if given, both if
* both, or if both are zero then the group is looked up and sent there.
*/
-int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,
+int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 __group,
gfp_t gfp_mask)
{
struct cn_callback_entry *__cbq;
@@ -98,7 +103,7 @@ int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,
if (!portid && !netlink_has_listeners(dev->nls, group))
return -ESRCH;
- size = sizeof(*msg) + msg->len;
+ size = sizeof(*msg) + len;
skb = nlmsg_new(size, gfp_mask);
if (!skb)
@@ -121,6 +126,14 @@ int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,
gfp_mask);
return netlink_unicast(dev->nls, skb, portid, !(gfp_mask&__GFP_WAIT));
}
+EXPORT_SYMBOL_GPL(cn_netlink_send_mult);
+
+/* same as cn_netlink_send_mult except msg->len is used for len */
+int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,
+ gfp_t gfp_mask)
+{
+ return cn_netlink_send_mult(msg, msg->len, portid, __group, gfp_mask);
+}
EXPORT_SYMBOL_GPL(cn_netlink_send);
/*
diff --git a/include/linux/connector.h b/include/linux/connector.h
index be9c4747..f8fe863 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -71,6 +71,7 @@ struct cn_dev {
int cn_add_callback(struct cb_id *id, const char *name,
void (*callback)(struct cn_msg *, struct netlink_skb_parms *));
void cn_del_callback(struct cb_id *);
+int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 group, gfp_t gfp_mask);
int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 group, gfp_t gfp_mask);
int cn_queue_add_callback(struct cn_queue_dev *dev, const char *name,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] w1: optional bundling of netlink kernel replies
2014-03-23 1:27 [RFC] w1: fixes and bundling replies David Fries
2014-03-23 1:27 ` [PATCH 1/3] w1: fix netlink refcnt leak on error path David Fries
2014-03-23 1:27 ` [PATCH 2/3] connector: allow multiple messages to be sent in one packet David Fries
@ 2014-03-23 1:27 ` David Fries
2014-04-07 14:13 ` [RFC] w1: fixes and bundling replies David Fries
3 siblings, 0 replies; 6+ messages in thread
From: David Fries @ 2014-03-23 1:27 UTC (permalink / raw)
To: linux-kernel; +Cc: Evgeniy Polyakov
Applications can submit a set of commands in one packet to the kernel,
and in some cases it is required such as reading the temperature
sensor results. This adds an option W1_CN_BUNDLE to the flags of
cn_msg to request the kernel to reply in one packet for efficiency.
The cn_msg flags now check for unknown flag values and return an error
if one is seen. See "Proper handling of unknown flags in system
calls" http://lwn.net/Articles/588444/
This corrects the ack values returned as per the protocol standard,
namely the original ack for status messages and seq + 1 for all others
such as the data returned from a read.
Signed-off-by: David Fries <David@Fries.net>
Some of the common variable names have been standardized as follows.
struct cn_msg *cn
struct w1_netlink_msg *msg
struct w1_netlink_cmd *cmd
struct w1_master *dev
When an argument and a function scope variable would collide, add req_
to the argument.
---
Documentation/connector/connector.txt | 2 +-
Documentation/w1/w1.generic | 2 +-
Documentation/w1/w1.netlink | 13 +-
drivers/w1/w1.h | 8 -
drivers/w1/w1_netlink.c | 649 ++++++++++++++++++++-------------
drivers/w1/w1_netlink.h | 36 ++
6 files changed, 447 insertions(+), 263 deletions(-)
diff --git a/Documentation/connector/connector.txt b/Documentation/connector/connector.txt
index e56abdb..f6215f9 100644
--- a/Documentation/connector/connector.txt
+++ b/Documentation/connector/connector.txt
@@ -118,7 +118,7 @@ acknowledge number MUST be the same + 1.
If we receive a message and its sequence number is not equal to one we
are expecting, then it is a new message. If we receive a message and
its sequence number is the same as one we are expecting, but its
-acknowledge is not equal to the acknowledge number in the original
+acknowledge is not equal to the sequence number in the original
message + 1, then it is a new message.
Obviously, the protocol header contains the above id.
diff --git a/Documentation/w1/w1.generic b/Documentation/w1/w1.generic
index a31c5a2..b2033c6 100644
--- a/Documentation/w1/w1.generic
+++ b/Documentation/w1/w1.generic
@@ -82,7 +82,7 @@ driver - (standard) symlink to the w1 driver
w1_master_add - Manually register a slave device
w1_master_attempts - the number of times a search was attempted
w1_master_max_slave_count
- - the maximum slaves that may be attached to a master
+ - maximum number of slaves to search for at a time
w1_master_name - the name of the device (w1_bus_masterX)
w1_master_pullup - 5V strong pullup 0 enabled, 1 disabled
w1_master_remove - Manually remove a slave device
diff --git a/Documentation/w1/w1.netlink b/Documentation/w1/w1.netlink
index 927a52c..ef27271 100644
--- a/Documentation/w1/w1.netlink
+++ b/Documentation/w1/w1.netlink
@@ -30,7 +30,7 @@ Protocol.
W1_SLAVE_CMD
userspace command for slave device
(read/write/touch)
- __u8 res - reserved
+ __u8 status - error indication from kernel
__u16 len - size of data attached to this header data
union {
__u8 id[8]; - slave unique device id
@@ -44,10 +44,14 @@ Protocol.
__u8 cmd - command opcode.
W1_CMD_READ - read command
W1_CMD_WRITE - write command
- W1_CMD_TOUCH - touch command
- (write and sample data back to userspace)
W1_CMD_SEARCH - search command
W1_CMD_ALARM_SEARCH - alarm search command
+ W1_CMD_TOUCH - touch command
+ (write and sample data back to userspace)
+ W1_CMD_RESET - send bus reset
+ W1_CMD_SLAVE_ADD - add slave to kernel list
+ W1_CMD_SLAVE_REMOVE - remove slave from kernel list
+ W1_CMD_LIST_SLAVES - get slaves list from kernel
__u8 res - reserved
__u16 len - length of data for this command
For read command data must be allocated like for write command
@@ -87,8 +91,7 @@ format:
id0 ... idN
Each message is at most 4k in size, so if number of master devices
- exceeds this, it will be split into several messages,
- cn.seq will be increased for each one.
+ exceeds this, it will be split into several messages.
W1 search and alarm search commands.
request:
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 734dab7..56a49ba 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -203,7 +203,6 @@ enum w1_master_flags {
* @search_id: allows continuing a search
* @refcnt: reference count
* @priv: private data storage
- * @priv_size: size allocated
* @enable_pullup: allows a strong pullup
* @pullup_duration: time for the next strong pullup
* @flags: one of w1_master_flags
@@ -214,7 +213,6 @@ enum w1_master_flags {
* @dev: sysfs device
* @bus_master: io operations available
* @seq: sequence number used for netlink broadcasts
- * @portid: destination for the current netlink command
*/
struct w1_master
{
@@ -241,7 +239,6 @@ struct w1_master
atomic_t refcnt;
void *priv;
- int priv_size;
/** 5V strong pullup enabled flag, 1 enabled, zero disabled. */
int enable_pullup;
@@ -260,11 +257,6 @@ struct w1_master
struct w1_bus_master *bus_master;
u32 seq;
- /* port id to send netlink responses to. The value is temporarily
- * stored here while processing a message, set after locking the
- * mutex, zero before unlocking the mutex.
- */
- u32 portid;
};
/**
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index a02704a..351a297 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -29,51 +29,247 @@
#include "w1_netlink.h"
#if defined(CONFIG_W1_CON) && (defined(CONFIG_CONNECTOR) || (defined(CONFIG_CONNECTOR_MODULE) && defined(CONFIG_W1_MODULE)))
-void w1_netlink_send(struct w1_master *dev, struct w1_netlink_msg *msg)
+
+#define MIN(a, b) (((a) < (b)) ? (a) : (b))
+
+/* Bundle together everything required to process a request in one memory
+ * allocation.
+ */
+struct w1_cb_block {
+ atomic_t refcnt;
+ u32 portid; /* Sending process port ID */
+ /* maximum value for first_cn->len */
+ u16 maxlen;
+ /* pointers to building up the reply message */
+ struct cn_msg *first_cn; /* fixed once the structure is populated */
+ struct cn_msg *cn; /* advances as cn_msg is appeneded */
+ struct w1_netlink_msg *msg; /* advances as w1_netlink_msg is appened */
+ struct w1_netlink_cmd *cmd; /* advances as cmds are appened */
+ struct w1_netlink_msg *cur_msg; /* currently message being processed */
+ /* copy of the original request follows */
+ struct cn_msg request_cn;
+ /* followed by variable length:
+ * cn_msg, data (w1_netlink_msg and w1_netlink_cmd)
+ * one or more struct w1_cb_node
+ * reply first_cn, data (w1_netlink_msg and w1_netlink_cmd)
+ */
+};
+struct w1_cb_node {
+ struct w1_async_cmd async;
+ /* pointers within w1_cb_block and cn data */
+ struct w1_cb_block *block;
+ struct w1_netlink_msg *msg;
+ struct w1_slave *sl;
+ struct w1_master *dev;
+};
+
+/**
+ * w1_reply_len() - calculate current reply length, compare to maxlen
+ * @block: block to calculate
+ *
+ * Calculates the current message length including possible multiple
+ * cn_msg and data, excludes the first sizeof(struct cn_msg). Direclty
+ * compariable to maxlen and usable to send the message.
+ */
+static u16 w1_reply_len(struct w1_cb_block *block)
+{
+ if (!block->cn)
+ return 0;
+ return (u8 *)block->cn - (u8 *)block->first_cn + block->cn->len;
+}
+
+static void w1_unref_block(struct w1_cb_block *block)
+{
+ if (atomic_sub_return(1, &block->refcnt) == 0) {
+ u16 len = w1_reply_len(block);
+ if (len) {
+ cn_netlink_send_mult(block->first_cn, len,
+ block->portid, 0, GFP_KERNEL);
+ }
+ kfree(block);
+ }
+}
+
+/**
+ * w1_reply_make_space() - send message if needed to make space
+ * @block: block to make space on
+ * @space: how many bytes requested
+ *
+ * Verify there is enough room left for the caller to add "space" bytes to the
+ * message, if there isn't send the message and reset.
+ */
+static void w1_reply_make_space(struct w1_cb_block *block, u16 space)
+{
+ u16 len = w1_reply_len(block);
+ if (len + space >= block->maxlen) {
+ cn_netlink_send_mult(block->first_cn, len, block->portid, 0, GFP_KERNEL);
+ block->first_cn->len = 0;
+ block->cn = NULL;
+ block->msg = NULL;
+ block->cmd = NULL;
+ }
+}
+
+/* Early send when replies aren't bundled. */
+static void w1_netlink_check_send(struct w1_cb_block *block)
+{
+ if (!(block->request_cn.flags & W1_CN_BUNDLE) && block->cn)
+ w1_reply_make_space(block, block->maxlen);
+}
+
+/**
+ * w1_netlink_setup_msg() - prepare to write block->msg
+ * @block: block to operate on
+ * @ack: determines if cn can be reused
+ *
+ * block->cn will be setup with the correct ack, advancing if needed
+ * block->cn->len does not include space for block->msg
+ * block->msg advances but remains uninitialized
+ */
+static void w1_netlink_setup_msg(struct w1_cb_block *block, u32 ack)
+{
+ if (block->cn && block->cn->ack == ack) {
+ block->msg = (struct w1_netlink_msg *)(block->cn->data + block->cn->len);
+ } else {
+ /* advance or set to data */
+ if (block->cn)
+ block->cn = (struct cn_msg *)(block->cn->data +
+ block->cn->len);
+ else
+ block->cn = block->first_cn;
+
+ memcpy(block->cn, &block->request_cn, sizeof(*block->cn));
+ block->cn->len = 0;
+ block->cn->ack = ack;
+ block->msg = (struct w1_netlink_msg *)block->cn->data;
+ }
+}
+
+/* Append cmd to msg, include cmd->data as well. This is because
+ * any following data goes with the command and in the case of a read is
+ * the results.
+ */
+static void w1_netlink_queue_cmd(struct w1_cb_block *block,
+ struct w1_netlink_cmd *cmd)
+{
+ u32 space;
+ w1_reply_make_space(block, sizeof(struct cn_msg) +
+ sizeof(struct w1_netlink_msg) + sizeof(*cmd) + cmd->len);
+
+ /* There's a status message sent after each command, so no point
+ * in trying to bundle this cmd after an existing one, because
+ * there won't be one. Allocate and copy over a new cn_msg.
+ */
+ w1_netlink_setup_msg(block, block->request_cn.seq + 1);
+ memcpy(block->msg, block->cur_msg, sizeof(*block->msg));
+ block->cn->len += sizeof(*block->msg);
+ block->msg->len = 0;
+ block->cmd = (struct w1_netlink_cmd *)(block->msg->data);
+
+ space = sizeof(*cmd) + cmd->len;
+ if (block->cmd != cmd)
+ memcpy(block->cmd, cmd, space);
+ block->cn->len += space;
+ block->msg->len += space;
+}
+
+/* Append req_msg and req_cmd, no other commands and no data from req_cmd are
+ * copied.
+ */
+static void w1_netlink_queue_status(struct w1_cb_block *block,
+ struct w1_netlink_msg *req_msg, struct w1_netlink_cmd *req_cmd,
+ int error)
{
- char buf[sizeof(struct cn_msg) + sizeof(struct w1_netlink_msg)];
- struct cn_msg *m = (struct cn_msg *)buf;
- struct w1_netlink_msg *w = (struct w1_netlink_msg *)(m+1);
+ u16 space = sizeof(struct cn_msg) + sizeof(*req_msg) + sizeof(*req_cmd);
+ w1_reply_make_space(block, space);
+ w1_netlink_setup_msg(block, block->request_cn.ack);
+
+ memcpy(block->msg, req_msg, sizeof(*req_msg));
+ block->cn->len += sizeof(*req_msg);
+ block->msg->len = 0;
+ block->msg->status = (u8)-error;
+ if (req_cmd) {
+ struct w1_netlink_cmd *cmd = (struct w1_netlink_cmd *)block->msg->data;
+ memcpy(cmd, req_cmd, sizeof(*cmd));
+ block->cn->len += sizeof(*cmd);
+ block->msg->len += sizeof(*cmd);
+ cmd->len = 0;
+ }
+ w1_netlink_check_send(block);
+}
- memset(buf, 0, sizeof(buf));
+/**
+ * w1_netlink_send_error() - sends the error message now
+ * @cn: original cn_msg
+ * @msg: original w1_netlink_msg
+ * @portid: where to send it
+ * @error: error status
+ *
+ * Use when a block isn't available to queue the message to and cn, msg
+ * might not be contiguous.
+ */
+static void w1_netlink_send_error(struct cn_msg *cn, struct w1_netlink_msg *msg,
+ int portid, int error)
+{
+ struct {
+ struct cn_msg cn;
+ struct w1_netlink_msg msg;
+ } packet;
+ memcpy(&packet.cn, cn, sizeof(packet.cn));
+ memcpy(&packet.msg, msg, sizeof(packet.msg));
+ packet.cn.len = sizeof(packet.msg);
+ packet.msg.len = 0;
+ packet.msg.status = (u8)-error;
+ cn_netlink_send(&packet.cn, portid, 0, GFP_KERNEL);
+}
+
+/**
+ * w1_netlink_send() - sends w1 netlink notifications
+ * @dev: w1_master the even is associated with or for
+ * @msg: w1_netlink_msg message to be sent
+ *
+ * This are notifications generated from the kernel.
+ */
+void w1_netlink_send(struct w1_master *dev, struct w1_netlink_msg *msg)
+{
+ struct {
+ struct cn_msg cn;
+ struct w1_netlink_msg msg;
+ } packet;
+ memset(&packet, 0, sizeof(packet));
- m->id.idx = CN_W1_IDX;
- m->id.val = CN_W1_VAL;
+ packet.cn.id.idx = CN_W1_IDX;
+ packet.cn.id.val = CN_W1_VAL;
- m->seq = dev->seq++;
- m->len = sizeof(struct w1_netlink_msg);
+ packet.cn.seq = dev->seq++;
+ packet.cn.len = sizeof(*msg);
- memcpy(w, msg, sizeof(struct w1_netlink_msg));
+ memcpy(&packet.msg, msg, sizeof(*msg));
+ packet.msg.len = 0;
- cn_netlink_send(m, dev->portid, 0, GFP_KERNEL);
+ cn_netlink_send(&packet.cn, 0, 0, GFP_KERNEL);
}
static void w1_send_slave(struct w1_master *dev, u64 rn)
{
- struct cn_msg *msg = dev->priv;
- struct w1_netlink_msg *hdr = (struct w1_netlink_msg *)(msg + 1);
- struct w1_netlink_cmd *cmd = (struct w1_netlink_cmd *)(hdr + 1);
- int avail;
+ struct w1_cb_block *block = dev->priv;
+ struct w1_netlink_cmd *cache_cmd = block->cmd;
u64 *data;
- avail = dev->priv_size - cmd->len;
+ w1_reply_make_space(block, sizeof(*data));
- if (avail < 8) {
- msg->ack++;
- cn_netlink_send(msg, dev->portid, 0, GFP_KERNEL);
-
- msg->len = sizeof(struct w1_netlink_msg) +
- sizeof(struct w1_netlink_cmd);
- hdr->len = sizeof(struct w1_netlink_cmd);
- cmd->len = 0;
+ /* Add cmd back if the packet was sent */
+ if (!block->cmd) {
+ cache_cmd->len = 0;
+ w1_netlink_queue_cmd(block, cache_cmd);
}
- data = (void *)(cmd + 1) + cmd->len;
+ data = (u64 *)(block->cmd->data + block->cmd->len);
*data = rn;
- cmd->len += 8;
- hdr->len += 8;
- msg->len += 8;
+ block->cn->len += sizeof(*data);
+ block->msg->len += sizeof(*data);
+ block->cmd->len += sizeof(*data);
}
static void w1_found_send_slave(struct w1_master *dev, u64 rn)
@@ -85,40 +281,15 @@ static void w1_found_send_slave(struct w1_master *dev, u64 rn)
}
/* Get the current slave list, or search (with or without alarm) */
-static int w1_get_slaves(struct w1_master *dev,
- struct cn_msg *req_msg, struct w1_netlink_msg *req_hdr,
- struct w1_netlink_cmd *req_cmd)
+static int w1_get_slaves(struct w1_master *dev, struct w1_netlink_cmd *req_cmd)
{
- struct cn_msg *msg;
- struct w1_netlink_msg *hdr;
- struct w1_netlink_cmd *cmd;
struct w1_slave *sl;
- msg = kzalloc(PAGE_SIZE, GFP_KERNEL);
- if (!msg)
- return -ENOMEM;
-
- msg->id = req_msg->id;
- msg->seq = req_msg->seq;
- msg->ack = 0;
- msg->len = sizeof(struct w1_netlink_msg) +
- sizeof(struct w1_netlink_cmd);
-
- hdr = (struct w1_netlink_msg *)(msg + 1);
- cmd = (struct w1_netlink_cmd *)(hdr + 1);
-
- hdr->type = W1_MASTER_CMD;
- hdr->id = req_hdr->id;
- hdr->len = sizeof(struct w1_netlink_cmd);
-
- cmd->cmd = req_cmd->cmd;
- cmd->len = 0;
-
- dev->priv = msg;
- dev->priv_size = PAGE_SIZE - msg->len - sizeof(struct cn_msg);
+ req_cmd->len = 0;
+ w1_netlink_queue_cmd(dev->priv, req_cmd);
if (req_cmd->cmd == W1_CMD_LIST_SLAVES) {
- __u64 rn;
+ u64 rn;
mutex_lock(&dev->list_mutex);
list_for_each_entry(sl, &dev->slist, w1_slave_entry) {
memcpy(&rn, &sl->reg_num, sizeof(rn));
@@ -126,73 +297,26 @@ static int w1_get_slaves(struct w1_master *dev,
}
mutex_unlock(&dev->list_mutex);
} else {
- w1_search_process_cb(dev, cmd->cmd == W1_CMD_ALARM_SEARCH ?
+ w1_search_process_cb(dev, req_cmd->cmd == W1_CMD_ALARM_SEARCH ?
W1_ALARM_SEARCH : W1_SEARCH, w1_found_send_slave);
}
- msg->ack = 0;
- cn_netlink_send(msg, dev->portid, 0, GFP_KERNEL);
-
- dev->priv = NULL;
- dev->priv_size = 0;
-
- kfree(msg);
-
return 0;
}
-static int w1_send_read_reply(struct cn_msg *msg, struct w1_netlink_msg *hdr,
- struct w1_netlink_cmd *cmd, u32 portid)
-{
- void *data;
- struct w1_netlink_msg *h;
- struct w1_netlink_cmd *c;
- struct cn_msg *cm;
- int err;
-
- data = kzalloc(sizeof(struct cn_msg) +
- sizeof(struct w1_netlink_msg) +
- sizeof(struct w1_netlink_cmd) +
- cmd->len, GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- cm = (struct cn_msg *)(data);
- h = (struct w1_netlink_msg *)(cm + 1);
- c = (struct w1_netlink_cmd *)(h + 1);
-
- memcpy(cm, msg, sizeof(struct cn_msg));
- memcpy(h, hdr, sizeof(struct w1_netlink_msg));
- memcpy(c, cmd, sizeof(struct w1_netlink_cmd));
-
- cm->ack = msg->seq+1;
- cm->len = sizeof(struct w1_netlink_msg) +
- sizeof(struct w1_netlink_cmd) + cmd->len;
-
- h->len = sizeof(struct w1_netlink_cmd) + cmd->len;
-
- memcpy(c->data, cmd->data, c->len);
-
- err = cn_netlink_send(cm, portid, 0, GFP_KERNEL);
-
- kfree(data);
-
- return err;
-}
-
-static int w1_process_command_io(struct w1_master *dev, struct cn_msg *msg,
- struct w1_netlink_msg *hdr, struct w1_netlink_cmd *cmd)
+static int w1_process_command_io(struct w1_master *dev,
+ struct w1_netlink_cmd *cmd)
{
int err = 0;
switch (cmd->cmd) {
case W1_CMD_TOUCH:
w1_touch_block(dev, cmd->data, cmd->len);
- w1_send_read_reply(msg, hdr, cmd, dev->portid);
+ w1_netlink_queue_cmd(dev->priv, cmd);
break;
case W1_CMD_READ:
w1_read_block(dev, cmd->data, cmd->len);
- w1_send_read_reply(msg, hdr, cmd, dev->portid);
+ w1_netlink_queue_cmd(dev->priv, cmd);
break;
case W1_CMD_WRITE:
w1_write_block(dev, cmd->data, cmd->len);
@@ -206,14 +330,13 @@ static int w1_process_command_io(struct w1_master *dev, struct cn_msg *msg,
}
static int w1_process_command_addremove(struct w1_master *dev,
- struct cn_msg *msg, struct w1_netlink_msg *hdr,
struct w1_netlink_cmd *cmd)
{
struct w1_slave *sl;
int err = 0;
struct w1_reg_num *id;
- if (cmd->len != 8)
+ if (cmd->len != sizeof(*id))
return -EINVAL;
id = (struct w1_reg_num *)cmd->data;
@@ -241,7 +364,6 @@ static int w1_process_command_addremove(struct w1_master *dev,
}
static int w1_process_command_master(struct w1_master *dev,
- struct cn_msg *req_msg, struct w1_netlink_msg *req_hdr,
struct w1_netlink_cmd *req_cmd)
{
int err = -EINVAL;
@@ -254,13 +376,13 @@ static int w1_process_command_master(struct w1_master *dev,
case W1_CMD_ALARM_SEARCH:
case W1_CMD_LIST_SLAVES:
mutex_unlock(&dev->bus_mutex);
- err = w1_get_slaves(dev, req_msg, req_hdr, req_cmd);
+ err = w1_get_slaves(dev, req_cmd);
mutex_lock(&dev->bus_mutex);
break;
case W1_CMD_READ:
case W1_CMD_WRITE:
case W1_CMD_TOUCH:
- err = w1_process_command_io(dev, req_msg, req_hdr, req_cmd);
+ err = w1_process_command_io(dev, req_cmd);
break;
case W1_CMD_RESET:
err = w1_reset_bus(dev);
@@ -269,8 +391,7 @@ static int w1_process_command_master(struct w1_master *dev,
case W1_CMD_SLAVE_REMOVE:
mutex_unlock(&dev->bus_mutex);
mutex_lock(&dev->mutex);
- err = w1_process_command_addremove(dev, req_msg, req_hdr,
- req_cmd);
+ err = w1_process_command_addremove(dev, req_cmd);
mutex_unlock(&dev->mutex);
mutex_lock(&dev->bus_mutex);
break;
@@ -282,22 +403,21 @@ static int w1_process_command_master(struct w1_master *dev,
return err;
}
-static int w1_process_command_slave(struct w1_slave *sl, struct cn_msg *msg,
- struct w1_netlink_msg *hdr, struct w1_netlink_cmd *cmd)
+static int w1_process_command_slave(struct w1_slave *sl,
+ struct w1_netlink_cmd *cmd)
{
dev_dbg(&sl->master->dev, "%s: %02x.%012llx.%02x: cmd=%02x, len=%u.\n",
__func__, sl->reg_num.family, (unsigned long long)sl->reg_num.id,
sl->reg_num.crc, cmd->cmd, cmd->len);
- return w1_process_command_io(sl->master, msg, hdr, cmd);
+ return w1_process_command_io(sl->master, cmd);
}
-static int w1_process_command_root(struct cn_msg *msg,
- struct w1_netlink_msg *mcmd, u32 portid)
+static int w1_process_command_root(struct cn_msg *req_cn, u32 portid)
{
- struct w1_master *m;
+ struct w1_master *dev;
struct cn_msg *cn;
- struct w1_netlink_msg *w;
+ struct w1_netlink_msg *msg;
u32 *id;
cn = kmalloc(PAGE_SIZE, GFP_KERNEL);
@@ -307,32 +427,30 @@ static int w1_process_command_root(struct cn_msg *msg,
cn->id.idx = CN_W1_IDX;
cn->id.val = CN_W1_VAL;
- cn->seq = msg->seq;
- cn->ack = 1;
+ cn->seq = req_cn->seq;
+ cn->ack = req_cn->seq + 1;
cn->len = sizeof(struct w1_netlink_msg);
- w = (struct w1_netlink_msg *)(cn + 1);
+ msg = (struct w1_netlink_msg *)cn->data;
- w->type = W1_LIST_MASTERS;
- w->status = 0;
- w->len = 0;
- id = (u32 *)(w + 1);
+ msg->type = W1_LIST_MASTERS;
+ msg->status = 0;
+ msg->len = 0;
+ id = (u32 *)msg->data;
mutex_lock(&w1_mlock);
- list_for_each_entry(m, &w1_masters, w1_master_entry) {
+ list_for_each_entry(dev, &w1_masters, w1_master_entry) {
if (cn->len + sizeof(*id) > PAGE_SIZE - sizeof(struct cn_msg)) {
cn_netlink_send(cn, portid, 0, GFP_KERNEL);
- cn->ack++;
cn->len = sizeof(struct w1_netlink_msg);
- w->len = 0;
- id = (u32 *)(w + 1);
+ msg->len = 0;
+ id = (u32 *)msg->data;
}
- *id = m->id;
- w->len += sizeof(*id);
+ *id = dev->id;
+ msg->len += sizeof(*id);
cn->len += sizeof(*id);
id++;
}
- cn->ack = 0;
cn_netlink_send(cn, portid, 0, GFP_KERNEL);
mutex_unlock(&w1_mlock);
@@ -340,100 +458,44 @@ static int w1_process_command_root(struct cn_msg *msg,
return 0;
}
-static int w1_netlink_send_error(struct cn_msg *rcmsg, struct w1_netlink_msg *rmsg,
- struct w1_netlink_cmd *rcmd, int portid, int error)
-{
- struct cn_msg *cmsg;
- struct w1_netlink_msg *msg;
- struct w1_netlink_cmd *cmd;
-
- cmsg = kzalloc(sizeof(*msg) + sizeof(*cmd) + sizeof(*cmsg), GFP_KERNEL);
- if (!cmsg)
- return -ENOMEM;
-
- msg = (struct w1_netlink_msg *)(cmsg + 1);
- cmd = (struct w1_netlink_cmd *)(msg + 1);
-
- memcpy(cmsg, rcmsg, sizeof(*cmsg));
- cmsg->len = sizeof(*msg);
-
- memcpy(msg, rmsg, sizeof(*msg));
- msg->len = 0;
- msg->status = (short)-error;
-
- if (rcmd) {
- memcpy(cmd, rcmd, sizeof(*cmd));
- cmd->len = 0;
- msg->len += sizeof(*cmd);
- cmsg->len += sizeof(*cmd);
- }
-
- error = cn_netlink_send(cmsg, portid, 0, GFP_KERNEL);
- kfree(cmsg);
-
- return error;
-}
-
-/* Bundle together a reference count, the full message, and broken out
- * commands to be executed on each w1 master kthread in one memory allocation.
- */
-struct w1_cb_block {
- atomic_t refcnt;
- u32 portid; /* Sending process port ID */
- struct cn_msg msg;
- /* cn_msg data */
- /* one or more variable length struct w1_cb_node */
-};
-struct w1_cb_node {
- struct w1_async_cmd async;
- /* pointers within w1_cb_block and msg data */
- struct w1_cb_block *block;
- struct w1_netlink_msg *m;
- struct w1_slave *sl;
- struct w1_master *dev;
-};
-
static void w1_process_cb(struct w1_master *dev, struct w1_async_cmd *async_cmd)
{
struct w1_cb_node *node = container_of(async_cmd, struct w1_cb_node,
async);
- u16 mlen = node->m->len;
- u8 *cmd_data = node->m->data;
+ u16 mlen = node->msg->len;
+ u16 len;
int err = 0;
struct w1_slave *sl = node->sl;
- struct w1_netlink_cmd *cmd = NULL;
+ struct w1_netlink_cmd *cmd = (struct w1_netlink_cmd *)node->msg->data;
mutex_lock(&dev->bus_mutex);
- dev->portid = node->block->portid;
+ dev->priv = node->block;
if (sl && w1_reset_select_slave(sl))
err = -ENODEV;
+ node->block->cur_msg = node->msg;
while (mlen && !err) {
- cmd = (struct w1_netlink_cmd *)cmd_data;
-
if (cmd->len + sizeof(struct w1_netlink_cmd) > mlen) {
err = -E2BIG;
break;
}
if (sl)
- err = w1_process_command_slave(sl, &node->block->msg,
- node->m, cmd);
+ err = w1_process_command_slave(sl, cmd);
else
- err = w1_process_command_master(dev, &node->block->msg,
- node->m, cmd);
+ err = w1_process_command_master(dev, cmd);
+ w1_netlink_check_send(node->block);
- w1_netlink_send_error(&node->block->msg, node->m, cmd,
- node->block->portid, err);
+ w1_netlink_queue_status(node->block, node->msg, cmd, err);
err = 0;
- cmd_data += cmd->len + sizeof(struct w1_netlink_cmd);
- mlen -= cmd->len + sizeof(struct w1_netlink_cmd);
+ len = sizeof(*cmd) + cmd->len;
+ cmd = (struct w1_netlink_cmd *)((u8 *)cmd + len);
+ mlen -= len;
}
if (!cmd || err)
- w1_netlink_send_error(&node->block->msg, node->m, cmd,
- node->block->portid, err);
+ w1_netlink_queue_status(node->block, node->msg, cmd, err);
/* ref taken in w1_search_slave or w1_search_master_id when building
* the block
@@ -442,99 +504,186 @@ static void w1_process_cb(struct w1_master *dev, struct w1_async_cmd *async_cmd)
w1_unref_slave(sl);
else
atomic_dec(&dev->refcnt);
- dev->portid = 0;
+ dev->priv = NULL;
mutex_unlock(&dev->bus_mutex);
mutex_lock(&dev->list_mutex);
list_del(&async_cmd->async_entry);
mutex_unlock(&dev->list_mutex);
- if (atomic_sub_return(1, &node->block->refcnt) == 0)
- kfree(node->block);
+ w1_unref_block(node->block);
+}
+
+static void w1_list_count_cmds(struct w1_netlink_msg *msg, int *cmd_count,
+ u16 *slave_len)
+{
+ struct w1_netlink_cmd *cmd = (struct w1_netlink_cmd *)msg->data;
+ u16 mlen = msg->len;
+ u16 len;
+ int slave_list = 0;
+ while (mlen) {
+ if (cmd->len + sizeof(struct w1_netlink_cmd) > mlen)
+ break;
+
+ switch (cmd->cmd) {
+ case W1_CMD_SEARCH:
+ case W1_CMD_ALARM_SEARCH:
+ case W1_CMD_LIST_SLAVES:
+ ++slave_list;
+ }
+ ++*cmd_count;
+ len = sizeof(*cmd) + cmd->len;
+ cmd = (struct w1_netlink_cmd *)((u8 *)cmd + len);
+ mlen -= len;
+ }
+
+ if (slave_list) {
+ struct w1_master *dev = w1_search_master_id(msg->id.mst.id);
+ if (dev) {
+ /* Bytes, and likely an overstimate, and if it isn't
+ * the results can still be split between packets.
+ */
+ *slave_len += sizeof(struct w1_reg_num) * slave_list *
+ (dev->slave_count + dev->max_slave_count);
+ /* search incremented it */
+ atomic_dec(&dev->refcnt);
+ }
+ }
}
-static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
+static void w1_cn_callback(struct cn_msg *cn, struct netlink_skb_parms *nsp)
{
- struct w1_netlink_msg *m = (struct w1_netlink_msg *)(msg + 1);
+ struct w1_netlink_msg *msg = (struct w1_netlink_msg *)(cn + 1);
struct w1_slave *sl;
struct w1_master *dev;
u16 msg_len;
+ u16 slave_len = 0;
int err = 0;
struct w1_cb_block *block = NULL;
struct w1_cb_node *node = NULL;
int node_count = 0;
+ int cmd_count = 0;
+
+ /* If any unknown flag is set let the application know, that way
+ * applications can detect the absence of features in kernels that
+ * don't know about them. http://lwn.net/Articles/587527/
+ */
+ if (cn->flags & ~(W1_CN_BUNDLE)) {
+ w1_netlink_send_error(cn, msg, nsp->portid, -EINVAL);
+ return;
+ }
/* Count the number of master or slave commands there are to allocate
* space for one cb_node each.
*/
- msg_len = msg->len;
+ msg_len = cn->len;
while (msg_len && !err) {
- if (m->len + sizeof(struct w1_netlink_msg) > msg_len) {
+ if (msg->len + sizeof(struct w1_netlink_msg) > msg_len) {
err = -E2BIG;
break;
}
- if (m->type == W1_MASTER_CMD || m->type == W1_SLAVE_CMD)
+ /* count messages for nodes and allocate any additional space
+ * required for slave lists
+ */
+ if (msg->type == W1_MASTER_CMD || msg->type == W1_SLAVE_CMD) {
++node_count;
+ w1_list_count_cmds(msg, &cmd_count, &slave_len);
+ }
- msg_len -= sizeof(struct w1_netlink_msg) + m->len;
- m = (struct w1_netlink_msg *)(((u8 *)m) +
- sizeof(struct w1_netlink_msg) + m->len);
+ msg_len -= sizeof(struct w1_netlink_msg) + msg->len;
+ msg = (struct w1_netlink_msg *)(((u8 *)msg) +
+ sizeof(struct w1_netlink_msg) + msg->len);
}
- m = (struct w1_netlink_msg *)(msg + 1);
+ msg = (struct w1_netlink_msg *)(cn + 1);
if (node_count) {
- /* msg->len doesn't include itself */
- long size = sizeof(struct w1_cb_block) + msg->len +
- node_count*sizeof(struct w1_cb_node);
- block = kmalloc(size, GFP_KERNEL);
+ int size;
+ u16 reply_size = sizeof(*cn) + cn->len + slave_len;
+ if (cn->flags & W1_CN_BUNDLE) {
+ /* bundling duplicats some of the messages */
+ reply_size += 2 * cmd_count * (sizeof(struct cn_msg) +
+ sizeof(struct w1_netlink_msg) +
+ sizeof(struct w1_netlink_cmd));
+ }
+ reply_size = MIN(CONNECTOR_MAX_MSG_SIZE, reply_size);
+
+ /* allocate space for the block, a copy of the original message,
+ * one node per cmd to point into the original message,
+ * space for replies which is the original message size plus
+ * space for any list slave data and status messages
+ * cn->len doesn't include itself which is part of the block
+ * */
+ size = /* block + original message */
+ sizeof(struct w1_cb_block) + sizeof(*cn) + cn->len +
+ /* space for nodes */
+ node_count * sizeof(struct w1_cb_node) +
+ /* replies */
+ sizeof(struct cn_msg) + reply_size;
+ block = kzalloc(size, GFP_KERNEL);
if (!block) {
- w1_netlink_send_error(msg, m, NULL, nsp->portid,
- -ENOMEM);
+ /* if the system is already out of memory,
+ * (A) will this work, and (B) would it be better
+ * to not try?
+ */
+ w1_netlink_send_error(cn, msg, nsp->portid, -ENOMEM);
return;
}
atomic_set(&block->refcnt, 1);
block->portid = nsp->portid;
- memcpy(&block->msg, msg, sizeof(*msg) + msg->len);
- node = (struct w1_cb_node *)((u8 *)block->msg.data + msg->len);
+ memcpy(&block->request_cn, cn, sizeof(*cn) + cn->len);
+ node = (struct w1_cb_node *)(block->request_cn.data + cn->len);
+
+ /* Sneeky, when not bundling, reply_size is the allocated space
+ * required for the reply, cn_msg isn't part of maxlen so
+ * it should be reply_size - sizeof(struct cn_msg), however
+ * when checking if there is enough space, w1_reply_make_space
+ * is called with the full message size including cn_msg,
+ * because it isn't known at that time if an additional cn_msg
+ * will need to be allocated. So an extra cn_msg is added
+ * above in "size".
+ */
+ block->maxlen = reply_size;
+ block->first_cn = (struct cn_msg *)(node + node_count);
+ memset(block->first_cn, 0, sizeof(*block->first_cn));
}
- msg_len = msg->len;
+ msg_len = cn->len;
while (msg_len && !err) {
dev = NULL;
sl = NULL;
- if (m->len + sizeof(struct w1_netlink_msg) > msg_len) {
+ if (msg->len + sizeof(struct w1_netlink_msg) > msg_len) {
err = -E2BIG;
break;
}
/* execute on this thread, no need to process later */
- if (m->type == W1_LIST_MASTERS) {
- err = w1_process_command_root(msg, m, nsp->portid);
+ if (msg->type == W1_LIST_MASTERS) {
+ err = w1_process_command_root(cn, nsp->portid);
goto out_cont;
}
/* All following message types require additional data,
* check here before references are taken.
*/
- if (!m->len) {
+ if (!msg->len) {
err = -EPROTO;
goto out_cont;
}
- /* both search calls take reference counts */
- if (m->type == W1_MASTER_CMD) {
- dev = w1_search_master_id(m->id.mst.id);
- } else if (m->type == W1_SLAVE_CMD) {
- sl = w1_search_slave((struct w1_reg_num *)m->id.id);
+ /* both search calls take references */
+ if (msg->type == W1_MASTER_CMD) {
+ dev = w1_search_master_id(msg->id.mst.id);
+ } else if (msg->type == W1_SLAVE_CMD) {
+ sl = w1_search_slave((struct w1_reg_num *)msg->id.id);
if (sl)
dev = sl->master;
} else {
printk(KERN_NOTICE
- "%s: msg: %x.%x, wrong type: %u, len: %u.\n",
- __func__, msg->id.idx, msg->id.val,
- m->type, m->len);
+ "%s: cn: %x.%x, wrong type: %u, len: %u.\n",
+ __func__, cn->id.idx, cn->id.val,
+ msg->type, msg->len);
err = -EPROTO;
goto out_cont;
}
@@ -549,8 +698,8 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
atomic_inc(&block->refcnt);
node->async.cb = w1_process_cb;
node->block = block;
- node->m = (struct w1_netlink_msg *)((u8 *)&block->msg +
- (size_t)((u8 *)m - (u8 *)msg));
+ node->msg = (struct w1_netlink_msg *)((u8 *)&block->request_cn +
+ (size_t)((u8 *)msg - (u8 *)cn));
node->sl = sl;
node->dev = dev;
@@ -561,11 +710,15 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
++node;
out_cont:
+ /* Can't queue because that modifies block and another
+ * thread could be processing the messages by now and
+ * there isn't a lock, send directly.
+ */
if (err)
- w1_netlink_send_error(msg, m, NULL, nsp->portid, err);
- msg_len -= sizeof(struct w1_netlink_msg) + m->len;
- m = (struct w1_netlink_msg *)(((u8 *)m) +
- sizeof(struct w1_netlink_msg) + m->len);
+ w1_netlink_send_error(cn, msg, nsp->portid, err);
+ msg_len -= sizeof(struct w1_netlink_msg) + msg->len;
+ msg = (struct w1_netlink_msg *)(((u8 *)msg) +
+ sizeof(struct w1_netlink_msg) + msg->len);
/*
* Let's allow requests for nonexisting devices.
@@ -573,8 +726,8 @@ out_cont:
if (err == -ENODEV)
err = 0;
}
- if (block && atomic_sub_return(1, &block->refcnt) == 0)
- kfree(block);
+ if (block)
+ w1_unref_block(block);
}
int w1_init_netlink(void)
@@ -591,7 +744,7 @@ void w1_fini_netlink(void)
cn_del_callback(&w1_id);
}
#else
-void w1_netlink_send(struct w1_master *dev, struct w1_netlink_msg *msg)
+void w1_netlink_send(struct w1_master *dev, struct w1_netlink_msg *cn)
{
}
diff --git a/drivers/w1/w1_netlink.h b/drivers/w1/w1_netlink.h
index 1e9504e..c99a9ce 100644
--- a/drivers/w1/w1_netlink.h
+++ b/drivers/w1/w1_netlink.h
@@ -28,6 +28,17 @@
#include "w1.h"
/**
+ * enum w1_cn_msg_flags - bitfield flags for struct cn_msg.flags
+ *
+ * @W1_CN_BUNDLE: Request bundling replies into fewer messagse. Be prepared
+ * to handle multiple struct cn_msg, struct w1_netlink_msg, and
+ * struct w1_netlink_cmd in one packet.
+ */
+enum w1_cn_msg_flags {
+ W1_CN_BUNDLE = 1,
+};
+
+/**
* enum w1_netlink_message_types - message type
*
* @W1_SLAVE_ADD: notification that a slave device was added
@@ -49,6 +60,19 @@ enum w1_netlink_message_types {
W1_LIST_MASTERS,
};
+/**
+ * struct w1_netlink_msg - holds w1 message type, id, and result
+ *
+ * @type: one of enum w1_netlink_message_types
+ * @status: kernel feedback for success 0 or errno failure value
+ * @len: length of data following w1_netlink_msg
+ * @id: union holding master bus id (msg.id) and slave device id (id[8]).
+ * @data: start address of any following data
+ *
+ * The base message structure for w1 messages over netlink.
+ * The netlink connector data sequence is, struct nlmsghdr, struct cn_msg,
+ * then one or more struct w1_netlink_msg (each with optional data).
+ */
struct w1_netlink_msg
{
__u8 type;
@@ -66,6 +90,7 @@ struct w1_netlink_msg
/**
* enum w1_commands - commands available for master or slave operations
+ *
* @W1_CMD_READ: read len bytes
* @W1_CMD_WRITE: write len bytes
* @W1_CMD_SEARCH: initiate a standard search, returns only the slave
@@ -93,6 +118,17 @@ enum w1_commands {
W1_CMD_MAX
};
+/**
+ * struct w1_netlink_cmd - holds the command and data
+ *
+ * @cmd: one of enum w1_commands
+ * @res: reserved
+ * @len: length of data following w1_netlink_cmd
+ * @data: start address of any following data
+ *
+ * One or more struct w1_netlink_cmd is placed starting at w1_netlink_msg.data
+ * each with optional data.
+ */
struct w1_netlink_cmd
{
__u8 cmd;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] w1: fixes and bundling replies
2014-03-23 1:27 [RFC] w1: fixes and bundling replies David Fries
` (2 preceding siblings ...)
2014-03-23 1:27 ` [PATCH 3/3] w1: optional bundling of netlink kernel replies David Fries
@ 2014-04-07 14:13 ` David Fries
2014-04-08 19:56 ` Evgeniy Polyakov
3 siblings, 1 reply; 6+ messages in thread
From: David Fries @ 2014-04-07 14:13 UTC (permalink / raw)
To: linux-kernel; +Cc: Evgeniy Polyakov
Evgeniy,
Could you review this set of patches in this thread? Thanks.
On Sat, Mar 22, 2014 at 08:27:44PM -0500, David Fries wrote:
> On Fri, Feb 21, 2014 at 01:07:28AM +0400, Evgeniy Polyakov wrote:
> > Your approach and patch seem correct, but I worry about how old
> > commands are processed. Do I get it right, that replies to old
> > non-bundle commands are queued back instead of sending immediately,
> > but since it is not bundle but single command, queued reply is being
> > sent 'almost' immediately? Basically, nothing changed to old
> > clients, but there is new way to send batch requests now?
>
> Yes, with the previous patch set, if a client sent one command per
> message they would get the replies in individual packets. However in
> some cases clients had to bundle multiple commands in one message.
> For instance when using a slave command to read a temperature sensor
> it takes one W1_CMD_WRITE command to send a W1_READ_SCRATCHPAD, then
> another W1_CMD_READ command to read the data back and they have to be
> sent in the same W1_SLAVE_CMD because there can't be a reset between
> the two write and read, and you can't do that with a slave command.
> You can with master commands, but if you are going to issue individual
> reset, write, and read commands why would you not bundle them?
>
> Here is a new set of patches making bundling opt in.
>
> In this version of the patch, W1_CN_BUNDLE set in the cn_msg.flags
> enables the kernel to bundle the reply, otherwise replies aren't
> bundled.
>
> The previous patch separated the data replies and status replies
> because they have a different ack value which is in the cn_msg and
> cn_netlink_send could only send one cn_msg in a call. I didn't much
> like that solution because now status and data replies were out of
> order. This version creates cn_netlink_send_mult which takes a length
> which allows multiple cn_msg structures to be sent at once (inside one
> nlmsghdr, so clients can see there are more cn_msg structures to
> read), so now status and data messages can be sent in order.
>
> This is somewhat suboptimal as each command has a status reply and
> possibly a data reply, each requiring a different ack, so even if a
> client sent multiple commands in one w1_netlink_msg like,
>
> cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [read], w1_netlink_cmd [read]
>
> the response can't pack the commands into one w1_netlink_msg because
> of the ack, so there's duplicate of cn_msg and w1_netlink_msg
> structures,
>
> cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data]
> cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status]
> cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data]
> cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status]
>
> cn_msg is 20 bytes, w1_netlink_msg 12, so compared to sending separate
> packets with all the context switches, select overhead and such,
> bundling is going to be well worth it.
>
> Another alternative could be one cn_msg [seq+1] with the data, and
> followed by cn_msg [ack ] with the status messages, but they are back
> to out of order.
>
> I wasn't completely sure what to call the new sending function, I
> decided on mult for multiple cn_netlink_send_mult, cn_netlink_send_len
> as another idea, or if there are any better ideas let me know.
>
> I tested with my client program that will bundle up 14 temperature
> sensor conversions, then after a delay, bundle up another set of
> commands to read all 14 with the bundle bit set. I also tested with a
> two year old version of the software that sends requests two one slave
> at a time (bundling only the write/read to get the data out), and
> doesn't have code to read the bundling the this patch adds. Both
> operate correctly running at the same time.
>
> Posted before, no changes
>
> connector support for sending multiple cn_msg
>
> bundling,
> corrects ack value to previous ack or seq + 1
> corrects the variables to be name consistently
>
> Documentation/connector/connector.txt | 15 +-
> Documentation/w1/w1.generic | 2 +-
> Documentation/w1/w1.netlink | 13 +-
> drivers/connector/connector.c | 17 +-
> drivers/w1/w1.h | 8 -
> drivers/w1/w1_netlink.c | 673 ++++++++++++++++++++-------------
> drivers/w1/w1_netlink.h | 36 ++
> include/linux/connector.h | 1 +
> 8 files changed, 489 insertions(+), 276 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
David Fries <david@fries.net> PGP pub CB1EE8F0
http://fries.net/~david/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] w1: fixes and bundling replies
2014-04-07 14:13 ` [RFC] w1: fixes and bundling replies David Fries
@ 2014-04-08 19:56 ` Evgeniy Polyakov
0 siblings, 0 replies; 6+ messages in thread
From: Evgeniy Polyakov @ 2014-04-08 19:56 UTC (permalink / raw)
To: David Fries, linux-kernel@vger.kernel.org
Hi David
07.04.2014, 18:13, "David Fries" <David@Fries.net>:
> Evgeniy,
> Could you review this set of patches in this thread? Thanks.
I've checked it and it looks good, please resend it to lkml and Greg Kroah-Hartman, he will pull it into the tree.
Feel free to add my Acked-by.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-08 19:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-23 1:27 [RFC] w1: fixes and bundling replies David Fries
2014-03-23 1:27 ` [PATCH 1/3] w1: fix netlink refcnt leak on error path David Fries
2014-03-23 1:27 ` [PATCH 2/3] connector: allow multiple messages to be sent in one packet David Fries
2014-03-23 1:27 ` [PATCH 3/3] w1: optional bundling of netlink kernel replies David Fries
2014-04-07 14:13 ` [RFC] w1: fixes and bundling replies David Fries
2014-04-08 19:56 ` Evgeniy Polyakov
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).