Re: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters

[Vincent Mailhol]

On Wed. 9 Mar 2022 at 21:54, Max Staudt <max@xxxxxxxxx> wrote:
> Thanks a lot Vincent for your review!
>
> Most points are self explanatory, for the others I've added replies
> below.
>
>
>
> On Tue, 8 Mar 2022 16:01:12 +0900
> Vincent Mailhol <vincent.mailhol@xxxxxxxxx> wrote:
>
> > Hi Max, this is a partial review.
> >
> > > +/* Bits in elm->cmds_todo */
> > > +enum ELM_TODO {
> > > +       TODO_CAN_DATA = 0,
> > > +       TODO_CANID_11BIT,
> > > +       TODO_CANID_29BIT_LOW,
> > > +       TODO_CANID_29BIT_HIGH,
> > > +       TODO_CAN_CONFIG_PART2,
> > > +       TODO_CAN_CONFIG,
> > > +       TODO_RESPONSES,
> > > +       TODO_SILENT_MONITOR,
> > > +       TODO_INIT
> >
> > Nitpick but the TODO name is bugging me. What does this acronym mean?
> > Is it possible to change this so it doesn't look like a FIXME tag?
>
> Good point, I'll change it.
>
> It's an ordered list of things to send next to the adapter.

Then TX_QUEUE or TX_FIFO sounds like a better name then.

> For
> example, whenever the sending CAN ID needs to be changed, the relevant
> TODO_* bits are set, and the new CAN ID is sent down the UART before a
> payload (*_DATA) is ever sent.
>

Also, prefix the enum entries with your module name.
e.g. ELM327_

>
> > > +       frame.can_id = CAN_ERR_FLAG;
> > > +       frame.can_dlc = CAN_ERR_DLC;
> > > +       frame.data[5] = 'R';
> > > +       frame.data[6] = 'I';
> > > +       frame.data[7] = 'P';
> > > +       elm327_feed_frame_to_netdev(elm, &frame);
> >
> > There is a framework to notify a bus off. Refer to:
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_core.c#L815
>
> Thanks, will do.
>
>
> > > +/* Compare a buffer to a fixed string */
> > > +static inline int _memstrcmp(const u8 *mem, const char *str)
> > > +{
> > > +       return memcmp(mem, str, strlen(str));
> >
> > strcpy()?

I think you got it but I meant strcmp()…

> > Did you check for buffer overflow?
>
> There is no buffer overflow, as this only ever takes string constants
> as *str. The compiler figures out the strlen() and can generate an
> optimised memcmp() for this given string length.

Are you sure that the compiler does not already produce optimized
code for strcmp()? Did you check the assembly output?

> It's the caller's job to ensure that *mem is large enough.
>
>
> > > +
> > > +       /* Use spaces in CAN ID to distinguish 29 or 11 bit address
> > > length.
> > > +        * No out-of-bounds access:
> > > +        * We use the fact that we can always read from elm->rxbuf.
> > > +        */
> > > +       if (elm->rxbuf[2] == ' ' && elm->rxbuf[5] == ' ' &&
> > > +           elm->rxbuf[8] == ' ' && elm->rxbuf[11] == ' ' &&
> > > +           elm->rxbuf[13] == ' ') {
> >
> > Define an inline function elm327_is_eff().
>
> It would only be used this one time, so I don't see the utility? It'd
> just make it harder to read, IMHO.
>
> It's ASCII lexer/parser code, so it's bound to be ugly... :(

That comment was a nitpick, and it is about colors and taste. So
I won't insist more.

>
> > > +       /* Read CAN ID */
> > > +       if (frame.can_id & CAN_EFF_FLAG) {
> > > +               frame.can_id |= (hex_to_bin(elm->rxbuf[0]) << 28)
> > > +                             | (hex_to_bin(elm->rxbuf[1]) << 24)
> > > +                             | (hex_to_bin(elm->rxbuf[3]) << 20)
> > > +                             | (hex_to_bin(elm->rxbuf[4]) << 16)
> > > +                             | (hex_to_bin(elm->rxbuf[6]) << 12)
> > > +                             | (hex_to_bin(elm->rxbuf[7]) << 8)
> > > +                             | (hex_to_bin(elm->rxbuf[9]) << 4)
> > > +                             | (hex_to_bin(elm->rxbuf[10]) << 0);
> > > +       } else {
> > > +               frame.can_id |= (hex_to_bin(elm->rxbuf[0]) << 8)
> > > +                             | (hex_to_bin(elm->rxbuf[1]) << 4)
> > > +                             | (hex_to_bin(elm->rxbuf[2]) << 0);
> >
> > hex2bin()?
>
> Good idea!
>
>
> > > +       /* Parse the data nibbles. */
> > > +       for (i = 0; i < frame.can_dlc; i++) {
> >
> > frame.can_dlc is deprecated. Use frame.len instead.
>
> Thanks!
>
>
> [ ... snip self explanatory stuff ... ]
>
>
> > > +       case ELM_RECEIVING:
> > > +               /* Find <CR> delimiting feedback lines. */
> > > +               for (len = 0;
> > > +                    (len < elm->rxfill) && (elm->rxbuf[len] !=
> > > '\r');
> >
> > Did you use ./script/checkpath?
>
> checkpatch? Yes I did (and kudos to whoever wrote it).
>
> Why?

Because I thought that checkpath would have spotted some
unnecessary parentheses... But I was wrong.

For reference, if put in an "if" statement, you would have got this output:

CHECK: Unnecessary parentheses around 'len < elm->rxfill'
+    if ((len < elm->rxfill) && (elm->rxbuf[len] != '\r')) {

I was expecting the for loop to yield the same.

>
> > > +/* Dummy needed to use can_rx_offload */
> > > +static struct sk_buff *elmcan_mailbox_read(struct can_rx_offload
> > > *offload,
> > > +                                          unsigned int n, u32
> > > *timestamp,
> > > +                                          bool drop)
> > > +{
> > > +       WARN_ON_ONCE(1); /* This function is a dummy, so don't call
> > > it! */ +
> > > +       return ERR_PTR(-ENOBUFS);
> > > +}
> >
> > Could you elaborate on why you need can_rx_offload if the mailbox
> > feature is not needed?
>
> The code previously used netif_rx_ni(), and Marc noted that it may
> reorder packets. To avoid that, he suggested rx_offload:
>
>   Message-ID: 88c08b2c-aa4a-8858-6267-deeeac2796df@xxxxxxxxxxxxxx
>
>   https://www.spinics.net/lists/linux-can/msg01859.html
>
>
> But rx_offload needs the mailbox_read function, even if it's a dummy,
> because can_rx_offload_add_fifo() checks:
>
>         if (!offload->mailbox_read)
>                 return -EINVAL;

I think that there should not be a dummy functions like this one.

Either we agree that using can_rx_offload without implementing
the mailbox_read() is OK and in that case, the can_rx_offload
framework should be modified to allow mailbox_read() to be a NULL
pointer.

Either it is not the case and you use the more classic
netif_rx().

And I do not have the answer. I haven't studied can_rx_offload
enough to be a judge here. Sorry.

@Marc, any thoughts?

>
> > > +/* Send a can_frame to a TTY. */
> > > +static netdev_tx_t elmcan_netdev_start_xmit(struct sk_buff *skb,
> > > +                                           struct net_device *dev)
> > > +{
> > > +       struct elmcan *elm = netdev_priv(dev);
> > > +       struct can_frame *frame = (struct can_frame *)skb->data;
> > > +
> > > +       if (skb->len != sizeof(struct can_frame))
> > > +               goto out;
> >
> > Isn’t this aleardy guaranteed by the upper layers?
>
> Copy-pasta from slcan.c - will look into it.

Also give a look at can_dropped_invalid_skb().

>
> > > +       if (!netif_running(dev))  {
> > > +               netdev_warn(elm->dev, "xmit: iface is down.\n");
> > > +               goto out;
> > > +       }
> >
> > Even if this check succeeds, interface might still go down at the
> > next cycle. What is the purpose of checking if interface is up here?
>
> No purpose. It's copy-pasta from slip.c via slcan.c.