[w1] userspace commands extensions.

[w1] userspace commands extensions.

[Evgeniy Polyakov]

Hi.

This small patchset extendes existing commands with reset, master IO and
status messages. Reset is used to reset the bus for given master device,
master IO command allows to initiate IO against bus itself not selecting
slave device first, which can be used to probe the device for example.
And status messages carry command completion status back to the
userspace (namely very useful to get -ENODEV from when requested device
was not found).

Great thanks to Paul Alfille <paul.alfille@gmail.com> of OWFS for
testing and commands suggestions.

[w1] Allow master IO commands.

[Evgeniy Polyakov]

This patch allows to start IO not against already found slave devices,
but against the bus itself, which can be used for example to probe device.

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 drivers/w1/w1_netlink.c |  126 ++++++++++++++++++++++++++---------------------
 1 files changed, 69 insertions(+), 57 deletions(-)

diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index 24ad6bb..0267fe4 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -95,51 +95,8 @@ static int w1_process_search_command(struct w1_master *dev, struct cn_msg *msg,
 	return 0;
 }
 
-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;
-	struct cn_msg *msg;
-	struct w1_netlink_msg *hdr;
-	struct w1_netlink_cmd *cmd;
-
-	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;
-
-	switch (cmd->cmd) {
-		case W1_CMD_SEARCH:
-		case W1_CMD_ALARM_SEARCH:
-			err = w1_process_search_command(dev, msg,
-				PAGE_SIZE - msg->len - sizeof(struct cn_msg));
-			break;
-		default:
-			cmd->res = EINVAL;
-			cn_netlink_send(msg, 0, GFP_KERNEL);
-			break;
-	}
-
-	kfree(msg);
-	return err;
-}
-
-static int w1_send_read_reply(struct w1_slave *sl, struct cn_msg *msg,
-		struct w1_netlink_msg *hdr, struct w1_netlink_cmd *cmd)
+static int w1_send_read_reply(struct cn_msg *msg, struct w1_netlink_msg *hdr,
+		struct w1_netlink_cmd *cmd)
 {
 	void *data;
 	struct w1_netlink_msg *h;
@@ -163,7 +120,8 @@ static int w1_send_read_reply(struct w1_slave *sl, struct cn_msg *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;
+	cm->len = sizeof(struct w1_netlink_msg) +
+		sizeof(struct w1_netlink_cmd) + cmd->len;
 
 	h->len = sizeof(struct w1_netlink_cmd) + cmd->len;
 
@@ -176,26 +134,22 @@ static int w1_send_read_reply(struct w1_slave *sl, struct cn_msg *msg,
 	return err;
 }
 
-static int w1_process_command_slave(struct w1_slave *sl, struct cn_msg *msg,
+static int w1_process_command_io(struct w1_master *dev, struct cn_msg *msg,
 		struct w1_netlink_msg *hdr, struct w1_netlink_cmd *cmd)
 {
 	int err = 0;
 
-	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);
-
 	switch (cmd->cmd) {
 		case W1_CMD_TOUCH:
-			w1_touch_block(sl->master, cmd->data, cmd->len);
-			w1_send_read_reply(sl, msg, hdr, cmd);
+			w1_touch_block(dev, cmd->data, cmd->len);
+			w1_send_read_reply(msg, hdr, cmd);
 			break;
 		case W1_CMD_READ:
-			w1_read_block(sl->master, cmd->data, cmd->len);
-			w1_send_read_reply(sl, msg, hdr, cmd);
+			w1_read_block(dev, cmd->data, cmd->len);
+			w1_send_read_reply(msg, hdr, cmd);
 			break;
 		case W1_CMD_WRITE:
-			w1_write_block(sl->master, cmd->data, cmd->len);
+			w1_write_block(dev, cmd->data, cmd->len);
 			break;
 		default:
 			err = -1;
@@ -205,6 +159,64 @@ static int w1_process_command_slave(struct w1_slave *sl, struct cn_msg *msg,
 	return err;
 }
 
+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;
+	struct cn_msg *msg;
+	struct w1_netlink_msg *hdr;
+	struct w1_netlink_cmd *cmd;
+
+	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;
+
+	switch (cmd->cmd) {
+		case W1_CMD_SEARCH:
+		case W1_CMD_ALARM_SEARCH:
+			err = w1_process_search_command(dev, msg,
+				PAGE_SIZE - msg->len - sizeof(struct cn_msg));
+			break;
+		case W1_CMD_READ:
+		case W1_CMD_WRITE:
+		case W1_CMD_TOUCH:
+			err = w1_process_command_io(dev, msg, hdr, cmd);
+			break;
+		default:
+			cmd->res = EINVAL;
+			cn_netlink_send(msg, 0, GFP_KERNEL);
+			break;
+	}
+
+	kfree(msg);
+	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)
+{
+	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);
+}
+
 static int w1_process_command_root(struct cn_msg *msg, struct w1_netlink_msg *mcmd)
 {
 	struct w1_master *m;
@@ -214,7 +226,7 @@ static int w1_process_command_root(struct cn_msg *msg, struct w1_netlink_msg *mc
 
 	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);
+			__func__, msg->id.idx, msg->id.val, mcmd->type, mcmd->len);
 		return -EPROTO;
 	}
 
-- 
1.5.6.3

[w1] Move w1 commands from defines to enum.

[Evgeniy Polyakov]

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 drivers/w1/w1_netlink.h |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/w1/w1_netlink.h b/drivers/w1/w1_netlink.h
index 99dd21b..01d86a7 100644
--- a/drivers/w1/w1_netlink.h
+++ b/drivers/w1/w1_netlink.h
@@ -52,11 +52,14 @@ struct w1_netlink_msg
 	__u8				data[0];
 };
 
