[PATCH 09/14] greybus: cpc: acknowledge all incoming messages

Damien Riégel posted 14 patches 2 days, 8 hours ago
There is a newer version of this series
[PATCH 09/14] greybus: cpc: acknowledge all incoming messages
Posted by Damien Riégel 2 days, 8 hours ago
Currently, CPC doesn't send messages on its own, it only prepends its
header to outgoing messages. This can lead to messages not being
acknowledged, for instance in the case of an SVC Ping

	Host				Device

  SVC Ping (seq=X, ack=Y)
				  SVC Ping Reply (seq=Y, ack=X+1)

The "Ping Reply" is never acknowledged at the CPC level, which can lead
to retransmissions, or worst the device might think the link is broken
and do something to recover.

To prevent that scenario, an ack mechanism is implemented in the most
straightforward manner: send an ACK to all incoming messages. Here, two
flags need to be added:
 - First, an ACK frame should not be passed to the Greybus layer, so a
   "CONTROL" flag is added. If this flag is set, it means it's a control
   messages and should stay at the CPC level. Currently there is only
   one type of control frame, the standalone ack. Control messages have
   the same format as Greybus operations.
 - Second, ack themselves should not be acked, so to determine if a
   message should be acked or not, a REQUEST_ACK flag is added.

Signed-off-by: Damien Riégel <damien.riegel@silabs.com>
---
 drivers/greybus/cpc/cpc.h      |  3 ++
 drivers/greybus/cpc/cport.c    |  1 +
 drivers/greybus/cpc/header.c   | 41 +++++++++++++++++++++++++
 drivers/greybus/cpc/header.h   |  3 ++
 drivers/greybus/cpc/protocol.c | 55 +++++++++++++++++++++++++++++-----
 5 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/drivers/greybus/cpc/cpc.h b/drivers/greybus/cpc/cpc.h
index 87b54a4fd34..725fd7f4afc 100644
--- a/drivers/greybus/cpc/cpc.h
+++ b/drivers/greybus/cpc/cpc.h
@@ -51,6 +51,9 @@ struct cpc_skb_cb {
 	struct gb_message *gb_message;
 
 	u8 seq;
+
+#define CPC_SKB_FLAG_REQ_ACK (1 << 0)
+	u8 cpc_flags;
 };
 
 #define CPC_SKB_CB(__skb) ((struct cpc_skb_cb *)&((__skb)->cb[0]))
diff --git a/drivers/greybus/cpc/cport.c b/drivers/greybus/cpc/cport.c
index 2ee0b129996..35a148abbed 100644
--- a/drivers/greybus/cpc/cport.c
+++ b/drivers/greybus/cpc/cport.c
@@ -86,6 +86,7 @@ int cpc_cport_transmit(struct cpc_cport *cport, struct sk_buff *skb)
 	mutex_lock(&cport->lock);
 
 	CPC_SKB_CB(skb)->seq = cport->tcb.seq;
+	CPC_SKB_CB(skb)->cpc_flags = CPC_SKB_FLAG_REQ_ACK;
 
 	cport->tcb.seq++;
 	ack = cport->tcb.ack;
diff --git a/drivers/greybus/cpc/header.c b/drivers/greybus/cpc/header.c
index 62946d6077e..8875a6fed26 100644
--- a/drivers/greybus/cpc/header.c
+++ b/drivers/greybus/cpc/header.c
@@ -3,8 +3,25 @@
  * Copyright (c) 2025, Silicon Laboratories, Inc.
  */
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+
 #include "header.h"
 
