Re: [PATCH 14/14] greybus: cpc: add CPC SDIO host driver

["Yacin Belmihoub-Martel" <yacin.belmihoub-martel@silabs.com>]

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