Messages in this thread | | | Date | Thu, 7 Dec 2023 22:33:54 +0530 | Subject | Re: [PATCH 1/1] greybus: gb-beagleplay: Remove use of pad bytes | From | Ayush Singh <> |
| |
>> + * >> + * @cport: cport id >> + * @hdr: greybus operation header >> + * @payload: greybus message payload >> + */ >> +struct hdlc_greybus_frame { >> + __le16 cport; >> + struct gb_operation_msg_hdr hdr; >> + u8 payload[]; >> +} __packed; >> + >> static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len) >> { >> - u16 cport_id; >> - struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf; >> + struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf; >> + u16 cport_id = le16_to_cpu(gb_frame->cport); >> >> - memcpy(&cport_id, hdr->pad, sizeof(cport_id)); >> + /* Ensure that the greybus message is valid */ >> + if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) { >> + dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message"); > Don't spam the kernel log for corrupted data on the line, that would be > a mess. Use a tracepoint? > >> + return; >> + } >> >> dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received", >> - hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result); >> + gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result); > Better yet, put the error in the debug message? Shouldn't corrupt data be a warning rather than debug message, since it indicates something wrong with the transport? >> >> - greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len); >> + greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)], > Fun with pointer math. This feels really fragile, why not just point to > the field instead? It seems that taking address of members of packed structures is not valid. I get the `address-of-packed-member` warnings. Is it fine to ignore those in kernel? >> } >> >> static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len) >> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = { >> static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask) >> { >> struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev); >> - struct hdlc_payload payloads[2]; >> + struct hdlc_payload payloads[3]; > why 3? > > It's ok to put this on the stack?
Well, the HDLC payload is just to store the length of the payload along with a pointer to its data. (kind of emulate a fat pointer). The reason for doing it this way is to avoid having to create a temp buffer for each message when sending data over UART (which was done in the initial version of the driver).
Ayush Singh
| |