lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1359a0e5024445d0e2bc1c8659094c35dc85c090.camel@hexdev.de>
Date: Thu, 02 May 2024 20:19:48 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Oliver Hartkopp <socketcan@...tkopp.net>, Marc Kleine-Budde
 <mkl@...gutronix.de>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, "David
 S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,  Conor
 Dooley <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>, Benjamin
 Tissoires <bentiss@...nel.org>,  Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Sebastian Reichel <sre@...nel.org>, Linus
 Walleij <linus.walleij@...aro.org>, Andreas Lauser
 <andreas.lauser@...cedes-benz.com>,  Jonathan Corbet <corbet@....net>,
 Pavel Pisa <pisa@....felk.cvut.cz>, linux-can@...r.kernel.org, 
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-input@...r.kernel.org,  linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 07/12] can: Add support for serdev LIN adapters

On Thu, 2024-05-02 at 10:44 +0200, Jiri Slaby wrote:
> On 02. 05. 24, 9:55, Christoph Fritz wrote:
> > This commit introduces LIN-Bus support for UART devices equipped with
> > LIN transceivers, utilizing the Serial Device Bus (serdev) interface.
> > 
> > For more details on an adapter, visit: https://hexdev.de/hexlin#tty
> ...
> > --- /dev/null
> > +++ b/drivers/net/can/lin-serdev.c
> > @@ -0,0 +1,514 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> > +
> > +#include <linux/module.h>
> > +#include <linux/wait.h>
> > +#include <linux/init.h>
> > +#include <linux/errno.h>
> > +#include <linux/string.h>
> > +#include <linux/kernel.h>
> 
> What do you need kernel.h for? You should explicitly require what you 
> need (you apparently do), so kernel.h should not be needed.

OK

> 
> > +#include <net/lin.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include <linux/slab.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/tty.h>
> 
> Might be eaier to maintain if you sort them.

OK, hid driver gets also sorted

> 
> > +#define LINSER_SAMPLES_PER_CHAR		10
> > +#define LINSER_TX_BUFFER_SIZE		11
> > +#define LINSER_RX_FIFO_SIZE		256
> > +#define LINSER_PARSE_BUFFER		24
> > +
> > +struct linser_rx {
> > +	u8 data;
> > +	u8 flag;
> > +};
> > +
> > +enum linser_rx_status {
> > +	NEED_MORE = -1,
> > +	MODE_OK = 0,
> > +	NEED_FORCE,
> > +};
> > +
> > +struct linser_priv {
> > +	struct lin_device *lin_dev;
> > +	struct serdev_device *serdev;
> > +	DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx);
> > +	struct delayed_work rx_work;
> > +	ulong break_usleep_min;
> > +	ulong break_usleep_max;
> > +	ulong post_break_usleep_min;
> > +	ulong post_break_usleep_max;
> > +	ulong force_timeout_jfs;
> 
> The same as for uint :)

OK

> 
> > +	struct lin_responder_answer respond_answ[LIN_NUM_IDS];
> > +	struct mutex resp_lock; /* protects respond_answ */
> > +	bool is_stopped;
> > +};
> ...
> > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate)
> > +{
> > +	unsigned long break_baud = (bitrate * 2) / 3;
> > +	unsigned long timeout_us;
> > +
> 
> Are those 1000000UL USEC_PER_SEC?

yes

> 
> > +	priv->break_usleep_min = (1000000UL * LINSER_SAMPLES_PER_CHAR) /
> > +				 break_baud;
> > +	priv->break_usleep_max = priv->break_usleep_min + 50;
> > +	priv->post_break_usleep_min = (1000000UL * 1 /* 1 bit */) / break_baud;
> > +	priv->post_break_usleep_max = priv->post_break_usleep_min + 30;
> > +
> > +	timeout_us = DIV_ROUND_CLOSEST(1000000UL * 256 /* bit */, bitrate);
> > +	priv->force_timeout_jfs = usecs_to_jiffies(timeout_us);
> > +}
> ...
> > +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id)
> > +{
> > +	struct lin_responder_answer *answ = &priv->respond_answ[id];
> > +	struct serdev_device *serdev = priv->serdev;
> > +	u8 buf[LINSER_TX_BUFFER_SIZE];
> > +	u8 checksum, count, n;
> > +	ssize_t write_len;
> > +
> > +	mutex_lock(&priv->resp_lock);
> > +
> > +	if (!answ->is_active)
> > +		goto unlock_and_exit_false;
> > +
> > +	if (answ->is_event_frame) {
> > +		struct lin_responder_answer *e_answ;
> > +
> > +		e_answ = &priv->respond_answ[answ->event_associated_id];
> > +		n = min(e_answ->lf.len, LIN_MAX_DLEN);
> > +		if (memcmp(answ->lf.data, e_answ->lf.data, n) != 0) {
> > +			memcpy(answ->lf.data, e_answ->lf.data, n);
> > +			checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> > +						    n, e_answ->lf.data,
> > +						    answ->lf.checksum_mode);
> > +			answ = e_answ;
> > +		} else {
> > +			goto unlock_and_exit_false;
> 
> Can't you simply use guard(mutex) above and avoid the error-prone 
> gotos/cleanup completely?

OK

> 
> > +		}
> > +	} else {
> > +		checksum = answ->lf.checksum;
> > +	}
> > +
> > +	count = min(answ->lf.len, LIN_MAX_DLEN);
> > +	memcpy(&buf[0], answ->lf.data, count);
> > +	buf[count] = checksum;
> > +
> > +	mutex_unlock(&priv->resp_lock);
> > +
> > +	write_len = serdev_device_write(serdev, buf, count + 1, 0);
> > +	if (write_len < count + 1)
> > +		return false;
> > +
> > +	serdev_device_wait_until_sent(serdev, 0);
> > +
> > +	return true;
> > +
> > +unlock_and_exit_false:
> > +	mutex_unlock(&priv->resp_lock);
> > +	return false;
> > +}
> > +
> > +static void linser_pop_fifo(struct linser_priv *priv, size_t n)
> > +{
> > +	struct serdev_device *serdev = priv->serdev;
> > +	struct linser_rx dummy;
> > +	size_t ret, i;
> > +
> > +	for (i = 0; i < n; i++) {
> > +		ret = kfifo_out(&priv->rx_fifo, &dummy, 1);
> 
> Does kfifo_skip() not work for records? (I added it recently for serial.)

Using kfifo_skip() greatly simplifies this function and it works for
records (uses __kfifo_skip_r), tests are successful.

Maybe the comment in kfifo.h could be made more clear from:

 "kfifo_skip - skip output data"
to
 "kfifo_skip - skip the next fifo record"

> 
> > +		if (ret != 1) {
> > +			dev_err(&serdev->dev, "Failed to pop from FIFO\n");
> > +			break;
> > +		}
> > +	}
> > +}
> 

Let me address these points and reroll in v3.

Thanks
  -- Christoph

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