Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <786e357b-df63-42bb-bc8a-b5fc8d6e9146@ieee.org>
Date:   Tue, 12 Dec 2023 08:01:08 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 1/1] greybus: gb-beagleplay: Remove use of pad bytes
Content-Language: en-US
To:     Ayush Singh <ayushdevel1325@gmail.com>,
        greybus-dev@lists.linaro.org
Cc:     johan@kernel.org, elder@kernel.org, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, jkridner@beagleboard.org, nm@ti.com,
        yujie.liu@intel.com
References: <20231211065420.213664-1-ayushdevel1325@gmail.com>
 <20231211065420.213664-2-ayushdevel1325@gmail.com>
From:   Alex Elder <elder@ieee.org>
In-Reply-To: <20231211065420.213664-2-ayushdevel1325@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/11/23 12:54 AM, Ayush Singh wrote:
> Make gb-beagleplay greybus spec compliant by moving cport information to
> transport layer instead of using `header->pad` bytes.
> 
> Greybus HDLC frame now has the following payload:
> 1. le16 cport
> 2. gb_operation_msg_hdr msg_header
> 3. u8 *msg_payload
> 
> Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>

I would say that this is an improvement, but I wish I
had a better picture in mind of how this works.  The
initial commit provided some explanation, but even
there it talks about the "CC1352 (running SVC Zephyr
application)" and that leads me to wonder even how
the hardware is structured.  (I'm not really asking
you for this right now, but you have a reference to
something that provides some background, you should
provide it for context.)

Another general comment is that the use of HDLC seems
like it could be a more clearly separated layer that
could be used by other Greybus protocols or applications.
Maybe that's overkill, but it is a distinct layer, right?

I had a comment or two about using (void *) instead of
(u8 *), to reduce the need for explicit type casts.  But
I found that (u8 *) is used elsewhere in the Greybus code.

One comment I *will* share is that the serdev RX callback
has a const receive buffer.  I recommend you preserve that
"constness" in your code.

					-Alex

> ---
>   drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++---------
>   1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> index 1e70ff7e3da4..82dc8a25e6b9 100644
> --- a/drivers/greybus/gb-beagleplay.c
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -85,17 +85,31 @@ struct hdlc_payload {
>   	void *buf;
>   };
>   
> +/**
> + * struct hdlc_greybus_frame - Structure to represent greybus HDLC frame payload
> + *
> + * @cport: cport id
> + * @hdr: greybus operation header
> + * @payload: greybus message payload
> + *
> + * The HDLC payload sent over UART for greybus address has cport preappended to greybus message
> + */
> +struct hdlc_greybus_frame {
> +	__le16 cport;
> +	struct gb_operation_msg_hdr hdr;
> +	u8 payload[];
> +} __packed;
> +
>   static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>   {
> -	u16 cport_id;
> -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
> -
> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
> +	u16 gb_msg_len = le16_to_cpu(gb_frame->hdr.size);
>   
>   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
> +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
>   
> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> +	greybus_data_rcvd(bg->gb_hd, cport_id, (u8 *)&gb_frame->hdr, gb_msg_len);
>   }
>   
>   static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> @@ -336,10 +350,23 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>   	.write_wakeup = gb_tty_wakeup,
>   };
>   
> +/**
> + * gb_message_send() - Send greybus message using HDLC over UART
> + *
> + * @hd: pointer to greybus host device
> + * @cport: AP cport where message originates
> + * @msg: greybus message to send
> + * @mask: gfp mask
> + *
> + * Greybus HDLC frame has the following format:
> + * 1. le16 cport
> + * 2. gb_operation_msg_hdr msg_header
> + * 3. u8 *msg_payload
> + */
>   static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
>   {
>   	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
> -	struct hdlc_payload payloads[2];
> +	struct hdlc_payload payloads[3];
>   	__le16 cport_id = le16_to_cpu(cport);
>   
>   	dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
> @@ -348,14 +375,14 @@ static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_messa
>   	if (le16_to_cpu(msg->header->size) > RX_HDLC_PAYLOAD)
>   		return dev_err_probe(&hd->dev, -E2BIG, "Greybus message too big");
>   
> -	memcpy(msg->header->pad, &cport_id, sizeof(cport_id));
> -
> -	payloads[0].buf = msg->header;
> -	payloads[0].len = sizeof(*msg->header);
> -	payloads[1].buf = msg->payload;
> -	payloads[1].len = msg->payload_size;
> +	payloads[0].buf = &cport_id;
> +	payloads[0].len = sizeof(cport_id);
> +	payloads[1].buf = msg->header;
> +	payloads[1].len = sizeof(*msg->header);
> +	payloads[2].buf = msg->payload;
> +	payloads[2].len = msg->payload_size;
>   
> -	hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 2);
> +	hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 3);
>   	greybus_message_sent(bg->gb_hd, msg, 0);
>   
>   	return 0;