[PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Ayush Singh]

Ensure that the following values are little-endian:
- header->pad (which is used for cport_id)
- header->size

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>
---
V3:
- Fix endiness while sending.
V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
- Ensure endianess for header->pad
V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/

 drivers/greybus/gb-beagleplay.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
index 43318c1993ba..8b21c3e1e612 100644
--- a/drivers/greybus/gb-beagleplay.c
+++ b/drivers/greybus/gb-beagleplay.c
@@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
 	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
 
 	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
-		hdr->operation_id, hdr->type, cport_id, hdr->result);
+		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
 
-	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
+	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
 }
 
 static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
@@ -340,14 +340,15 @@ static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_messa
 {
 	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
 	struct hdlc_payload payloads[2];
+	__le16 cport_id = cpu_to_le16(cport);
 
 	dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
 		msg->header->operation_id, msg->header->type, cport);
 
-	if (msg->header->size > RX_HDLC_PAYLOAD)
+	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, sizeof(cport));
+	memcpy(msg->header->pad, &cport_id, sizeof(cport_id));
 
 	payloads[0].buf = msg->header;
 	payloads[0].len = sizeof(*msg->header);
-- 
2.43.0

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Johan Hovold]

On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
> Ensure that the following values are little-endian:
> - header->pad (which is used for cport_id)
> - header->size
> 
> 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>
> ---
> V3:
> - Fix endiness while sending.
> V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
> - Ensure endianess for header->pad
> V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
> 
>  drivers/greybus/gb-beagleplay.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> index 43318c1993ba..8b21c3e1e612 100644
> --- a/drivers/greybus/gb-beagleplay.c
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>  	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>  
>  	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> -		hdr->operation_id, hdr->type, cport_id, hdr->result);
> +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>  
> -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
> +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);

This looks broken; a quick against mainline (and linux-next) check shows
cport_id to be u16.

I think you want get_unaligned_le16() or something instead of that
memcpy() above.

But that just begs the question: why has this driver repurposed the pad
bytes like this? The header still says that these shall be set to zero.

>  }
>  
>  static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> @@ -340,14 +340,15 @@ static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_messa
>  {
>  	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
>  	struct hdlc_payload payloads[2];
> +	__le16 cport_id = cpu_to_le16(cport);
>  
>  	dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
>  		msg->header->operation_id, msg->header->type, cport);
>  
> -	if (msg->header->size > RX_HDLC_PAYLOAD)
> +	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, sizeof(cport));
> +	memcpy(msg->header->pad, &cport_id, sizeof(cport_id));

put_unaligned_le16(), if the driver should be messing with the pad
bytes like this at all...

>  
>  	payloads[0].buf = msg->header;
>  	payloads[0].len = sizeof(*msg->header);

Johan

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Ayush Singh]

On 12/4/23 19:42, Johan Hovold wrote:
> On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
>> Ensure that the following values are little-endian:
>> - header->pad (which is used for cport_id)
>> - header->size
>>
>> 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>
>> ---
>> V3:
>> - Fix endiness while sending.
>> V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
>> - Ensure endianess for header->pad
>> V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
>>
>>   drivers/greybus/gb-beagleplay.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
>> index 43318c1993ba..8b21c3e1e612 100644
>> --- a/drivers/greybus/gb-beagleplay.c
>> +++ b/drivers/greybus/gb-beagleplay.c
>> @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>   	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>>   
>>   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>> -		hdr->operation_id, hdr->type, cport_id, hdr->result);
>> +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>>   
>> -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
>> +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> This looks broken; a quick against mainline (and linux-next) check shows
> cport_id to be u16.
>
> I think you want get_unaligned_le16() or something instead of that
> memcpy() above.
Thanks, will do.
>
> But that just begs the question: why has this driver repurposed the pad
> bytes like this? The header still says that these shall be set to zero.

So, the reason is multiplexing. The original gbridge setup used to do 
this, so I followed it when I moved gbridge to the coprocessor (during 
GSoC).

Using the padding for storing cport information allows not having to 
wrap the message in some other format at the two transport layers (UART 
and TCP sockets) beagle connect is using.This also seems better than 
trying to do something bespoke, especially since the padding bytes are 
not being used for anything else.

