Re: [PATCH 1/1] greybus: gb-beagleplay: Remove use of pad bytes

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Dec 06, 2023 at 08:36:00PM +0530, Ayush Singh wrote:
> Make gb-beagleplay greybus spec compliant by using a wrapper around HDLC
> payload to include cport information.

"wrapper"?  You just changed the data you send on your "wire", right?

> Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Closes: https://lore.kernel.org/r/202311072329.Xogj7hGW-lkp@intel.com/
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> ---
>  drivers/greybus/gb-beagleplay.c | 44 +++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> index 1e70ff7e3da4..fb40ade9364f 100644
> --- a/drivers/greybus/gb-beagleplay.c
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -85,17 +85,35 @@ struct hdlc_payload {
>  	void *buf;
>  };
>  
> +/**
> + * struct hdlc_greybus_frame - Structure to represent greybus HDLC frame

Represent it where?  And where is this documented?

> + *
> + * @cport: cport id
> + * @hdr: greybus operation header
> + * @payload: greybus message payload
> + */
> +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;
> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
>  
> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> +	/* Ensure that the greybus message is valid */
> +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
> +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");

Don't spam the kernel log for corrupted data on the line, that would be
a mess.  Use a tracepoint?

> +		return;
> +	}
>  
>  	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);

Better yet, put the error in the debug message?

>  
> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],

Fun with pointer math.  This feels really fragile, why not just point to
the field instead?

> +			  le16_to_cpu(gb_frame->hdr.size));

You convert the size twice, might as well do it once up above, right?

Also, it's best to use the same value you checked as the value you pass
in here, odds are you can't change the data underneath you (like you
could if this came from userspace), but it's best to not get in the
habit of coding like this.


>  }
>  
>  static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>  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];

why 3?

It's ok to put this on the stack?

>  	__le16 cport_id = le16_to_cpu(cport);
>  
>  	dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
> @@ -348,14 +366,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;

So you send the cport number as the first message?  Don't you need to
document this somewhere?

thanks,

greg k-h