+#define CPC_HEADER_CONTROL_IS_CONTROL_MASK BIT(7)
+#define CPC_HEADER_CONTROL_REQ_ACK_MASK BIT(6)
+
+/**
+ * cpc_header_is_control() - Identify if this is a control frame.
+ * @hdr: CPC header.
+ *
+ * Return: True if this is a control frame, false if this a Greybus frame.
+ */
+bool cpc_header_is_control(const struct cpc_header *hdr)
+{
+	return hdr->ctrl_flags & CPC_HEADER_CONTROL_IS_CONTROL_MASK;
+}
+
 /**
  * cpc_header_get_seq() - Get the sequence number.
  * @hdr: CPC header.
@@ -15,3 +32,27 @@ u8 cpc_header_get_seq(const struct cpc_header *hdr)
 {
 	return hdr->seq;
 }
+
+/**
+ * cpc_header_get_req_ack() - Get the request acknowledge frame flag.
+ * @hdr: CPC header.
+ *
+ * Return: Request acknowledge frame flag.
+ */
+bool cpc_header_get_req_ack(const struct cpc_header *hdr)
+{
+	return FIELD_GET(CPC_HEADER_CONTROL_REQ_ACK_MASK, hdr->ctrl_flags);
+}
+
+/**
+ * cpc_header_encode_ctrl_flags() - Encode parameters into the control byte.
+ * @control: True if CPC control frame, false if Greybus frame.
+ * @req_ack: Frame flag indicating a request to be acknowledged.
+ *
+ * Return: Encoded control byte.
+ */
+u8 cpc_header_encode_ctrl_flags(bool control, bool req_ack)
+{
+	return FIELD_PREP(CPC_HEADER_CONTROL_IS_CONTROL_MASK, control) |
+	       FIELD_PREP(CPC_HEADER_CONTROL_REQ_ACK_MASK, req_ack);
+}
diff --git a/drivers/greybus/cpc/header.h b/drivers/greybus/cpc/header.h
index 2a64aa8d278..0c9f6e56524 100644
--- a/drivers/greybus/cpc/header.h
+++ b/drivers/greybus/cpc/header.h
@@ -41,6 +41,9 @@ struct cpc_header {
 #define CPC_HEADER_SIZE (sizeof(struct cpc_header))
 #define GREYBUS_HEADER_SIZE (sizeof(struct gb_operation_msg_hdr))
 
+bool cpc_header_is_control(const struct cpc_header *hdr);
 u8 cpc_header_get_seq(const struct cpc_header *hdr);
+bool cpc_header_get_req_ack(const struct cpc_header *hdr);
+u8 cpc_header_encode_ctrl_flags(bool control, bool req_ack);
 
 #endif
diff --git a/drivers/greybus/cpc/protocol.c b/drivers/greybus/cpc/protocol.c
index 037910e899f..b4dd4e173a1 100644
--- a/drivers/greybus/cpc/protocol.c
+++ b/drivers/greybus/cpc/protocol.c
@@ -9,6 +9,11 @@
 #include "header.h"
 #include "host.h"
 
+static bool cpc_skb_is_sequenced(struct sk_buff *skb)
+{
+	return CPC_SKB_CB(skb)->cpc_flags & CPC_SKB_FLAG_REQ_ACK;
+}
+
 void cpc_protocol_prepare_header(struct sk_buff *skb, u8 ack)
 {
 	struct cpc_header *hdr;
@@ -18,28 +23,62 @@ void cpc_protocol_prepare_header(struct sk_buff *skb, u8 ack)
 	hdr = (struct cpc_header *)skb->data;
 	hdr->ack = ack;
 	hdr->recv_wnd = 0;
-	hdr->ctrl_flags = 0;
 	hdr->seq = CPC_SKB_CB(skb)->seq;
+	hdr->ctrl_flags = cpc_header_encode_ctrl_flags(!CPC_SKB_CB(skb)->gb_message,
+						       cpc_skb_is_sequenced(skb));
+}
+
+static void cpc_protocol_queue_ack(struct cpc_cport *cport, u8 ack)
+{
+	struct gb_operation_msg_hdr *gb_hdr;
+	struct sk_buff *skb;
+
+	skb = alloc_skb(CPC_HEADER_SIZE + sizeof(*gb_hdr), GFP_KERNEL);
+	if (!skb)
+		return;
+
+	skb_reserve(skb, CPC_HEADER_SIZE);
+
+	gb_hdr = skb_put(skb, sizeof(*gb_hdr));
+	memset(gb_hdr, 0, sizeof(*gb_hdr));
+
+	/* In the CPC Operation Header, only the size and cport_id matter for ACKs. */
+	gb_hdr->size = sizeof(*gb_hdr);
+	cpc_cport_pack(gb_hdr, cport->id);
+
+	cpc_protocol_prepare_header(skb, ack);
+
+	cpc_hd_send_skb(cport->cpc_hd, skb);
 }
 
 void cpc_protocol_on_data(struct cpc_cport *cport, struct sk_buff *skb)
 {
 	struct cpc_header *cpc_hdr = (struct cpc_header *)skb->data;
+	bool require_ack = cpc_header_get_req_ack(cpc_hdr);
 	u8 seq = cpc_header_get_seq(cpc_hdr);
 	bool expected_seq = false;
+	u8 ack;
 
 	mutex_lock(&cport->lock);
 
-	expected_seq = seq == cport->tcb.ack;
-	if (expected_seq)
-		cport->tcb.ack++;
-	else
-		dev_warn(cpc_hd_dev(cport->cpc_hd), "unexpected seq: %u, expected seq: %u\n", seq,
-			 cport->tcb.ack);
+	if (require_ack) {
+		expected_seq = seq == cport->tcb.ack;
+		if (expected_seq)
+			cport->tcb.ack++;
+		else
+			dev_warn(cpc_hd_dev(cport->cpc_hd),
+				 "unexpected seq: %u, expected seq: %u\n", seq, cport->tcb.ack);
+	}
+
+	ack = cport->tcb.ack;
 
 	mutex_unlock(&cport->lock);
 
-	if (expected_seq) {
+	/* Ack no matter if the sequence was valid or not, to resync with remote */
+	if (require_ack)
+		cpc_protocol_queue_ack(cport, ack);
+
+	if (expected_seq && !cpc_header_is_control(cpc_hdr)) {
 		skb_pull(skb, CPC_HEADER_SIZE);
 
 		greybus_data_rcvd(cport->cpc_hd->gb_hd, cport->id, skb->data, skb->len);
-- 
2.49.0

Re: [PATCH 09/14] greybus: cpc: acknowledge all incoming messages
Posted by kernel test robot 1 day ago
Hi Damien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.18 next-20251212]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Ri-gel/greybus-cpc-add-minimal-CPC-Host-Device-infrastructure/20251213-010308
base:   linus/master
patch link:    https://lore.kernel.org/r/20251212161308.25678-10-damien.riegel%40silabs.com
patch subject: [PATCH 09/14] greybus: cpc: acknowledge all incoming messages
config: arc-randconfig-r122-20251213 (https://download.01.org/0day-ci/archive/20251214/202512140750.0RkH4TN6-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 14.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251214/202512140750.0RkH4TN6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512140750.0RkH4TN6-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/greybus/cpc/protocol.c:46:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] size @@     got unsigned int @@
   drivers/greybus/cpc/protocol.c:46:22: sparse:     expected restricted __le16 [usertype] size
   drivers/greybus/cpc/protocol.c:46:22: sparse:     got unsigned int

vim +46 drivers/greybus/cpc/protocol.c

    30	
    31	static void cpc_protocol_queue_ack(struct cpc_cport *cport, u8 ack)
    32	{
    33		struct gb_operation_msg_hdr *gb_hdr;
    34		struct sk_buff *skb;
    35	
    36		skb = alloc_skb(CPC_HEADER_SIZE + sizeof(*gb_hdr), GFP_KERNEL);
    37		if (!skb)
    38			return;
    39	
    40		skb_reserve(skb, CPC_HEADER_SIZE);
    41	
    42		gb_hdr = skb_put(skb, sizeof(*gb_hdr));
    43		memset(gb_hdr, 0, sizeof(*gb_hdr));
    44	
    45		/* In the CPC Operation Header, only the size and cport_id matter for ACKs. */
  > 46		gb_hdr->size = sizeof(*gb_hdr);
    47		cpc_cport_pack(gb_hdr, cport->id);
    48	
    49		cpc_protocol_prepare_header(skb, ack);
    50	
    51		cpc_hd_send_skb(cport->cpc_hd, skb);
    52	}
    53	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 09/14] greybus: cpc: acknowledge all incoming messages
Posted by kernel test robot 1 day, 9 hours ago
Hi Damien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.18 next-20251212]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Ri-gel/greybus-cpc-add-minimal-CPC-Host-Device-infrastructure/20251213-010308
base:   linus/master
patch link:    https://lore.kernel.org/r/20251212161308.25678-10-damien.riegel%40silabs.com
patch subject: [PATCH 09/14] greybus: cpc: acknowledge all incoming messages
config: alpha-randconfig-r112-20251213 (https://download.01.org/0day-ci/archive/20251213/202512132212.2zg1V6JU-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 9.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251213/202512132212.2zg1V6JU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512132212.2zg1V6JU-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/greybus/cpc/protocol.c:46:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] size @@     got unsigned long @@
   drivers/greybus/cpc/protocol.c:46:22: sparse:     expected restricted __le16 [usertype] size
   drivers/greybus/cpc/protocol.c:46:22: sparse:     got unsigned long

vim +46 drivers/greybus/cpc/protocol.c

    30	
    31	static void cpc_protocol_queue_ack(struct cpc_cport *cport, u8 ack)
    32	{
    33		struct gb_operation_msg_hdr *gb_hdr;
    34		struct sk_buff *skb;
    35	
    36		skb = alloc_skb(CPC_HEADER_SIZE + sizeof(*gb_hdr), GFP_KERNEL);
    37		if (!skb)
    38			return;
    39	
    40		skb_reserve(skb, CPC_HEADER_SIZE);
    41	
    42		gb_hdr = skb_put(skb, sizeof(*gb_hdr));
    43		memset(gb_hdr, 0, sizeof(*gb_hdr));
    44	
    45		/* In the CPC Operation Header, only the size and cport_id matter for ACKs. */
  > 46		gb_hdr->size = sizeof(*gb_hdr);
    47		cpc_cport_pack(gb_hdr, cport->id);
    48	
    49		cpc_protocol_prepare_header(skb, ack);
    50	
    51		cpc_hd_send_skb(cport->cpc_hd, skb);
    52	}
    53	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki