Message-Id: <DF58WUG0NKOO.1ZRGT1H7HFMRO@silabs.com>
Date: Mon, 22 Dec 2025 21:37:49 -0500
From: Damien Riégel <damien.riegel@...abs.com>
To: "Yacin Belmihoub-Martel" <yacin.belmihoub-martel@...abs.com>,
        <greybus-dev@...ts.linaro.org>
Cc: <linux-kernel@...r.kernel.org>, "Johan Hovold" <johan@...nel.org>,
        "Alex
 Elder" <elder@...nel.org>,
        "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>,
        "Silicon Labs Kernel Team"
 <linux-devel@...abs.com>,
        "Gabriel Beaulieu" <gabriel.beaulieu@...abs.com>
Subject: Re: [PATCH 14/14] greybus: cpc: add CPC SDIO host driver

On Fri Dec 12, 2025 at 5:01 PM EST, Yacin Belmihoub-Martel wrote:
> 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`).

Thanks, I'll check that. I wont reply to all your reviews to avoid
unnecessary noise, but I will apply the suggestions you made and fix the
errors reported by kernel test robot before spinning a new version.

>> +/**
>> + * 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.

I renamed some variables last minute and I have missed some styling
issues like this, will take care of that.

>
>> +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`.

Sure I can swap the length and the pointer.

>> +
>> +	/* 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.

Agreed, anyway I plan to revisit aggregation, the way it's done
currently is kind of a waste of space, and reading an invalid value here
basically means the device sent us garbage, so I agree that the right
course of action would be to just return and drop garbage data.

With the new aggregation scheme, the device would be basically free to
aggregate as many frames as it wants without breaking the protocol


>> +
>> +	/* 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.

Sure

>> +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`.

This is a pattern I picked up from SDIO WiFi drivers. At least a few of
them do that, I think to give some chance to the TX path to be exercised
when the radio is consistently trying to send packets to the host.
Otherwise, the TX path might be stuck most of the time on
sdio_claim_host() as they will fight for access to the bus.

I will leave it as is for now. This is something we can revisit later if
we're still not happy with it.

Cheers,
-- 
Damien

Powered by blists - more mailing lists