lkml.org 
[lkml]   [2022]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters
    On 17.03.2022 21:18:22, Max Staudt wrote:
    > > > +/* Bits in elm->cmds_todo */
    > > > +enum ELM_TODO {
    > > ^^^^^^^^
    > > small caps please, and Vincent alreadt commented on the name.
    >
    > Small caps? Sorry, that's not possible in plain ASCII.
    > You probably mean something else, but I'm not sure what?

    I meant to say lowercase, sorry for the confusion.

    [...]

    > > > + /* Regular parsing */
    > > > + switch (elm->state) {
    > > > + case ELM_RECEIVING:
    > > > + if (elm327_parse_frame(elm, len)) {
    > > > + /* Parse an error line. */
    > > > + elm327_parse_error(elm, len);
    > > > +
    > > > + /* Start afresh. */
    > > > + elm327_kick_into_cmd_mode(elm);
    > > > + }
    > > > + break;
    > > > + default:
    > > > + break;
    > > > + }
    > > > +}
    > > > +
    > > > +/* Assumes elm->lock taken. */
    > > > +static void elm327_handle_prompt(struct elmcan *elm)
    > > > +{
    > > > + struct can_frame *frame = &elm->can_frame;
    > > > + char local_txbuf[20];
    > >
    > > How can you be sure, that the local_txbuf is large enough?
    >
    > It's filled in this very same function, with sprintf() or a strcpy()
    > from one of the short strings in elm327_init_script (see next quote
    > below). I've calculated the maximum length that can occur out of all
    > these possibilities in the current code, and set that as the length of
    > local_txbuf.

    You can use something like "local_txbuf[sizeof("ATZ;ATDT0815;ATH")]"
    with the longest ATZ command you can produce here.

    > > > + /* Reconfigure ELM327 step by step as indicated by
    > > > elm->cmds_todo */
    > > > + if (test_bit(TODO_INIT, &elm->cmds_todo)) {
    > > > + strcpy(local_txbuf, *elm->next_init_cmd);
    > >
    > > strncpy()
    >
    > For this, there would have to be an entry in elm327_init_script that is
    > longer than sizeof(local_txbuf) - 1. I highly doubt there ever will be,
    > and even if someone does come up with one (maybe a huge new command in a
    > future ELM327 revision), then strncpy would silently cut off the end and
    > induce unexpected failure. Most importantly, this failure would be
    > silent - the driver doesn't check the ELM's responses by design!
    >
    > I suggest an assert here. How about something like this?
    >
    > if (strlen(*elm->next_init_cmd) < sizeof(local_txbuf))
    > strcpy(local_txbuf, *elm->next_init_cmd);
    > else
    > WARN_ONCE(...)
    >
    > If elm327_init_script contains an item longer than this buffer, then
    > the buffer size needs to be increased. Simple programming error IMHO.
    > I'd also add a comment to state this, next to elm327_init_script.
    >
    > What do you think?

    You can use BUILD_BUG_ON() (see linux/build_bug.h) inside your C
    function to make a compile time check, or static_assert() outside of C
    functions.

    > > > + } else if (test_and_clear_bit(TODO_SILENT_MONITOR,
    > > > &elm->cmds_todo)) {
    > > > + sprintf(local_txbuf, "ATCSM%i\r",
    > > > + !(!(elm->can.ctrlmode &
    > > > CAN_CTRLMODE_LISTENONLY)));
    > >
    > > snprintf()
    >
    > See above. This size is predictable, and used to size local_txbuf.
    >
    > Thinking about it, since this size is easily predictable, the compiler
    > could also do it, and that would turn snprintf() into a compile time
    > check.
    >
    > Unfortunately I couldn't make GCC shout at me for giving snprintf() too
    > small a buffer to fit all possible expansions of this format string. Is
    > this even possible?

    In user space, I've seen warnings like that, not sure about the kernel.

    > > > +static int elmcan_netdev_open(struct net_device *dev)
    > > > +{
    > > > + struct elmcan *elm = netdev_priv(dev);
    > > > + int err;
    > > > +
    > > > + spin_lock_bh(&elm->lock);
    > > > + if (elm->hw_failure) {
    > > > + netdev_err(elm->dev, "Refusing to open interface
    > > > after a hardware fault has been detected.\n");
    > > > + spin_unlock_bh(&elm->lock);
    > > > + return -EIO;
    > > > + }
    > >
    > > How to recover from this error?
    >
    > The user can detach and reattach the ldisc as often as desired.
    >
    > There is currently no intention to recover automatically. Once
    > elm->hw_failure is set, something really weird must have happened such
    > as unexpected characters on the UART. Since these devices are usually a
    > PIC right next to a UART-USB bridge chip, which is why I deem this
    > indicative of hardware too faulty to be trusted in any way.
    >
    > Regular "expected" errors are parsed and dealt with by sending error
    > frames in elm327_parse_error(). These do not trigger hw_failure.

    Ok, in other drivers I usually do a full reset during an ifdown/ifup
    cycle....at least for non hot plug-able devices.

    > > > + elm->txbuf = kmalloc(ELM327_SIZE_TXBUF, GFP_KERNEL);
    > >
    > > Why do you allocate an extra buffer?
    >
    > If I remember correctly, I was told that this is preferred because
    > drivers can DMA out of the aligned buffer. I didn't question that. I
    > can simply allocate a buffer as part of struct elmcan if you prefer.

    You can force proper alignment with marking the memory as
    ____cacheline_aligned. Extra bonus for checking (and optimizing)
    structure packing with the "pahole" tool.

    regards,
    Marc

    --
    Pengutronix e.K. | Marc Kleine-Budde |
    Embedded Linux | https://www.pengutronix.de |
    Vertretung West/Dortmund | Phone: +49-231-2826-924 |
    Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2022-03-17 22:00    [W:2.410 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site