The initial spec was for project Ara (modular smartphone), so the 
current use for IoT is significantly different from the initial goals of 
the protocol. Maybe a future version of the spec can be more focused on 
IoT, but that will probably only happen once it has proven somewhat 
useful in this space.

Ayush Singh

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Greg KH]

On Mon, Dec 04, 2023 at 09:58:55PM +0530, Ayush Singh wrote:
> 
> On 12/4/23 19:42, Johan Hovold wrote:
> > On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
> > > Ensure that the following values are little-endian:
> > > - header->pad (which is used for cport_id)
> > > - header->size
> > > 
> > > 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>
> > > ---
> > > V3:
> > > - Fix endiness while sending.
> > > V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
> > > - Ensure endianess for header->pad
> > > V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
> > > 
> > >   drivers/greybus/gb-beagleplay.c | 9 +++++----
> > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> > > index 43318c1993ba..8b21c3e1e612 100644
> > > --- a/drivers/greybus/gb-beagleplay.c
> > > +++ b/drivers/greybus/gb-beagleplay.c
> > > @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
> > >   	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> > >   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> > > -		hdr->operation_id, hdr->type, cport_id, hdr->result);
> > > +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
> > > -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
> > > +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> > This looks broken; a quick against mainline (and linux-next) check shows
> > cport_id to be u16.
> > 
> > I think you want get_unaligned_le16() or something instead of that
> > memcpy() above.
> Thanks, will do.
> > 
> > But that just begs the question: why has this driver repurposed the pad
> > bytes like this? The header still says that these shall be set to zero.
> 
> So, the reason is multiplexing. The original gbridge setup used to do this,
> so I followed it when I moved gbridge to the coprocessor (during GSoC).
> 
> Using the padding for storing cport information allows not having to wrap
> the message in some other format at the two transport layers (UART and TCP
> sockets) beagle connect is using.This also seems better than trying to do
> something bespoke, especially since the padding bytes are not being used for
> anything else.
> 
> The initial spec was for project Ara (modular smartphone), so the current
> use for IoT is significantly different from the initial goals of the
> protocol. Maybe a future version of the spec can be more focused on IoT, but
> that will probably only happen once it has proven somewhat useful in this
> space.

Please don't violate the spec today this way, I missed that previously,
that's not ok.  We can change the spec for new things if you need it,
but to not follow it, and still say it is "greybus" isn't going to work
and will cause problems in the long-run.

Should I just disable the driver for now until this is fixed up?

thanks,

greg k-h

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Ayush Singh]

On 12/5/23 05:44, Greg KH wrote:
> On Mon, Dec 04, 2023 at 09:58:55PM +0530, Ayush Singh wrote:
>> On 12/4/23 19:42, Johan Hovold wrote:
>>> On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
>>>> Ensure that the following values are little-endian:
>>>> - header->pad (which is used for cport_id)
>>>> - header->size
>>>>
>>>> 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>
>>>> ---
>>>> V3:
>>>> - Fix endiness while sending.
>>>> V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
>>>> - Ensure endianess for header->pad
>>>> V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
>>>>
>>>>    drivers/greybus/gb-beagleplay.c | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
>>>> index 43318c1993ba..8b21c3e1e612 100644
>>>> --- a/drivers/greybus/gb-beagleplay.c
>>>> +++ b/drivers/greybus/gb-beagleplay.c
>>>> @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>>>    	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>>>>    	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>>>> -		hdr->operation_id, hdr->type, cport_id, hdr->result);
>>>> +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>>>> -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
>>>> +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
>>> This looks broken; a quick against mainline (and linux-next) check shows
>>> cport_id to be u16.
>>>
>>> I think you want get_unaligned_le16() or something instead of that
>>> memcpy() above.
>> Thanks, will do.
>>> But that just begs the question: why has this driver repurposed the pad
>>> bytes like this? The header still says that these shall be set to zero.
>> So, the reason is multiplexing. The original gbridge setup used to do this,
>> so I followed it when I moved gbridge to the coprocessor (during GSoC).
>>
>> Using the padding for storing cport information allows not having to wrap
>> the message in some other format at the two transport layers (UART and TCP
>> sockets) beagle connect is using.This also seems better than trying to do
>> something bespoke, especially since the padding bytes are not being used for
>> anything else.
>>
>> The initial spec was for project Ara (modular smartphone), so the current
>> use for IoT is significantly different from the initial goals of the
>> protocol. Maybe a future version of the spec can be more focused on IoT, but
>> that will probably only happen once it has proven somewhat useful in this
>> space.
> Please don't violate the spec today this way, I missed that previously,
> that's not ok.  We can change the spec for new things if you need it,
> but to not follow it, and still say it is "greybus" isn't going to work
> and will cause problems in the long-run.
>
> Should I just disable the driver for now until this is fixed up?
>
> thanks,
>
> greg k-h

