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?
> > > + 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?
> > > + 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/hdlc_log_backend.c
Ok, but perhaps do something that doesn't need so much stack space just
for a debug message?
> > > + }
> > > +
> > > + 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 :)
thanks,
greg k-h
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|