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?
- 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.
- 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/...
- }
- 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
+static void beagleplay_greybus_start_svc(struct beagleplay_greybus *bg) +{
- const u8 command[1] = { CONTROL_SVC_START };
Are you sure this is allowed on the stack?
Sorry, that should just be an u8 instead of u8 array. Will fix it.
- }
- ret = gb_hd_add(bg->gb_host_device);
- if (ret) {
dev_err(&serdev->dev, "Failed to add Greybus Host Device");return ret;Did you just leak memory?
Good catch
Thanks for the feedback. I will send a new patch once everything mentioned is fixed and tested.
Ayush Singh