Well, I will look into some ways to pass along the cport information 
(maybe using a wrapper over greybus message) for now. However, I would 
prefer having some bytes in greybus messages reserved for passing around 
this information in a transport agnostic way.

Ayush Singh

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Greg KH]

On Tue, Dec 05, 2023 at 05:15:33PM +0530, Ayush Singh wrote:
> 
> On 12/5/23 05:44, Greg KH wrote:
> > On Mon, Dec 04, 2023 at 09:58:55PM +0530, Ayush Singh wrote:
> > > On 12/4/23 19:42, Johan Hovold wrote:
> > > > On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
> > > > > Ensure that the following values are little-endian:
> > > > > - header->pad (which is used for cport_id)
> > > > > - header->size
> > > > > 
> > > > > 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>
> > > > > ---
> > > > > V3:
> > > > > - Fix endiness while sending.
> > > > > V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
> > > > > - Ensure endianess for header->pad
> > > > > V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
> > > > > 
> > > > >    drivers/greybus/gb-beagleplay.c | 9 +++++----
> > > > >    1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> > > > > index 43318c1993ba..8b21c3e1e612 100644
> > > > > --- a/drivers/greybus/gb-beagleplay.c
> > > > > +++ b/drivers/greybus/gb-beagleplay.c
> > > > > @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
> > > > >    	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> > > > >    	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> > > > > -		hdr->operation_id, hdr->type, cport_id, hdr->result);
> > > > > +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
> > > > > -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
> > > > > +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> > > > This looks broken; a quick against mainline (and linux-next) check shows
> > > > cport_id to be u16.
> > > > 
> > > > I think you want get_unaligned_le16() or something instead of that
> > > > memcpy() above.
> > > Thanks, will do.
> > > > But that just begs the question: why has this driver repurposed the pad
> > > > bytes like this? The header still says that these shall be set to zero.
> > > So, the reason is multiplexing. The original gbridge setup used to do this,
> > > so I followed it when I moved gbridge to the coprocessor (during GSoC).
> > > 
> > > Using the padding for storing cport information allows not having to wrap
> > > the message in some other format at the two transport layers (UART and TCP
> > > sockets) beagle connect is using.This also seems better than trying to do
> > > something bespoke, especially since the padding bytes are not being used for
> > > anything else.
> > > 
> > > The initial spec was for project Ara (modular smartphone), so the current
> > > use for IoT is significantly different from the initial goals of the
> > > protocol. Maybe a future version of the spec can be more focused on IoT, but
> > > that will probably only happen once it has proven somewhat useful in this
> > > space.
> > Please don't violate the spec today this way, I missed that previously,
> > that's not ok.  We can change the spec for new things if you need it,
> > but to not follow it, and still say it is "greybus" isn't going to work
> > and will cause problems in the long-run.
> > 
> > Should I just disable the driver for now until this is fixed up?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Well, I will look into some ways to pass along the cport information (maybe
> using a wrapper over greybus message) for now. However, I would prefer
> having some bytes in greybus messages reserved for passing around this
> information in a transport agnostic way.

I'm confused, what exactly is needed here to be sent that isn't in the
existing message definition.

And as to your original statement, the protocol definition was not
designed for any specific use case that would make IoT "special" here
that I can see.  It was designed to provide a discoverable way to
describe and control hardware on an unknown transport layer for devices
that are not discoverable by definition (serial, i2c, etc.)

The fact that we implemented this on both USB and unipro successfully
provided that the transport layer for the data should be working and
agnositic.

thanks,

greg k-h

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Ayush Singh]

On 12/6/23 01:15, Greg KH wrote:

