Re: [RFC PATCH v2 11/12] greybus: cpc: honour remote's RX window

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

On Fri Nov 14, 2025 at 10:07 AM EST, Damien Riégel wrote:
> +/**
> + * cpc_header_get_frames_acked_count() - Get frames to be acknowledged.
> + * @seq: Current sequence number of the endpoint.
> + * @ack: Acknowledge number of the received frame.
> + *
> + * Return: Frames to be acknowledged.
> + */
> +u8 cpc_header_get_frames_acked_count(u8 seq, u8 ack)
> +{
> +	u8 frames_acked_count;
> +
> +	/* Find number of frames acknowledged with ACK number. */
> +	if (ack > seq) {
> +		frames_acked_count = ack - seq;
> +	} else {
> +		frames_acked_count = 256 - seq;
> +		frames_acked_count += ack;
> +	}
> +
> +	return frames_acked_count;
> +}

There is no need to check whether `ack > seq` since the return value in
downcasted to a `u8`. For example, if `ack=0` and `seq=254`, we can
simply do `(u8)(0-254)=2`.

> +static void __cpc_protocol_receive_ack(struct cpc_cport *cport, u8 recv_wnd, u8 ack)
> +{
> +	struct gb_host_device *gb_hd = cport->cpc_hd->gb_hd;
> +	struct sk_buff *skb;
> +	u8 acked_frames;
> +
> +	cport->tcb.send_wnd = recv_wnd;

Little bit of a nitpick, but handling the RX window update in
`__cpc_protocol_receive_ack` seems a bit misleading to me, in the sense
that the ACK itself is not directly related to the window. 

I would either rename the function to indicate that we are handling all 
the CPort's metadata received from the CPC header, or I would move the 
RX window update to the caller.

Thanks,
-- 
Yacin Belmihoub-Martel
Silicon Laboratories