Re: [PATCH 0/4] w1: refcnt fix, skip non-error send, docs

[David Fries <david@fries.net>]

On Wed, Feb 05, 2014 at 03:48:45AM +0400, zbr@ioremap.net wrote:
> Hi
> 
> 04.02.2014, 09:51, "David Fries" <david@fries.net>:
> > Help me understand what the protocol is supposed to be.  Assuming
> > there aren't any errors, is there supposed to be a
> > w1_netlink_send_error generated reply per netlink packet (cn_msg), per
> > w1_netlink_msg, or per w1_netlink_cmd?
> 
> reply should be sent per cmd to specify each command status
> If there is no cmd in request or we didn't get to it (like failed to reset device), we should send error.
> 
> Depending on how w1-msg + (optional) w1-cmd are packed, client can detect what exact error happend
> 
> > What about the cn_msg seq and ack values?  I assume the kernel
> > response should carry the same seq number as the request, but what
> > should the ack be set to?
> 
> reply ack is seq + 1
> seq is the same to highlight request it belongs to

Here's a patch to implement that.  Is this what you have in mind?

>From 4ed65d81b0121a8c191a9833d041484e9097198b Mon Sep 17 00:00:00 2001
From: David Fries <David@Fries.net>
Date: Thu, 6 Feb 2014 23:45:05 -0600
Subject: [PATCH] w1: correct cn_msg ack, no change or seq + 1

Netlink messages sent from the kernel consists of kernel generated
notifications for adds or removes, the error message (also indicates
the message has been processed), and the messages that have data to
return.  The cn_msg ack is left alone for the first two, and when
returning data it is the sequence number + 1.  Modifying the code to
the protocol standard.

Signed-off-by: David Fries <David@Fries.net>
---
 Documentation/connector/connector.txt |    2 +-
 drivers/w1/w1_netlink.c               |    6 +-----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/Documentation/connector/connector.txt b/Documentation/connector/connector.txt
index 9bdfc1a..0bc3522 100644
--- a/Documentation/connector/connector.txt
+++ b/Documentation/connector/connector.txt
@@ -115,7 +115,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/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index 3c81689..d98b550 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -59,7 +59,6 @@ static void w1_send_slave(struct w1_master *dev, u64 rn)
 	avail = dev->priv_size - cmd->len;
 
 	if (avail < 8) {
-		msg->ack++;
 		cn_netlink_send(msg, dev->portid, 0, GFP_KERNEL);
 
 		msg->len = sizeof(struct w1_netlink_msg) +
@@ -130,7 +129,6 @@ static int w1_get_slaves(struct w1_master *dev,
 			W1_ALARM_SEARCH : W1_SEARCH, w1_found_send_slave);
 	}
 
-	msg->ack = 0;
 	cn_netlink_send(msg, dev->portid, 0, GFP_KERNEL);
 
 	dev->priv = NULL;
@@ -308,7 +306,7 @@ static int w1_process_command_root(struct cn_msg *msg,
 	cn->id.val = CN_W1_VAL;
 
 	cn->seq = msg->seq;
-	cn->ack = 1;
+	cn->ack = msg->seq + 1;
 	cn->len = sizeof(struct w1_netlink_msg);
 	w = (struct w1_netlink_msg *)(cn + 1);
 
@@ -321,7 +319,6 @@ static int w1_process_command_root(struct cn_msg *msg,
 	list_for_each_entry(m, &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);
@@ -332,7 +329,6 @@ static int w1_process_command_root(struct cn_msg *msg,
 		cn->len += sizeof(*id);
 		id++;
 	}
-	cn->ack = 0;
 	cn_netlink_send(cn, portid, 0, GFP_KERNEL);
 	mutex_unlock(&w1_mlock);
 
-- 
1.7.10.4