Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 9/6/23 15:29, Krzysztof Kozlowski wrote:

On 05/09/2023 18:27, Ayush Singh wrote:
+static void hdlc_handle_rx_frame(struct gb_beagleplay *bg)
+	u8 address = bg->rx_buffer[0];
+	char *buffer = &bg->rx_buffer[2];
+	size_t buffer_len = bg->rx_buffer_len - 4;
+	switch (address) {
+		hdlc_handle_dbg_frame(bg, buffer, buffer_len);
+		break;
+		hdlc_handle_greybus_frame(bg, buffer, buffer_len);
+		break;
+	default:
+		dev_warn(&bg->serdev->dev, "Got Unknown Frame %u", address);
Probably as well in several places with possible flooding.
I don't think `hdlc_handle_rx_frame` is the correct place since it only
processes a single completed HDLC frame.  The more appropriate place
would be `hdlc_rx` if we want to limit based on the number of HDLC
frames or `gb_beagleplay_tty_receive` to limit based on the number of bytes.

I would like to ask, though, why is rate limiting required here? Won't
`serdev_device_ops->receive_buf` already rate limit the number of bytes
somewhat? Or is it related to blocking in the
`serdev_device_ops->receive_buf` callback? In the case of latter, it
would probably make sense to ratelimit based on number of frames, I think.
My comment might not be accurate, so I do not insist. The name of the
function suggested something being called very often (on every frame),
thus you would print warning also very often.

Best regards,

Rate limiting the logs is not a bad idea. Initially I was not aware you meant about logging, hence the question. With proper firmware in CC1352, the warning will never be printed. But maybe it can cause problem with improper firmware.

Ayush Singh

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

[Index of Archives]     [Asterisk App Development]     [PJ SIP]     [Gnu Gatekeeper]     [IETF Sipping]     [Info Cyrus]     [ALSA User]     [Fedora Linux Users]     [Linux SCTP]     [DCCP]     [Gimp]     [Yosemite News]     [Deep Creek Hot Springs]     [Yosemite Campsites]     [ISDN Cause Codes]     [Asterisk Books]

  Powered by Linux