> I'm confused, what exactly is needed here to be sent that isn't in the
> existing message definition.
>
> And as to your original statement, the protocol definition was not
> designed for any specific use case that would make IoT "special" here
> that I can see.  It was designed to provide a discoverable way to
> describe and control hardware on an unknown transport layer for devices
> that are not discoverable by definition (serial, i2c, etc.)
>
> The fact that we implemented this on both USB and unipro successfully
> provided that the transport layer for the data should be working and
> agnositic.
>
> thanks,
>
> greg k-h

So, the missing information is the AP cport which is sending the 
message/for which the message is intended. Each AP cport will be 
connected to a cport in some greybus node. For a simple case like USB, 
where AP can directly talk to the node, and we do not really need the 
cport information outside of kernel driver.

I think under normal circumstances, the kernel driver is supposed to 
directly communicate with the node. However, in beagle play, the subghz 
transport is only present in CC1352 coprocessor. This means CC1352 needs 
to act as the middle man between AP and node (aka perform the APBridge 
tasks). So it needs to maintain a way to keep track of all active 
greybus connections, and route the messages between AP and Node cports.

I am not quite sure where SVC is supposed to be in Linux kernel greybus 
setup. Since SVC needs to be able to detect module insertion/removal, it 
needs to be able to access the same transport as APBridge. Thus, CC1352 
(and gbridge in old setup) are responsible for both SVC and APBridge roles.

Simply put, if the kernel driver cannot directly connect to the node, 
the processor / network entity handling APBridge tasks will need to 
cport information. And it probably is good to make it possible to 
separate APBridge from AP in complex networks.

Feel free to ask questions if I was unclear regarding something. Also 
feel free to correct me if I got something wrong since I only started 
working on greybus this summer.

Ayush Singh

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Alex Elder]

On 12/5/23 2:32 PM, Ayush Singh wrote:
> On 12/6/23 01:15, Greg KH wrote:
> 
>> I'm confused, what exactly is needed here to be sent that isn't in the
>> existing message definition.
>>
>> And as to your original statement, the protocol definition was not
>> designed for any specific use case that would make IoT "special" here
>> that I can see.  It was designed to provide a discoverable way to
>> describe and control hardware on an unknown transport layer for devices
>> that are not discoverable by definition (serial, i2c, etc.)
>>
>> The fact that we implemented this on both USB and unipro successfully
>> provided that the transport layer for the data should be working and
>> agnositic.
>>
>> thanks,
>>
>> greg k-h
> 
> So, the missing information is the AP cport which is sending the 
> message/for which the message is intended. Each AP cport will be 
> connected to a cport in some greybus node. For a simple case like USB, 
> where AP can directly talk to the node, and we do not really need the 
> cport information outside of kernel driver.

I think I lack some context here, but as Greg said Greybus
is intended to be a pure transport, and anything using it
should be able to get all information it needs as a layered
protocol on top of it.

If the BeaglePlay stuff requires CPort information, it sounds
like it's not managing the layering of abstractions properly.

> I think under normal circumstances, the kernel driver is supposed to 
> directly communicate with the node. However, in beagle play, the subghz 
> transport is only present in CC1352 coprocessor. This means CC1352 needs 
> to act as the middle man between AP and node (aka perform the APBridge 
> tasks). So it needs to maintain a way to keep track of all active 
> greybus connections, and route the messages between AP and Node cports.
> 
> I am not quite sure where SVC is supposed to be in Linux kernel greybus 
> setup. Since SVC needs to be able to detect module insertion/removal, it 
> needs to be able to access the same transport as APBridge. Thus, CC1352 
> (and gbridge in old setup) are responsible for both SVC and APBridge roles.

It sounds like CC1352 is serving in an SVC role... sort of?  Again, I 
don't have enough context right now to understand.

Greybus was developed for a particular hardware platform, and it
included an SVC.  The SVC was an independent processor that managed
the "endo", or the basic hardware "backplane" that held modules).
The AP bridge was how the AP connected to that, and the GP bridge
was how a given module interface connected to that.

It seems to me (this is partly from an impression I had a few years
ago) that the BeaglePlay model doesn't align perfectly with that.
And if that's the case, we need to figure out how to resolve any
mismatches.