-#define W1_CMD_READ		0x0
-#define W1_CMD_WRITE		0x1
-#define W1_CMD_SEARCH		0x2
-#define W1_CMD_ALARM_SEARCH	0x3
-#define W1_CMD_TOUCH		0x4
+enum w1_commands {
+	W1_CMD_READ = 0,
+	W1_CMD_WRITE,
+	W1_CMD_SEARCH,
+	W1_CMD_ALARM_SEARCH,
+	W1_CMD_TOUCH,
+	W1_CMD_MAX,
+};
 
 struct w1_netlink_cmd
 {
-- 
1.5.6.3

[w1] Added w1 reset command.

[Evgeniy Polyakov]

Command which allows to reset the bus.

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 Documentation/w1/w1.netlink |    6 ++++++
 drivers/w1/w1_netlink.c     |    3 +++
 drivers/w1/w1_netlink.h     |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/w1/w1.netlink b/Documentation/w1/w1.netlink
index 4560180..25721af 100644
--- a/Documentation/w1/w1.netlink
+++ b/Documentation/w1/w1.netlink
@@ -112,6 +112,12 @@ cn_msg->len = sizeof(struct w1_netlink_msg) +
 	      sizeof(struct w1_netlink_cmd) +
 	      N*8;
 
+W1 reset command.
+[cn_msg]
+  [w1_netlink_msg type = W1_MASTER_CMD
+  	id is equal to the bus master id to use for searching]
+  [w1_netlink_cmd cmd = W1_CMD_RESET]
+
 Operation steps in w1 core when new command is received.
 =======================================================
 
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index 0267fe4..e807265 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -197,6 +197,9 @@ static int w1_process_command_master(struct w1_master *dev, struct cn_msg *req_m
 		case W1_CMD_TOUCH:
 			err = w1_process_command_io(dev, msg, hdr, cmd);
 			break;
+		case W1_CMD_RESET:
+			err = w1_reset_bus(dev);
+			break;
 		default:
 			cmd->res = EINVAL;
 			cn_netlink_send(msg, 0, GFP_KERNEL);
diff --git a/drivers/w1/w1_netlink.h b/drivers/w1/w1_netlink.h
index 01d86a7..68a4ff4 100644
--- a/drivers/w1/w1_netlink.h
+++ b/drivers/w1/w1_netlink.h
@@ -58,6 +58,7 @@ enum w1_commands {
 	W1_CMD_SEARCH,
 	W1_CMD_ALARM_SEARCH,
 	W1_CMD_TOUCH,
+	W1_CMD_RESET,
 	W1_CMD_MAX,
 };
 
-- 
1.5.6.3

[w1] Send status messages after command processing.

[Evgeniy Polyakov]

Send completion status of the commands to the userspace.
Message and protocol are described in the documentation.

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 Documentation/w1/w1.netlink |   29 +++++++++++++++++++++++
 drivers/w1/w1_netlink.c     |   53 +++++++++++++++++++++++++++++++++++++-----
 drivers/w1/w1_netlink.h     |    2 +-
 3 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/Documentation/w1/w1.netlink b/Documentation/w1/w1.netlink
index 25721af..089e65e 100644
--- a/Documentation/w1/w1.netlink
+++ b/Documentation/w1/w1.netlink
@@ -118,6 +118,35 @@ W1 reset command.
   	id is equal to the bus master id to use for searching]
   [w1_netlink_cmd cmd = W1_CMD_RESET]
 
+
+Command status replies.
+======================
+
+Each command (either root, master or slave with or without w1_netlink_cmd
+structure) will be 'acked' by the w1 core. Format of the reply is the same
+as request message except that length parameters do not account for data
+requested by the user, i.e. read/write/touch IO requests will not contain
+data, so w1_netlink_cmd.len will be 0, w1_netlink_msg.len will be size
+of the w1_netlink_cmd structure and cn_msg.len will be equal to the sum
+of the sizeof(struct w1_netlink_msg) and sizeof(struct w1_netlink_cmd).
+If reply is generated for master or root command (which do not have
+w1_netlink_cmd attached), reply will contain only cn_msg and w1_netlink_msg
+structires.
+
+w1_netlink_msg.status field will carry positive error value
+(EINVAL for example) or zero in case of success.
+
+All other fields in every structure will mirror the same parameters in the
+request message (except lengths as described above).
+
+Status reply is generated for every w1_netlink_cmd embedded in the
+w1_netlink_msg, if there are no w1_netlink_cmd structures,
+reply will be generated for the w1_netlink_msg.
+
+All w1_netlink_cmd command structures are handled in every w1_netlink_msg,
+even if there were errors, only length mismatch interrupts message processing.
+
+
 Operation steps in w1 core when new command is received.
 =======================================================
 
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index e807265..7cb83ad 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -152,7 +152,7 @@ static int w1_process_command_io(struct w1_master *dev, struct cn_msg *msg,
 			w1_write_block(dev, cmd->data, cmd->len);
 			break;
 		default:
-			err = -1;
+			err = -EINVAL;
 			break;
 	}
 
@@ -195,14 +195,13 @@ static int w1_process_command_master(struct w1_master *dev, struct cn_msg *req_m
 		case W1_CMD_READ:
 		case W1_CMD_WRITE:
 		case W1_CMD_TOUCH:
-			err = w1_process_command_io(dev, msg, hdr, cmd);
+			err = w1_process_command_io(dev, req_msg, req_hdr, req_cmd);
 			break;
 		case W1_CMD_RESET:
 			err = w1_reset_bus(dev);
 			break;
 		default:
-			cmd->res = EINVAL;
-			cn_netlink_send(msg, 0, GFP_KERNEL);
+			err = -EINVAL;
 			break;
 	}
 
@@ -246,7 +245,7 @@ static int w1_process_command_root(struct cn_msg *msg, struct w1_netlink_msg *mc
 	w = (struct w1_netlink_msg *)(cn + 1);
 
 	w->type = W1_LIST_MASTERS;
-	w->reserved = 0;
+	w->status = 0;
 	w->len = 0;
 	id = (u32 *)(w + 1);
 
@@ -273,6 +272,40 @@ static int w1_process_command_root(struct cn_msg *msg, struct w1_netlink_msg *mc
 	return 0;
 }
 
+static int w1_netlink_send_error(struct cn_msg *rcmsg, struct w1_netlink_msg *rmsg,
+		struct w1_netlink_cmd *rcmd, 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, 0, GFP_KERNEL);
+	kfree(cmsg);
+
+	return error;
+}
+
 static void w1_cn_callback(void *data)
 {
 	struct cn_msg *msg = data;
@@ -289,6 +322,7 @@ static void w1_cn_callback(void *data)
 
 		dev = NULL;
 		sl = NULL;
+		cmd = NULL;
 
 		memcpy(&id, m->id.id, sizeof(id));
 #if 0
@@ -336,9 +370,12 @@ static void w1_cn_callback(void *data)
 			}
 
 			if (sl)
-				w1_process_command_slave(sl, msg, m, cmd);
+				err = w1_process_command_slave(sl, msg, m, cmd);
 			else
-				w1_process_command_master(dev, msg, m, cmd);
+				err = w1_process_command_master(dev, msg, m, cmd);
+
+			w1_netlink_send_error(msg, m, cmd, err);
+			err = 0;
 
 			cmd_data += cmd->len + sizeof(struct w1_netlink_cmd);
 			mlen -= cmd->len + sizeof(struct w1_netlink_cmd);
@@ -349,6 +386,8 @@ out_up:
 			atomic_dec(&sl->refcnt);
 		mutex_unlock(&dev->mutex);
 out_cont:
+		if (!cmd || err)
+			w1_netlink_send_error(msg, m, cmd, err);
 		msg->len -= sizeof(struct w1_netlink_msg) + m->len;
 		m = (struct w1_netlink_msg *)(((u8 *)m) + sizeof(struct w1_netlink_msg) + m->len);
 
diff --git a/drivers/w1/w1_netlink.h b/drivers/w1/w1_netlink.h
index 68a4ff4..27e950f 100644
--- a/drivers/w1/w1_netlink.h
+++ b/drivers/w1/w1_netlink.h
@@ -40,7 +40,7 @@ enum w1_netlink_message_types {
 struct w1_netlink_msg
 {
 	__u8				type;
-	__u8				reserved;
+	__u8				status;
 	__u16				len;
 	union {
 		__u8			id[8];
-- 
1.5.6.3

Re: [w1] Allow master IO commands.

[Andrew Morton]

On Tue, 16 Dec 2008 21:07:59 +0300
Evgeniy Polyakov <zbr@ioremap.net> wrote:

> This patch allows to start IO not against already found slave devices,
> but against the bus itself, which can be used for example to probe device.
> 
> ...
>
> +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;
> +	struct cn_msg *msg;
> +	struct w1_netlink_msg *hdr;
> +	struct w1_netlink_cmd *cmd;
> +
> +	msg = kzalloc(PAGE_SIZE, GFP_KERNEL);

The use of PAGE_SIZE is odd.  It's either too large (inefficient) or
too small (disaster).

Is it not practical to calculate this precisely?

> +	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;
> +
> +	switch (cmd->cmd) {
> +		case W1_CMD_SEARCH:

checkpatch correctly points out that the body of the switch statement
should be indented one tabstop less than this.

> +		case W1_CMD_ALARM_SEARCH:
> +			err = w1_process_search_command(dev, msg,
> +				PAGE_SIZE - msg->len - sizeof(struct cn_msg));

which would give more room for cleaning this up.

> +			break;
> +		case W1_CMD_READ:
> +		case W1_CMD_WRITE:
> +		case W1_CMD_TOUCH:
> +			err = w1_process_command_io(dev, msg, hdr, cmd);
> +			break;
> +		default:
> +			cmd->res = EINVAL;
> +			cn_netlink_send(msg, 0, GFP_KERNEL);
> +			break;
> +	}
> +
> +	kfree(msg);
> +	return err;
> +}
> +

Re: [w1] Allow master IO commands.

[Evgeniy Polyakov]

Hi Andrew.

On Wed, Dec 17, 2008 at 02:22:41PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> > +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;
> > +	struct cn_msg *msg;
> > +	struct w1_netlink_msg *hdr;
> > +	struct w1_netlink_cmd *cmd;
> > +
> > +	msg = kzalloc(PAGE_SIZE, GFP_KERNEL);
> 
> The use of PAGE_SIZE is odd.  It's either too large (inefficient) or
> too small (disaster).
> 
> Is it not practical to calculate this precisely?

It is not possible to calculate it in advance for the search commands,
since we do not know how many slave devices are attached to the bus.
This buffer is used as temporal area where IDs are stored and when
number of IDs is large enough message is emmitted to the userspace and
buffer becomes empty again. Sending one message per slave ID is a too
big overhead, so this could be half of a page or one third or whatever
else, I selected a page, which is the maximum guaranteed allocation
buffer size.

> > +	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;
> > +
> > +	switch (cmd->cmd) {
> > +		case W1_CMD_SEARCH:
> 
> checkpatch correctly points out that the body of the switch statement
> should be indented one tabstop less than this.
> 
> > +		case W1_CMD_ALARM_SEARCH:
> > +			err = w1_process_search_command(dev, msg,
> > +				PAGE_SIZE - msg->len - sizeof(struct cn_msg));
> 
> which would give more room for cleaning this up.

Ok, I will space it to the left. Should I sent patch on top of it or
instead of?

-- 
	Evgeniy Polyakov

Re: [w1] Allow master IO commands.

[Andrew Morton]

On Thu, 18 Dec 2008 01:33:52 +0300
Evgeniy Polyakov <zbr@ioremap.net> wrote:

> Hi Andrew.
> 
> On Wed, Dec 17, 2008 at 02:22:41PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > +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;
> > > +	struct cn_msg *msg;
> > > +	struct w1_netlink_msg *hdr;
> > > +	struct w1_netlink_cmd *cmd;
> > > +
> > > +	msg = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > 
> > The use of PAGE_SIZE is odd.  It's either too large (inefficient) or
> > too small (disaster).
> > 
> > Is it not practical to calculate this precisely?
> 
> It is not possible to calculate it in advance for the search commands,
> since we do not know how many slave devices are attached to the bus.
> This buffer is used as temporal area where IDs are stored and when
> number of IDs is large enough message is emmitted to the userspace and
> buffer becomes empty again. Sending one message per slave ID is a too
> big overhead, so this could be half of a page or one third or whatever
> else, I selected a page, which is the maximum guaranteed allocation
> buffer size.

OK.

Do we have checks in there to ensure that we can't run off the end of
the buffer?

> > > +	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;
> > > +
> > > +	switch (cmd->cmd) {
> > > +		case W1_CMD_SEARCH:
> > 
> > checkpatch correctly points out that the body of the switch statement
> > should be indented one tabstop less than this.
> > 
> > > +		case W1_CMD_ALARM_SEARCH:
> > > +			err = w1_process_search_command(dev, msg,
> > > +				PAGE_SIZE - msg->len - sizeof(struct cn_msg));
> > 
> > which would give more room for cleaning this up.
> 
> Ok, I will space it to the left. Should I sent patch on top of it or
> instead of?

Is OK, I'll fix it.  That change messes up the following patches so I
fixed them as well.

Re: [w1] Allow master IO commands.

[Evgeniy Polyakov]

On Wed, Dec 17, 2008 at 03:14:18PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> Do we have checks in there to ensure that we can't run off the end of
> the buffer?

Yes, w1_process_search_command() calls w1_search_devices() with the
w1_send_slave() callback which is invoked each time new slave is
detected. w1_send_slave() checks if amount of available data is enough
to store additional ID and sends message otherwise. The last message is
being sent with the flag indicating that, and it is sent no matter if
there were some slave devices found or not. 

> > Ok, I will space it to the left. Should I sent patch on top of it or
> > instead of?
> 
> Is OK, I'll fix it.  That change messes up the following patches so I
> fixed them as well.

Thanks a lot Andrew!

-- 
	Evgeniy Polyakov