On 8/25/23 17:08, Greg KH wrote:
On Fri, Aug 25, 2023 at 04:53:01PM +0530, Ayush Singh wrote:
On 8/23/23 23:06, Greg KH wrote:
+#define BEAGLEPLAY_GB_MAX_CPORTS 32
Where does this number come from?
Well, it is the max number of Cports our APBridge supports. Since it is a KConfig variable on Zephyr application side, maybe it should be a config variable here as well?
Just document it please. But if this gets out of sync with the device (as Linux has no idea what the device has), perhaps you should be able to detect it automatically?
Let me see what I can do about the detection. There is no mechanism in greybus to do that, so it will need to be something bespoke I guess.
- if (hdr->result) {
dev_warn(&bg->serdev->dev,"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",hdr->operation_id, hdr->type, cport_id, hdr->result);- } else {
dev_dbg(&bg->serdev->dev,"Successful Greybus Operation %u of Type %X on CPort %u",hdr->operation_id, hdr->type, cport_id);- }
- greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,
const char *buffer,size_t buffer_len)+{
- char buf[RX_HDLC_PAYLOAD];
Are you sure you can do all of that on the stack?
I think it should be fine. Zephyr application itself places a limit on the maximum size of an HDLC frame and compile time, and we will only process a single frame in this function. I do need to clean up some of these constants. And the zephyr application only supports a max frame of 256 bytes, so the current buffer is way too big.
Again, plutting that much data on the stack is generally a bad idea. Also, are you sure the hardware can copy from the stack properly? Many bus types can not handle this at all (i.e. USB), so can you just make it dynamic? Or is it needed at all?
I can shift it to heap. The reason I am copying it here is that the HDLC frame payload is not null terminated. If it was possible to supply the length of the string buffer without null terminating it, we could remove the whole copying.
I think I can actually just NULL terminate the supplied buffer without creating any problem in the current setup (it will just override CRC which is not going to be checked again). Let me try that.
Also, the data is being copied from a heap array (`beagleplay_greybus->rx_buffer`), not any hardware directly.
- memcpy(buf, buffer, buffer_len);
- buf[buffer_len] = 0;
- dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);
Why are you using a stack buffer for a debug log?
And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.
and are you sure this buffer is a string?
This is printing logs from BeaglePlay CC1352, which are transported over a specific HDLC address. This is because unless you have a JTAG, you cannot view anything happening in CC1352 without disabling the driver using dt overlay. This is incontinent for development and debugging.
It should always be a string since I am routing the zephyr log outputs over hdlc: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware/-/blob/develop/src/...
Ok, but perhaps do something that doesn't need so much stack space just for a debug message?
Well, a debug frame will contain min(HDLC_FRAME_SIZE, DEBUG_MSG_SIZE). And I only know HDLC_FRAME_SIZE at compile time. I can allocate a heap array for every debug message (to make it the size of the actual message), but well I think it would make more sense to just allocate once and reuse it.
- }
- beagleplay_greybus = dev_get_drvdata(&hd->dev);
- memcpy(message->header->pad, &cport_id, sizeof(u16));
- memcpy(&block_payload, message->header,
sizeof(struct gb_operation_msg_hdr));- memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],
message->payload, message->payload_size);- hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,
&block_payload, ADDRESS_GREYBUS, 0x03);- greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
- return 0;
+}
+static void gb_message_cancel(struct gb_message *message) +{ +}
Why is an empty function needed? That feels wrong.
Well, this is a required function to define: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/greybus/hd.c#L136
If it's required, an empty function is not allowed, right? Otherwise the core would allow an empty function :)
Well, the empty message was taken from `gb_netlink`. Anyway, I am not quite sure what the expected behavior of this function. A greybus message can be in one of the following stages:
1. Queued for sending over UART but still not left linux driver.
2. Sent to CC1352, but not sent to Greybus node yet.
3. Sent to greybus node,
If you add the state of response message as well, then you get 3 more for that.
Anyway, does cancel just mean that it does not want to receive a response for this message, or does it actively want to stop the message from being sent. In the former case, I can just maintain a list of cancelled messages and check each incoming message against it. This would mean the message will always be sent to node anyway and just not submitted back to `gb_host_device`.
The latter is much more difficult after stage 1. The APBridge on CC1352 is dumb by design and does not keep track of messages it send or receives. Making it keep track of every message from every direction will add a lot more complexity and cause performance problems (since it is just 1 core doing APBridge, SVC and node discovery stuff).
Ayush Singh