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,