Re: [PATCH 2/4] Add beagleplay greybus driver

[Ayush Singh <ayushdevel1325@xxxxxxxxx>]

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/hdlc_log_backend.c


> > +	}
> > +
> > +	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

_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx