Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <4cafbb5a-8ecd-407e-81a0-76d6505d013b@gmail.com>
Date:   Tue, 5 Dec 2023 17:15:33 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in
 transport
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Johan Hovold <johan@kernel.org>, greybus-dev@lists.linaro.org,
        elder@kernel.org, linux-kernel@vger.kernel.org,
        jkridner@beagleboard.org, kernel test robot <yujie.liu@intel.com>
References: <20231204131008.384583-1-ayushdevel1325@gmail.com>
 <ZW3ePt-c4Mu43DOV@hovoldconsulting.com>
 <7ead544b-9234-483f-aacb-55ed05b01fa3@gmail.com>
 <2023120515-mongrel-undertook-6e5a@gregkh>
Content-Language: en-US
From:   Ayush Singh <ayushdevel1325@gmail.com>
In-Reply-To: <2023120515-mongrel-undertook-6e5a@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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