From: Gabriel Beaulieu <gabriel.beaulieu@silabs.com>
This introduces a new module gb-cpc-sdio, in order to communicate with a
Greybus CPC device over SDIO.
Most of the complexity stems from aggregation: packets are aggregated to
minimize the number of CMD53s. In the first block, the first le32 is the
number of packets in this transfer. Immediately after that are all the
packet headers (CPC + Greybus). This lets the device process all the
headers in a single interrupt, and prepare the ADMA descriptors for all
the payloads in one go.
Payloads start at the beginning of the second block and are concatained.
Their lengths must be 32-bit aligned, so the driver takes care of adding
and removing padding if necessary.
Signed-off-by: Gabriel Beaulieu <gabriel.beaulieu@silabs.com>
Signed-off-by: Damien Riégel <damien.riegel@silabs.com>
---
drivers/greybus/cpc/Kconfig | 12 +
drivers/greybus/cpc/Makefile | 3 +
drivers/greybus/cpc/sdio.c | 554 +++++++++++++++++++++++++++++++++++
3 files changed, 569 insertions(+)
create mode 100644 drivers/greybus/cpc/sdio.c
diff --git a/drivers/greybus/cpc/Kconfig b/drivers/greybus/cpc/Kconfig
index ab96fedd0de..8223f56795f 100644
--- a/drivers/greybus/cpc/Kconfig
+++ b/drivers/greybus/cpc/Kconfig
@@ -8,3 +8,15 @@ config GREYBUS_CPC
To compile this code as a module, chose M here: the module will be
called gb-cpc.ko
+
+config GREYBUS_CPC_SDIO
+ tristate "Greybus CPC over SDIO"
+ depends on GREYBUS_CPC && MMC
+ help
+ This driver provides Greybus CPC host support for devices
+ connected via SDIO interface.
+
+ To compile this driver as a module, choose M here: the module
+ will be called gb-cpc-sdio.
+
+ If unsure, say N.
diff --git a/drivers/greybus/cpc/Makefile b/drivers/greybus/cpc/Makefile
index c4b530d27a3..3296536e86d 100644
--- a/drivers/greybus/cpc/Makefile
+++ b/drivers/greybus/cpc/Makefile
@@ -4,3 +4,6 @@ gb-cpc-y := cport.o header.o host.o protocol.o
# CPC core
obj-$(CONFIG_GREYBUS_CPC) += gb-cpc.o
+
+gb-cpc-sdio-y := sdio.o
+obj-$(CONFIG_GREYBUS_CPC_SDIO) += gb-cpc-sdio.o
diff --git a/drivers/greybus/cpc/sdio.c b/drivers/greybus/cpc/sdio.c
new file mode 100644
index 00000000000..5c467b83ad9
--- /dev/null
+++ b/drivers/greybus/cpc/sdio.c
@@ -0,0 +1,554 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025, Silicon Laboratories, Inc.
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitops.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kthread.h>
+#include <linux/minmax.h>
+#include <linux/mmc/sdio_func.h>
+#include <linux/mmc/sdio_ids.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include "cpc.h"
+#include "header.h"
+#include "host.h"
+
+#define GB_CPC_SDIO_MSG_SIZE_MAX 4096
+#define GB_CPC_SDIO_BLOCK_SIZE 256U
+#define GB_CPC_SDIO_FIFO_ADDR 0
+#define GB_CPC_SDIO_ALIGNMENT 4
+#define GB_CPC_SDIO_DEFAULT_AGGREGATION 1
+#define CPC_FRAME_HEADER_SIZE (CPC_HEADER_SIZE + GREYBUS_HEADER_SIZE)
+#define GB_CPC_SDIO_MAX_AGGREGATION ((GB_CPC_SDIO_BLOCK_SIZE - sizeof(u32)) / CPC_FRAME_HEADER_SIZE)
+
+enum cpc_sdio_flags {
+ CPC_SDIO_FLAG_IRQ_RUNNING,
+ CPC_SDIO_FLAG_TX_WORK_DELAYED,
+ CPC_SDIO_FLAG_SHUTDOWN,
+};
+
+struct cpc_sdio {
+ struct cpc_host_device *cpc_hd;
+ struct device *dev;
+ struct sdio_func *func;
+
+ struct work_struct tx_work;
+ unsigned long flags;
+
+ wait_queue_head_t event_queue;
+ u8 max_aggregation;
+};
+
+struct frame_header {
+ struct cpc_header cpc;
+ struct gb_operation_msg_hdr gb;
+} __packed;
+
+static inline struct cpc_sdio *cpc_hd_to_cpc_sdio(struct cpc_host_device *cpc_hd)
+{
+ return (struct cpc_sdio *)cpc_hd->priv;
+}
+
+static int gb_cpc_sdio_wake_tx(struct cpc_host_device *cpc_hd)
+{
+ struct cpc_sdio *ctx = cpc_hd_to_cpc_sdio(cpc_hd);
+
+ if (test_bit(CPC_SDIO_FLAG_SHUTDOWN, &ctx->flags))
+ return 0;
+
+ /* Use system workqueue for TX processing */
+ schedule_work(&ctx->tx_work);
+
+ return 0;
+}
+
+/**
+ * Return the memory requirement in bytes for the aggregated frame aligned to the block size
+ */
+static size_t cpc_sdio_get_aligned_size(struct cpc_sdio *ctx, struct sk_buff_head *frame_list)
+{
+ size_t size = 0;
+ struct sk_buff *frame;
+
+ /* Calculate total payload size */
+ skb_queue_walk(frame_list, frame) {
+ size_t payload_len = frame->len - CPC_FRAME_HEADER_SIZE;
+
+ /* payload is aligned and padded to 4 bytes */
+ size += ALIGN(payload_len, GB_CPC_SDIO_ALIGNMENT);
+ }
+
+ /* Make sure the total payload size is a round number of blocks */
+ size = ALIGN(size, GB_CPC_SDIO_BLOCK_SIZE);
+
+ /* Add an additional block for headers */
+ size += GB_CPC_SDIO_BLOCK_SIZE;
+
+ return size;
+}
+
+static unsigned char *cpc_sdio_build_aggregated_frame(struct cpc_sdio *ctx,
+ struct sk_buff_head *frame_list,
+ size_t *xfer_len)
+{
+ unsigned char *tx_buff;
+ struct sk_buff *frame;
+ __le32 *frame_count;
+ size_t xfer_size;
+ unsigned int i = 0;
+
+ xfer_size = cpc_sdio_get_aligned_size(ctx, frame_list);
+
+ /* Allocate aggregated frame */
+ tx_buff = kmalloc(xfer_size, GFP_KERNEL);
+ if (!tx_buff)
+ return NULL;
+
+ frame_count = (__le32 *)tx_buff;
+ *frame_count = cpu_to_le32(skb_queue_len(frame_list));
+ i += 4;
+
+ /* Copy frame headers to aggregate buffer */
+ skb_queue_walk(frame_list, frame) {
+ memcpy(&tx_buff[i], frame->data, CPC_FRAME_HEADER_SIZE);
+ i += CPC_FRAME_HEADER_SIZE;
+ }
+
+ /* Zero-pad remainder of header block to fill complete SDIO block */
+ if (i < GB_CPC_SDIO_BLOCK_SIZE)
+ memset(&tx_buff[i], 0, GB_CPC_SDIO_BLOCK_SIZE - i);
+
+ /* Start injecting payload at beginning of second block */
+ i = GB_CPC_SDIO_BLOCK_SIZE;
+
+ /* Build payload blocks if required */
+ if (xfer_size > GB_CPC_SDIO_BLOCK_SIZE) {
+ skb_queue_walk(frame_list, frame) {
+ size_t payload_len, padding_len;
+
+ if (frame->len <= CPC_FRAME_HEADER_SIZE)
+ continue;
+
+ payload_len = frame->len - CPC_FRAME_HEADER_SIZE;
+ memcpy(&tx_buff[i], &frame->data[CPC_FRAME_HEADER_SIZE], payload_len);
+ i += payload_len;
+
+ padding_len = ALIGN(payload_len, GB_CPC_SDIO_ALIGNMENT) - payload_len;
+ if (padding_len) {
+ memset(&tx_buff[i], 0, padding_len);
+ i += padding_len;
+ }
+ }
+ }
+
+ *xfer_len = xfer_size;
+
+ return tx_buff;
+}
+
+static bool cpc_sdio_get_payload_size(struct cpc_sdio *ctx, const struct frame_header *header,
+ size_t *payload_size)
+{
+ size_t gb_size;
+
+ gb_size = le16_to_cpu(header->gb.size);
+
+ /* Validate that the size is at least as large as the Greybus header */
+ if (gb_size < GREYBUS_HEADER_SIZE) {
+ dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
+ return false;
+ }
+
+ /* Validate maximum size */
+ if (gb_size > (GB_CPC_SDIO_MSG_SIZE_MAX + GREYBUS_HEADER_SIZE)) {
+ dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
+ return false;
+ }
+
+ /* Size includes the Greybus header, so subtract it to get payload size */
+ *payload_size = gb_size - GREYBUS_HEADER_SIZE;
+
+ return true;
+}
+
+/**
+ * Process aggregated frame
+ * Reconstructed frame layout:
+ * +-----+-----+-----+------+------+------+------+-------+---------+
+ * | CPC Header (4B) | Size | OpID | Type | Stat | CPort | Payload |
+ * +-----+-----+-----+------+------+------+------+-------+---------+
+ */
+static int cpc_sdio_process_aggregated_frame(struct cpc_sdio *ctx, unsigned char *aggregated_frame,
+ unsigned int frame_len)
+{
+ const struct frame_header *header;
+ size_t aligned_payload_size;
+ struct sk_buff *rx_skb;
+ const unsigned char *payload_start;
+ size_t payload_size;
+ size_t frame_size;
+ u32 frame_count;
+ unsigned int payload_offset;
+
+ /* Get frame count from aggregated frame (4-byte u32) */
+ frame_count = le32_to_cpu(*((__le32 *)aggregated_frame));
+ if (frame_count == 0) {
+ dev_dbg(ctx->dev, "Process aggregated frame: invalid frame count: %u\n",
+ frame_count);
+ return -EINVAL;
+ }
+
+ /* Ensure frame count doesn't exceed our negotiated maximum */
+ if (frame_count > ctx->max_aggregation) {
+ dev_warn(ctx->dev,
+ "Process aggregated frame: frame count %u exceeds negotiated maximum %u\n",
+ frame_count, ctx->max_aggregation);
+ //frame_count = ctx->effective_max_aggregation;
+ }
+
+ /* Header starts at block 0 after frame count */
+ header = (struct frame_header *)&aggregated_frame[sizeof(__le32)];
+
+ /* Payloads start at block 1 (after complete block 0) */
+ payload_offset = GB_CPC_SDIO_BLOCK_SIZE;
+
+ for (unsigned int i = 0; i < frame_count; i++) {
+ payload_start = &aggregated_frame[payload_offset];
+
+ /* Get payload size for this frame */
+ if (!cpc_sdio_get_payload_size(ctx, header, &payload_size)) {
+ dev_err(ctx->dev,
+ "Process aggregated frame: failed to get payload size, aborting.\n");
+ return -ERANGE;
+ }
+
+ aligned_payload_size = ALIGN(payload_size, GB_CPC_SDIO_ALIGNMENT);
+
+ /* Validate the payload is within the buffer boundary */
+ if (payload_offset + aligned_payload_size > frame_len) {
+ dev_err(ctx->dev,
+ "Process aggregated frame: payload is out of buffer boundary, aborting at frame %u\n",
+ i);
+ return -ENOSPC;
+ }
+
+ /* Calculate total frame size: CPC header + Greybus header + payload */
+ frame_size = CPC_FRAME_HEADER_SIZE + payload_size;
+
+ /* Allocate sk_buff for reconstructed frame */
+ rx_skb = alloc_skb(frame_size, GFP_KERNEL);
+ if (rx_skb) {
+ /* Copy header */
+ memcpy(skb_put(rx_skb, CPC_FRAME_HEADER_SIZE), header,
+ CPC_FRAME_HEADER_SIZE);
+
+ /* Copy payload */
+ if (payload_size > 0)
+ memcpy(skb_put(rx_skb, payload_size), payload_start, payload_size);
+
+ /* Send reconstructed frame to CPC core */
+ cpc_hd_rcvd(ctx->cpc_hd, rx_skb);
+ }
+ /* else: allocation failed, skip this frame but continue processing */
+
+ /* Move to next header and payload start address */
+ header++;
+ payload_offset += aligned_payload_size;
+ }
+
+ return 0;
+}
+
+static u32 cpc_sdio_get_rx_num_bytes(struct sdio_func *func, int *err)
+{
+ unsigned int rx_num_block_addr = 0x0C;
+
+ return sdio_readl(func, rx_num_block_addr, err);
+}
+
+static void gb_cpc_sdio_rx(struct cpc_sdio *ctx)
+{
+ unsigned char *rx_buff;
+ size_t data_len;
+ int err;
+
+ sdio_claim_host(ctx->func);
+ data_len = cpc_sdio_get_rx_num_bytes(ctx->func, &err);
+
+ if (err) {
+ dev_err(ctx->dev, "failed to obtain byte count (%d)\n", err);
+ goto release_host;
+ }
+
+ /* Validate minimum RX data length */
+ if (data_len < sizeof(u32) + CPC_FRAME_HEADER_SIZE) {
+ dev_err(ctx->dev, "failed to obtain enough bytes for a header (%zu)\n", data_len);
+ goto release_host;
+ }
+
+ /* Allocate sk_buff for RX data */
+ rx_buff = kmalloc(data_len, GFP_KERNEL);
+ if (!rx_buff)
+ goto release_host;
+
+ err = sdio_readsb(ctx->func, rx_buff, GB_CPC_SDIO_FIFO_ADDR, data_len);
+ sdio_release_host(ctx->func);
+
+ if (err) {
+ dev_err(ctx->dev, "failed to sdio_readsb (%d)\n", err);
+ goto free_rx_skb;
+ }
+
+ if (data_len < GB_CPC_SDIO_BLOCK_SIZE) {
+ dev_err(ctx->dev, "received %zd bytes, expected at least %u bytes\n", data_len,
+ GB_CPC_SDIO_BLOCK_SIZE);
+ goto free_rx_skb;
+ }
+
+ /* de-aggregate incoming skb into individual frames and send to CPC core */
+ cpc_sdio_process_aggregated_frame(ctx, rx_buff, data_len);
+
+free_rx_skb:
+ kfree(rx_buff);
+
+ return;
+
+release_host:
+ sdio_release_host(ctx->func);
+}
+
+static void gb_cpc_sdio_tx(struct cpc_sdio *ctx)
+{
+ struct sk_buff_head frame_list;
+ unsigned char *tx_buff;
+ size_t tx_len;
+ int err;
+
+ skb_queue_head_init(&frame_list);
+
+ /* Dequeue the negotiated maximum aggregated frames from the host device */
+ cpc_hd_dequeue_many(ctx->cpc_hd, &frame_list, ctx->max_aggregation);
+
+ /* Check if any frames were dequeued */
+ if (skb_queue_empty(&frame_list))
+ return;
+
+ tx_buff = cpc_sdio_build_aggregated_frame(ctx, &frame_list, &tx_len);
+ if (!tx_buff) {
+ dev_err(ctx->dev, "failed to build aggregated frame\n");
+ goto cleanup_frames;
+ }
+
+ sdio_claim_host(ctx->func);
+ err = sdio_writesb(ctx->func, GB_CPC_SDIO_FIFO_ADDR, tx_buff, tx_len);
+ sdio_release_host(ctx->func);
+
+ if (err)
+ dev_err(ctx->dev, "failed to sdio_writesb (%d)\n", err);
+
+ kfree(tx_buff);
+
+cleanup_frames:
+ /* Clean up any remaining frames in the list */
+ skb_queue_purge(&frame_list);
+}
+
+static void gb_cpc_sdio_rx_tx(struct cpc_sdio *ctx)
+{
+ gb_cpc_sdio_rx(ctx);
+
+ set_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
+ gb_cpc_sdio_tx(ctx);
+ clear_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
+}
+
+static void gb_cpc_sdio_tx_work(struct work_struct *work)
+{
+ struct cpc_sdio *ctx = container_of(work, struct cpc_sdio, tx_work);
+
+ /* Do not execute concurrently to the interrupt */
+ if (test_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags)) {
+ set_bit(CPC_SDIO_FLAG_TX_WORK_DELAYED, &ctx->flags);
+ return;
+ }
+
+ gb_cpc_sdio_tx(ctx);
+}
+
+static struct cpc_hd_driver cpc_sdio_driver = {
+ .wake_tx = gb_cpc_sdio_wake_tx,
+};
+
+static int cpc_sdio_init(struct sdio_func *func)
+{
+ unsigned char rx_data_ready_irq_en_bit = BIT(0);
+ unsigned int irq_enable_addr = 0x09;
+ int err;
+
+ /* Enable the read data ready interrupt. */
+ sdio_writeb(func, rx_data_ready_irq_en_bit, irq_enable_addr, &err);
+ if (err)
+ dev_err(&func->dev, "failed to set data ready interrupt (%d)\n", err);
+
+ return err;
+}
+
+static void cpc_sdio_irq_handler(struct sdio_func *func)
+{
+ struct cpc_sdio *ctx = sdio_get_drvdata(func);
+ struct device *dev = &func->dev;
+ unsigned int rx_data_pending_irq_bit = 0;
+ unsigned int irq_status_addr = 0x08;
+ unsigned long int_status;
+ int err;
+
+ int_status = sdio_readb(func, irq_status_addr, &err);
+ if (err) {
+ dev_err(dev, "failed to read interrupt status registers (%d)\n", err);
+ return;
+ }
+
+ if (__test_and_clear_bit(rx_data_pending_irq_bit, &int_status)) {
+ /* Clear the IRQ on the device side. */
+ sdio_writeb(func, BIT(rx_data_pending_irq_bit), irq_status_addr, &err);
+ if (err) {
+ dev_err(dev, "failed to clear read interrupt (%d), interrupt will repeat\n",
+ err);
+ return;
+ }
+
+ cancel_work_sync(&ctx->tx_work);
+ gb_cpc_sdio_rx_tx(ctx);
+
+ if (test_and_clear_bit(CPC_SDIO_FLAG_TX_WORK_DELAYED, &ctx->flags))
+ schedule_work(&ctx->tx_work);
+ }
+
+ if (int_status)
+ dev_err_once(dev, "unhandled interrupt from the device (%ld)\n", int_status);
+}
+
+static int cpc_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
+{
+ struct cpc_host_device *cpc_hd;
+ struct cpc_sdio *ctx;
+ int err;
+
+ /* Allocate our private context */
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ /* Create CPC host device with our context as private data */
+ cpc_hd = cpc_hd_create(&cpc_sdio_driver, &func->dev, ctx);
+ if (IS_ERR(cpc_hd)) {
+ kfree(ctx);
+ return PTR_ERR(cpc_hd);
+ }
+
+ /* Initialize context */
+ ctx->cpc_hd = cpc_hd;
+ ctx->dev = cpc_hd_dev(cpc_hd);
+ ctx->func = func;
+ ctx->max_aggregation = GB_CPC_SDIO_DEFAULT_AGGREGATION;
+
+ INIT_WORK(&ctx->tx_work, gb_cpc_sdio_tx_work);
+
+ /* Make ctx available to IRQ handler before enabling/claiming IRQ */
+ sdio_set_drvdata(func, ctx);
+
+ sdio_claim_host(func);
+
+ err = sdio_enable_func(func);
+ if (err)
+ goto release_host;
+
+ err = sdio_set_block_size(func, GB_CPC_SDIO_BLOCK_SIZE);
+ if (err)
+ goto disable_func;
+
+ err = cpc_sdio_init(func);
+ if (err)
+ goto disable_func;
+
+ err = sdio_claim_irq(func, cpc_sdio_irq_handler);
+ if (err)
+ goto disable_func;
+
+ err = cpc_hd_add(cpc_hd);
+ if (err)
+ goto release_irq;
+
+ sdio_release_host(func);
+
+ return 0;
+
+release_irq:
+ sdio_release_irq(func);
+
+disable_func:
+ sdio_disable_func(func);
+
+release_host:
+ sdio_release_host(func);
+ cpc_hd_put(cpc_hd);
+ kfree(ctx);
+
+ return err;
+}
+
+static void cpc_sdio_remove(struct sdio_func *func)
+{
+ struct cpc_sdio *ctx = sdio_get_drvdata(func);
+ struct cpc_host_device *cpc_hd = ctx->cpc_hd;
+
+ /* Prevent new TX wakeups and wake the thread */
+ set_bit(CPC_SDIO_FLAG_SHUTDOWN, &ctx->flags);
+
+ /* Cancel and flush any pending TX work */
+ cancel_work_sync(&ctx->tx_work);
+
+ sdio_claim_host(func);
+ sdio_release_irq(func);
+ sdio_disable_func(func);
+ sdio_release_host(func);
+
+ cpc_hd_del(cpc_hd);
+ cpc_hd_put(cpc_hd);
+
+ kfree(ctx);
+}
+
+/* NOTE: Development/RFC purposes only. */
+static const struct sdio_device_id sdio_ids[] = {
+ {
+ SDIO_DEVICE(0x0296, 0x5347),
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(sdio, sdio_ids);
+
+static struct sdio_driver gb_cpc_sdio_driver = {
+ .name = KBUILD_MODNAME,
+ .id_table = sdio_ids,
+ .probe = cpc_sdio_probe,
+ .remove = cpc_sdio_remove,
+ .drv = {
+ .owner = THIS_MODULE,
+ .name = KBUILD_MODNAME,
+ },
+};
+
+module_sdio_driver(gb_cpc_sdio_driver);
+
+MODULE_DESCRIPTION("Greybus Host Driver for Silicon Labs devices using SDIO");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Damien Riégel <damien.riegel@silabs.com>");
--
2.49.0
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-15-damien.riegel%40silabs.com
patch subject: [PATCH 14/14] greybus: cpc: add CPC SDIO host driver
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20251214/202512140305.VFN3KkVr-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251214/202512140305.VFN3KkVr-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/202512140305.VFN3KkVr-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/greybus/cpc/sdio.c:166:59: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
166 | dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
| ~~~ ^~~~~~~
| %zu
include/linux/dev_printk.h:165:39: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:285:19: note: expanded from macro 'dynamic_dev_dbg'
285 | dev, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:261:59: note: expanded from macro '_dynamic_func_call'
261 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:259:65: note: expanded from macro '_dynamic_func_call_cls'
259 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:231:15: note: expanded from macro '__dynamic_func_call_cls'
231 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
drivers/greybus/cpc/sdio.c:172:60: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
172 | dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
| ~~~ ^~~~~~~
| %zu
include/linux/dev_printk.h:165:39: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:285:19: note: expanded from macro 'dynamic_dev_dbg'
285 | dev, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:261:59: note: expanded from macro '_dynamic_func_call'
261 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:259:65: note: expanded from macro '_dynamic_func_call_cls'
259 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:231:15: note: expanded from macro '__dynamic_func_call_cls'
231 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
2 warnings generated.
vim +166 drivers/greybus/cpc/sdio.c
156
157 static bool cpc_sdio_get_payload_size(struct cpc_sdio *ctx, const struct frame_header *header,
158 size_t *payload_size)
159 {
160 size_t gb_size;
161
162 gb_size = le16_to_cpu(header->gb.size);
163
164 /* Validate that the size is at least as large as the Greybus header */
165 if (gb_size < GREYBUS_HEADER_SIZE) {
> 166 dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
167 return false;
168 }
169
170 /* Validate maximum size */
171 if (gb_size > (GB_CPC_SDIO_MSG_SIZE_MAX + GREYBUS_HEADER_SIZE)) {
172 dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
173 return false;
174 }
175
176 /* Size includes the Greybus header, so subtract it to get payload size */
177 *payload_size = gb_size - GREYBUS_HEADER_SIZE;
178
179 return true;
180 }
181
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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-15-damien.riegel%40silabs.com
patch subject: [PATCH 14/14] greybus: cpc: add CPC SDIO host driver
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20251213/202512131651.6JPWdgAH-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251213/202512131651.6JPWdgAH-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/202512131651.6JPWdgAH-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:621,
from include/asm-generic/bug.h:31,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/current.h:6,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/delay.h:13,
from drivers/greybus/cpc/sdio.c:9:
drivers/greybus/cpc/sdio.c: In function 'cpc_sdio_get_payload_size':
>> drivers/greybus/cpc/sdio.c:166:35: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
166 | dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:231:29: note: in definition of macro '__dynamic_func_call_cls'
231 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:261:9: note: in expansion of macro '_dynamic_func_call_cls'
261 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:284:9: note: in expansion of macro '_dynamic_func_call'
284 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:30: note: in expansion of macro 'dev_fmt'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/greybus/cpc/sdio.c:166:17: note: in expansion of macro 'dev_dbg'
166 | dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
| ^~~~~~~
drivers/greybus/cpc/sdio.c:166:67: note: format string is defined here
166 | dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
| ~~^
| |
| long unsigned int
| %u
In file included from include/linux/printk.h:621,
from include/asm-generic/bug.h:31,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/current.h:6,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/delay.h:13,
from drivers/greybus/cpc/sdio.c:9:
drivers/greybus/cpc/sdio.c:172:35: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
172 | dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:231:29: note: in definition of macro '__dynamic_func_call_cls'
231 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:261:9: note: in expansion of macro '_dynamic_func_call_cls'
261 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:284:9: note: in expansion of macro '_dynamic_func_call'
284 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:30: note: in expansion of macro 'dev_fmt'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/greybus/cpc/sdio.c:172:17: note: in expansion of macro 'dev_dbg'
172 | dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
| ^~~~~~~
drivers/greybus/cpc/sdio.c:172:68: note: format string is defined here
172 | dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
| ~~^
| |
| long unsigned int
| %u
--
>> Warning: drivers/greybus/cpc/sdio.c:73 This comment starts with '/**', but isn't a kernel-doc comment. Refer to Documentation/doc-guide/kernel-doc.rst
* Return the memory requirement in bytes for the aggregated frame aligned to the block size
vim +166 drivers/greybus/cpc/sdio.c
> 9 #include <linux/delay.h>
10 #include <linux/device.h>
11 #include <linux/kthread.h>
12 #include <linux/minmax.h>
13 #include <linux/mmc/sdio_func.h>
14 #include <linux/mmc/sdio_ids.h>
15 #include <linux/skbuff.h>
16 #include <linux/slab.h>
17 #include <linux/wait.h>
18 #include <linux/workqueue.h>
19
20 #include "cpc.h"
21 #include "header.h"
22 #include "host.h"
23
24 #define GB_CPC_SDIO_MSG_SIZE_MAX 4096
25 #define GB_CPC_SDIO_BLOCK_SIZE 256U
26 #define GB_CPC_SDIO_FIFO_ADDR 0
27 #define GB_CPC_SDIO_ALIGNMENT 4
28 #define GB_CPC_SDIO_DEFAULT_AGGREGATION 1
29 #define CPC_FRAME_HEADER_SIZE (CPC_HEADER_SIZE + GREYBUS_HEADER_SIZE)
30 #define GB_CPC_SDIO_MAX_AGGREGATION ((GB_CPC_SDIO_BLOCK_SIZE - sizeof(u32)) / CPC_FRAME_HEADER_SIZE)
31
32 enum cpc_sdio_flags {
33 CPC_SDIO_FLAG_IRQ_RUNNING,
34 CPC_SDIO_FLAG_TX_WORK_DELAYED,
35 CPC_SDIO_FLAG_SHUTDOWN,
36 };
37
38 struct cpc_sdio {
39 struct cpc_host_device *cpc_hd;
40 struct device *dev;
41 struct sdio_func *func;
42
43 struct work_struct tx_work;
44 unsigned long flags;
45
46 wait_queue_head_t event_queue;
47 u8 max_aggregation;
48 };
49
50 struct frame_header {
51 struct cpc_header cpc;
52 struct gb_operation_msg_hdr gb;
53 } __packed;
54
55 static inline struct cpc_sdio *cpc_hd_to_cpc_sdio(struct cpc_host_device *cpc_hd)
56 {
57 return (struct cpc_sdio *)cpc_hd->priv;
58 }
59
60 static int gb_cpc_sdio_wake_tx(struct cpc_host_device *cpc_hd)
61 {
62 struct cpc_sdio *ctx = cpc_hd_to_cpc_sdio(cpc_hd);
63
64 if (test_bit(CPC_SDIO_FLAG_SHUTDOWN, &ctx->flags))
65 return 0;
66
67 /* Use system workqueue for TX processing */
68 schedule_work(&ctx->tx_work);
69
70 return 0;
71 }
72
> 73 /**
74 * Return the memory requirement in bytes for the aggregated frame aligned to the block size
75 */
76 static size_t cpc_sdio_get_aligned_size(struct cpc_sdio *ctx, struct sk_buff_head *frame_list)
77 {
78 size_t size = 0;
79 struct sk_buff *frame;
80
81 /* Calculate total payload size */
82 skb_queue_walk(frame_list, frame) {
83 size_t payload_len = frame->len - CPC_FRAME_HEADER_SIZE;
84
85 /* payload is aligned and padded to 4 bytes */
86 size += ALIGN(payload_len, GB_CPC_SDIO_ALIGNMENT);
87 }
88
89 /* Make sure the total payload size is a round number of blocks */
90 size = ALIGN(size, GB_CPC_SDIO_BLOCK_SIZE);
91
92 /* Add an additional block for headers */
93 size += GB_CPC_SDIO_BLOCK_SIZE;
94
95 return size;
96 }
97
98 static unsigned char *cpc_sdio_build_aggregated_frame(struct cpc_sdio *ctx,
99 struct sk_buff_head *frame_list,
100 size_t *xfer_len)
101 {
102 unsigned char *tx_buff;
103 struct sk_buff *frame;
104 __le32 *frame_count;
105 size_t xfer_size;
106 unsigned int i = 0;
107
108 xfer_size = cpc_sdio_get_aligned_size(ctx, frame_list);
109
110 /* Allocate aggregated frame */
111 tx_buff = kmalloc(xfer_size, GFP_KERNEL);
112 if (!tx_buff)
113 return NULL;
114
115 frame_count = (__le32 *)tx_buff;
116 *frame_count = cpu_to_le32(skb_queue_len(frame_list));
117 i += 4;
118
119 /* Copy frame headers to aggregate buffer */
120 skb_queue_walk(frame_list, frame) {
121 memcpy(&tx_buff[i], frame->data, CPC_FRAME_HEADER_SIZE);
122 i += CPC_FRAME_HEADER_SIZE;
123 }
124
125 /* Zero-pad remainder of header block to fill complete SDIO block */
126 if (i < GB_CPC_SDIO_BLOCK_SIZE)
127 memset(&tx_buff[i], 0, GB_CPC_SDIO_BLOCK_SIZE - i);
128
129 /* Start injecting payload at beginning of second block */
130 i = GB_CPC_SDIO_BLOCK_SIZE;
131
132 /* Build payload blocks if required */
133 if (xfer_size > GB_CPC_SDIO_BLOCK_SIZE) {
134 skb_queue_walk(frame_list, frame) {
135 size_t payload_len, padding_len;
136
137 if (frame->len <= CPC_FRAME_HEADER_SIZE)
138 continue;
139
140 payload_len = frame->len - CPC_FRAME_HEADER_SIZE;
141 memcpy(&tx_buff[i], &frame->data[CPC_FRAME_HEADER_SIZE], payload_len);
142 i += payload_len;
143
144 padding_len = ALIGN(payload_len, GB_CPC_SDIO_ALIGNMENT) - payload_len;
145 if (padding_len) {
146 memset(&tx_buff[i], 0, padding_len);
147 i += padding_len;
148 }
149 }
150 }
151
152 *xfer_len = xfer_size;
153
154 return tx_buff;
155 }
156
157 static bool cpc_sdio_get_payload_size(struct cpc_sdio *ctx, const struct frame_header *header,
158 size_t *payload_size)
159 {
160 size_t gb_size;
161
162 gb_size = le16_to_cpu(header->gb.size);
163
164 /* Validate that the size is at least as large as the Greybus header */
165 if (gb_size < GREYBUS_HEADER_SIZE) {
> 166 dev_dbg(ctx->dev, "Invalid Greybus header size: %lu\n", gb_size);
167 return false;
168 }
169
170 /* Validate maximum size */
171 if (gb_size > (GB_CPC_SDIO_MSG_SIZE_MAX + GREYBUS_HEADER_SIZE)) {
172 dev_dbg(ctx->dev, "Payload size exceeds maximum: %lu\n", gb_size);
173 return false;
174 }
175
176 /* Size includes the Greybus header, so subtract it to get payload size */
177 *payload_size = gb_size - GREYBUS_HEADER_SIZE;
178
179 return true;
180 }
181
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri Dec 12, 2025 at 11:13 AM EST, Damien Riégel wrote:
> +#include <linux/atomic.h>
> +#include <linux/bitops.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kthread.h>
> +#include <linux/minmax.h>
> +#include <linux/mmc/sdio_func.h>
> +#include <linux/mmc/sdio_ids.h>
> +#include <linux/skbuff.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
I think there are a few includes that are not used here (`atomic.h`,
`delay.h`, `minmax.h`, `slab.h`).
> +/**
> + * Return the memory requirement in bytes for the aggregated frame aligned to the block size
> + */
> +static size_t cpc_sdio_get_aligned_size(struct cpc_sdio *ctx, struct sk_buff_head *frame_list)
> +{
> + size_t size = 0;
> + struct sk_buff *frame;
Check for reverse xmass tree notation, there are a few occurences in
this source file where this is not enforced.
> +static unsigned char *cpc_sdio_build_aggregated_frame(struct cpc_sdio *ctx,
> + struct sk_buff_head *frame_list,
> + size_t *xfer_len)
> +{
> + [...]
> + frame_count = (__le32 *)tx_buff;
> + *frame_count = cpu_to_le32(skb_queue_len(frame_list));
> + i += 4;
`i += sizeof(*frame_count);` to avoid magic value. Also, it is more
common to return the size of the built array instead of the array
itself, so I would instead pass `char **tx_buff` as an argument and
return `xfer_len`.
> +
> + /* Copy frame headers to aggregate buffer */
> + skb_queue_walk(frame_list, frame) {
> + memcpy(&tx_buff[i], frame->data, CPC_FRAME_HEADER_SIZE);
> + i += CPC_FRAME_HEADER_SIZE;
> + }
Declaring a local `struct frame_header*` would be more explicit.
> + /* Zero-pad remainder of header block to fill complete SDIO block */
> + if (i < GB_CPC_SDIO_BLOCK_SIZE)
> + memset(&tx_buff[i], 0, GB_CPC_SDIO_BLOCK_SIZE - i);
Remove unnecessary `if`.
> +/**
> + * Process aggregated frame
> + * Reconstructed frame layout:
> + * +-----+-----+-----+------+------+------+------+-------+---------+
> + * | CPC Header (4B) | Size | OpID | Type | Stat | CPort | Payload |
> + * +-----+-----+-----+------+------+------+------+-------+---------+
> + */
> +static int cpc_sdio_process_aggregated_frame(struct cpc_sdio *ctx, unsigned char *aggregated_frame,
> + unsigned int frame_len)
> +{
> + [...]
> + /* Ensure frame count doesn't exceed our negotiated maximum */
> + if (frame_count > ctx->max_aggregation) {
> + dev_warn(ctx->dev,
> + "Process aggregated frame: frame count %u exceeds negotiated maximum %u\n",
> + frame_count, ctx->max_aggregation);
> + //frame_count = ctx->effective_max_aggregation;
> + }
First off, remove inline comment. Also, this function returns an integer
that is never checked by the caller, so change the reurn type to `void`.
I think the solution to handling this error is to simply return.
> +
> + /* Header starts at block 0 after frame count */
> + header = (struct frame_header *)&aggregated_frame[sizeof(__le32)];
Use `sizeof(frame_count)` to make this more explicit, and make it easier
to maintain if `frame_count` ever changes type.
> + for (unsigned int i = 0; i < frame_count; i++) {
No need for `i` to be unsigned, just use an `int` to alleviate the code.
> + /* Allocate sk_buff for reconstructed frame */
> + rx_skb = alloc_skb(frame_size, GFP_KERNEL);
> + if (rx_skb) {
> + /* Copy header */
> + memcpy(skb_put(rx_skb, CPC_FRAME_HEADER_SIZE), header,
> + CPC_FRAME_HEADER_SIZE);
> +
> + /* Copy payload */
> + if (payload_size > 0)
> + memcpy(skb_put(rx_skb, payload_size), payload_start, payload_size);
> +
> + /* Send reconstructed frame to CPC core */
> + cpc_hd_rcvd(ctx->cpc_hd, rx_skb);
> + }
> + /* else: allocation failed, skip this frame but continue processing */
No? If we're not able to allocate, we should just return. Change the
`if` to check for a failed allocation and return. This has the added
benefit of keeping the nominal path unindented.
> +static u32 cpc_sdio_get_rx_num_bytes(struct sdio_func *func, int *err)
> +{
> + unsigned int rx_num_block_addr = 0x0C;
> +
> + return sdio_readl(func, rx_num_block_addr, err);
> +}
Have `0x0C` in a `GB_CPC_SDIO_RX_BLOCK_CNT_ADDR` define for better
readability.
> +static void gb_cpc_sdio_tx(struct cpc_sdio *ctx)
> +{
> +cleanup_frames:
> + /* Clean up any remaining frames in the list */
> + skb_queue_purge(&frame_list);
Misleading comment, since `frame_list` will always have frames left in
it, as they are never removed during TX.
> +static void gb_cpc_sdio_rx_tx(struct cpc_sdio *ctx)
> +{
> + gb_cpc_sdio_rx(ctx);
> +
> + set_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
> + gb_cpc_sdio_tx(ctx);
> + clear_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
> +}
This is very surprising to me, why are we processing our TX in the RX
IRQ? This seems entirely unnecessary. It feels like we could rework this
and remove `CPC_SDIO_FLAG_IRQ_RUNNING`.
Thanks,
--
Yacin Belmihoub-Martel
Silicon Laboratories
© 2016 - 2025 Red Hat, Inc.