[PATCH 0/1] Make gb-beagleplay driver Greybus compliant

[PATCH 0/1] Make gb-beagleplay driver Greybus compliant

[Ayush Singh]

In beagleplay beagleconnect setup, the AP is not directly connected to
greybus nodes. The SVC and APBridge tasks are moved to cc1352
coprocessor. This means that there is a need to send cport information
between linux host and cc1352.

In the initial version of the driver (and the reference implementation
gbridge I was using), the greybus header pad bytes were being used.
However, this was a violation of greybus spec.

The following patchset creates a wrapper around greybus message to send
the cport information without using the pad bytes.

Ayush Singh (1):
  greybus: gb-beagleplay: Remove use of pad bytes

 drivers/greybus/gb-beagleplay.c | 44 +++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 13 deletions(-)

-- 
2.43.0

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

[Ayush Singh]

Make gb-beagleplay greybus spec compliant by using a wrapper around HDLC
payload to include cport information.

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
+ *
+ * @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");
+		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);
 
-	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)],
+			  le16_to_cpu(gb_frame->hdr.size));
 }
 
 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];
 	__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;
 
-	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;
-- 
2.43.0

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

[Greg KH]

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

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

[Ayush Singh]

>> + *
>> + * @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?
Shouldn't corrupt data be a warning rather than debug message, since it 
indicates something wrong with the transport?
>>   
>> -	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?
It seems that taking address of members of packed structures is not 
valid. I get the `address-of-packed-member` warnings. Is it fine to 
ignore those in kernel?
>>   }
>>   
>>   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?

Well, the HDLC payload is just to store the length of the payload along 
with a pointer to its data. (kind of emulate a fat pointer). The reason 
for doing it this way is to avoid having to create a temp buffer for 
each message when sending data over UART (which was done in the initial 
version of the driver).

Ayush Singh

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

[Greg KH]

On Thu, Dec 07, 2023 at 10:33:54PM +0530, Ayush Singh wrote:
> > > + *
> > > + * @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?
> Shouldn't corrupt data be a warning rather than debug message, since it
> indicates something wrong with the transport?

Do you want messages like that spamming the kernel log all the time if a
network connection is corrupted?

Just handle the error and let the upper layers deal with it when the
problem is eventually reported to userspace, that's all that is needed.


> > > -	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?
> It seems that taking address of members of packed structures is not valid.

That feels really odd.

> I get the `address-of-packed-member` warnings. Is it fine to ignore
> those in kernel?

What error exactly are you getting?  Packed or not does not mean
anything to the address of a member.  If it does, perhaps you are doing
something wrong as you are really doing the same thing here, right?
Don't ignore the warning by open-coding it.

> > >   }
> > >   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?
> 
> Well, the HDLC payload is just to store the length of the payload along with
> a pointer to its data. (kind of emulate a fat pointer). The reason for doing
> it this way is to avoid having to create a temp buffer for each message when
> sending data over UART (which was done in the initial version of the
> driver).

Be careful, are you SURE you are allowed to send stack-allocated data?
I know that many busses forbid this (like USB).  So please check to be
sure that this is ok to do, and that these are not huge structures that
you are putting on the very-limited kernel-stack.

thanks,

greg k-h

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

[Ayush Singh]

On 12/8/23 11:03, Greg KH wrote:

> On Thu, Dec 07, 2023 at 10:33:54PM +0530, Ayush Singh wrote:
>>>> + *
>>>> + * @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?
>> Shouldn't corrupt data be a warning rather than debug message, since it
>> indicates something wrong with the transport?
> Do you want messages like that spamming the kernel log all the time if a
> network connection is corrupted?
>
> Just handle the error and let the upper layers deal with it when the
> problem is eventually reported to userspace, that's all that is needed.

Ok

>>>> -	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?
>> It seems that taking address of members of packed structures is not valid.
> That feels really odd.
>
>> I get the `address-of-packed-member` warnings. Is it fine to ignore
>> those in kernel?
> What error exactly are you getting?  Packed or not does not mean
> anything to the address of a member.  If it does, perhaps you are doing
> something wrong as you are really doing the same thing here, right?
> Don't ignore the warning by open-coding it.

So, the error I was getting was `taking address of packed member of 
'gb_frame' may result in an unaligned pointer value`. I can no longer 
reproduce the warning, though. I think I accidentally fixed the reason 
somewhere along the line.

>>>>    }
>>>>    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?
>> Well, the HDLC payload is just to store the length of the payload along with
>> a pointer to its data. (kind of emulate a fat pointer). The reason for doing
>> it this way is to avoid having to create a temp buffer for each message when
>> sending data over UART (which was done in the initial version of the
>> driver).
> Be careful, are you SURE you are allowed to send stack-allocated data?
> I know that many busses forbid this (like USB).  So please check to be
> sure that this is ok to do, and that these are not huge structures that
> you are putting on the very-limited kernel-stack.

Well, the greybus operation header and greybus message are not on the 
stack (as far as I know, since they are inputs to the function). The 
memory that is stack allocated for hdlc_payload array is `3 * 
(sizeof(u16) + sizeof(void *))`, i.e. 30 bytes for 64 bit pointers 
(ignoring any padding). So I don't think the size of hdlc_payload array 
should be an issue.

The function `hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 3);` 
actually copies the data to a different circ_buf, which is sent later by 
workqueue. So the final data sent is not stack allocated. In fact, none 
of the data from this function is going to be referenced after the call 
to `hdlc_tx_frames`.


Ayush Singh