(I'm not sure this is very helpful, but it's a little background.)

					-Alex

> Simply put, if the kernel driver cannot directly connect to the node, 
> the processor / network entity handling APBridge tasks will need to 
> cport information. And it probably is good to make it possible to 
> separate APBridge from AP in complex networks.
> 
> Feel free to ask questions if I was unclear regarding something. Also 
> feel free to correct me if I got something wrong since I only started 
> working on greybus this summer.
> 
> Ayush Singh
> 

Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

[Ayush Singh]

On 12/6/23 20:22, Alex Elder wrote:
> On 12/5/23 2:32 PM, Ayush Singh wrote:
>> On 12/6/23 01:15, Greg KH wrote:
>>
>>> I'm confused, what exactly is needed here to be sent that isn't in the
>>> existing message definition.
>>>
>>> And as to your original statement, the protocol definition was not
>>> designed for any specific use case that would make IoT "special" here
>>> that I can see.  It was designed to provide a discoverable way to
>>> describe and control hardware on an unknown transport layer for devices
>>> that are not discoverable by definition (serial, i2c, etc.)
>>>
>>> The fact that we implemented this on both USB and unipro successfully
>>> provided that the transport layer for the data should be working and
>>> agnositic.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> So, the missing information is the AP cport which is sending the 
>> message/for which the message is intended. Each AP cport will be 
>> connected to a cport in some greybus node. For a simple case like 
>> USB, where AP can directly talk to the node, and we do not really 
>> need the cport information outside of kernel driver.
>
> I think I lack some context here, but as Greg said Greybus
> is intended to be a pure transport, and anything using it
> should be able to get all information it needs as a layered
> protocol on top of it.
>
> If the BeaglePlay stuff requires CPort information, it sounds
> like it's not managing the layering of abstractions properly.
Well, I used gbridge as a reference during my GSoC work. So I just 
followed it's lead of using pad bytes for cport information.
>
>> I think under normal circumstances, the kernel driver is supposed to 
>> directly communicate with the node. However, in beagle play, the 
>> subghz transport is only present in CC1352 coprocessor. This means 
>> CC1352 needs to act as the middle man between AP and node (aka 
>> perform the APBridge tasks). So it needs to maintain a way to keep 
>> track of all active greybus connections, and route the messages 
>> between AP and Node cports.
>>
>> I am not quite sure where SVC is supposed to be in Linux kernel 
>> greybus setup. Since SVC needs to be able to detect module 
>> insertion/removal, it needs to be able to access the same transport 
>> as APBridge. Thus, CC1352 (and gbridge in old setup) are responsible 
>> for both SVC and APBridge roles.
>
> It sounds like CC1352 is serving in an SVC role... sort of? Again, I 
> don't have enough context right now to understand.
>
> Greybus was developed for a particular hardware platform, and it
> included an SVC.  The SVC was an independent processor that managed
> the "endo", or the basic hardware "backplane" that held modules).
> The AP bridge was how the AP connected to that, and the GP bridge
> was how a given module interface connected to that.
>
> It seems to me (this is partly from an impression I had a few years
> ago) that the BeaglePlay model doesn't align perfectly with that.
> And if that's the case, we need to figure out how to resolve any
> mismatches.
>
> (I'm not sure this is very helpful, but it's a little background.)
>
>                     -Alex

Yes, the BeaglePlay (and older gbridge) model does deviate from that. 
You can read more about beagle connect technology here [1] and the 
initial presentation [2].

However, to put it simply, it is trying to use greybus over transports 
other than Unipro. This means we do not have a Unipro switch. Instead, 
we use a coprocessor (CC1352) running specialized firmware to handle all 
the things Unipro switch would.

The current focus is 6lowpan (due to it's range). However, CC1352 also 
has a 2.4 and 5 ghz antenna, so in the future, that might also be used 
for transportation.

Since I am not much aware of greybus use outside of beagle connect, I do 
not have much knowledge of how it is supposed to be used in a 
traditional setting.

I have submitted new patches that remove the need for using pad bytes.


Ayush Singh


[1]: https://docs.beagleboard.org/latest/projects/beagleconnect/index.html

[2]: https://www.youtube.com/watch?v=7H50pv-4YXw