On Thu, Dec 07, 2023 at 10:33:54PM +0530, Ayush Singh wrote:
> > > + *
> > > + * @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?
Do you want messages like that spamming the kernel log all the time if a
network connection is corrupted?
Just handle the error and let the upper layers deal with it when the
problem is eventually reported to userspace, that's all that is needed.
> > > - 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.
That feels really odd.
> I get the `address-of-packed-member` warnings. Is it fine to ignore
> those in kernel?
What error exactly are you getting? Packed or not does not mean
anything to the address of a member. If it does, perhaps you are doing
something wrong as you are really doing the same thing here, right?
Don't ignore the warning by open-coding it.
> > > }
> > > 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).
Be careful, are you SURE you are allowed to send stack-allocated data?
I know that many busses forbid this (like USB). So please check to be
sure that this is ok to do, and that these are not huge structures that
you are putting on the very-limited kernel-stack.
thanks,
greg k-h
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|