From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE65FC433F5 for ; Sat, 12 Mar 2022 21:21:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231760AbiCLVW6 (ORCPT ); Sat, 12 Mar 2022 16:22:58 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46650 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229803AbiCLVW6 (ORCPT ); Sat, 12 Mar 2022 16:22:58 -0500 Received: from mail.enpas.org (zhong.enpas.org [IPv6:2a03:4000:2:537::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id D8F9C184B53; Sat, 12 Mar 2022 13:21:50 -0800 (PST) Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail.enpas.org (Postfix) with ESMTPSA id DEE0F101CB4; Sat, 12 Mar 2022 21:21:48 +0000 (UTC) Date: Sat, 12 Mar 2022 22:21:42 +0100 From: Max Staudt To: Vincent Mailhol , Marc Kleine-Budde Cc: Wolfgang Grandegger , linux-can@vger.kernel.org, Oliver Neukum , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Jiri Slaby Subject: Re: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters Message-ID: <20220312222142.21591629.max@enpas.org> In-Reply-To: References: <20220307214303.1822590-1-max@enpas.org> <20220309135414.34f77251.max@enpas.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-can@vger.kernel.org Hi Vincent, (Hi Marc,) Thank you for your in-depth look at it - especially regarding stale things that reflect its age, such as cf->can_dlc vs. cf->len. This driver has been gathering some dust. I've reworked the code according to your review. @Marc - could you please have a look at this? The elmcan code currently has a dummy mailbox_read() function for rx_offload because can_rx_offload_add_fifo() requires it - is this intentional? @Vincent - two more things have remained, and I hope it's okay once I explain them: 1. _memstrcmp() - memcmp() vs. str(n)cmp() The _memstrcmp() function does not compare strings, it compares raw buffers. I am just using C strings for the fixed buffers to compare against, as that allows for shorter and easier to read code. The NUL byte at the end of those strings goes unused. Also, I have not looked at the assembly produced, since the semantics are different: str(n)cmp() needs to look for NUL bytes in the buffer(s), which is unnecessary here. As a bonus, NUL will never even occur because my code filters those bytes out upon reception from the UART (it's a documented quirk of the ELM327). Finally, even if I were to use strcmp(), the code would still look just as ugly. Except the machine would also look for NUL bytes, and the next human to read the code would wonder why I'm comparing strings and not buffers. Hence memcmp(), to help the code self-document and the compiler optimise - I hope that's okay. 2. Useless parentheses in a for loop's condition: I left those in there because it gets hard to read otherwise, IMHO. It's really a matter of taste though, and if you insist, I'll remove them. With if, it's easier to keep it readable: if (a == 1 && b == 2) { ... } Whereas with for, I've already used multiple lines to visually separate the initialiser, the condition, and the incrementer. Hence the parentheses to visually separate the two subconditions. Matter of taste really, and I'll change it if you insist. That's it! Everything else is already patched for a future v4 of the code. I'll CC you once I send that out. But I'd like to wait for Marc's feedback first. Thanks, Max