[V2,1/1] greybus: gb-beagleplay: Remove use of pad bytes

  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>
---
 drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 14 deletions(-)
  

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;
  

On 12/12/23 19:31, Alex Elder wrote:
> 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.)

Yes, I am thinking of revamping the Beagle connect docs to reflect the 
new architecture with some charts and provide a better overall picture. 
It is sorely needed at this point.


> 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?

Initial commits of gb-beagleplay did separate all the HDLC parts from 
the driver. However, it was decided to keep it together and maybe 
extract it in the future if other drivers need it.


>
> 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

The constness of the receive buffer is actually preserved. The 
`gb_tty_receive` function calls `hdlc_rx` (which takes const u8 *). This 
function copies the data to a separate buffer 
(`gb_beagleplay->rx_buffer`) for further processing. So the const data 
is not modified.


Ayush Singh
  

On Mon, Dec 11, 2023 at 12:24:18PM +0530, 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>
> ---
>  drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 14 deletions(-)

This doesn't apply against my char-misc-next branch at all, what did you
generate it against?

thanks,

greg k-h
  

On 12/15/23 21:50, Greg KH wrote:

> On Mon, Dec 11, 2023 at 12:24:18PM +0530, 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>
>> ---
>>   drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++---------
>>   1 file changed, 41 insertions(+), 14 deletions(-)
> This doesn't apply against my char-misc-next branch at all, what did you
> generate it against?
>
> thanks,
>
> greg k-h

The base commit of my tree is 
`0f5f12ac05f36f117e793656c3f560625e927f1b`. The tag is `next-20231205`.

I can rebase to a newer tag if you wish.

Ayush Singh
  

On Sat, Dec 16, 2023 at 05:39:30AM +0530, Ayush Singh wrote:
> On 12/15/23 21:50, Greg KH wrote:
> 
> > On Mon, Dec 11, 2023 at 12:24:18PM +0530, 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>
> > > ---
> > >   drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++---------
> > >   1 file changed, 41 insertions(+), 14 deletions(-)
> > This doesn't apply against my char-misc-next branch at all, what did you
> > generate it against?
> > 
> > thanks,
> > 
> > greg k-h
> 
> The base commit of my tree is `0f5f12ac05f36f117e793656c3f560625e927f1b`.
> The tag is `next-20231205`.
> 
> I can rebase to a newer tag if you wish.

Please rebase on the char-misc-next branch otherwise I can not take it.

thanks,

greg k-h