Re: [PATCH v5 3/3] w1: add UART w1 bus driver

[Christoph Winklhofer <cj.winklhofer@gmail.com>]

On Wed, Jan 31, 2024 at 02:12:34PM +0100, Krzysztof Kozlowski wrote:
> On 26/01/2024 16:42, Christoph Winklhofer via B4 Relay wrote:
> > From: Christoph Winklhofer <cj.winklhofer@gmail.com>
> > 
> > Add a UART 1-Wire bus driver. The driver utilizes the UART interface via
> > the Serial Device Bus to create the 1-Wire timing patterns. The driver
> > was tested on a "Raspberry Pi 3B" with a DS18B20 and on a "Variscite
> > DART-6UL" with a DS18S20 temperature sensor.
> > 
> 
> ...
> 
> > + * struct w1_uart_config - configuration for 1-Wire operation
> > + *
> > + * @baudrate: baud-rate returned from serdev
> > + * @delay_us: delay to complete a 1-Wire cycle (in us)
> > + * @tx_byte: byte to generate 1-Wire timing pattern
> > + */
> > +struct w1_uart_config {
> > +	unsigned int baudrate;
> > +	unsigned int delay_us;
> > +	u8 tx_byte;
> > +};
> > +
> > +struct w1_uart_device {
> > +	struct serdev_device *serdev;
> > +	struct w1_bus_master bus;
> > +
> > +	struct w1_uart_config cfg_reset;
> > +	struct w1_uart_config cfg_touch_0;
> > +	struct w1_uart_config cfg_touch_1;
> > +
> > +	struct completion rx_byte_received;
> > +	int rx_err;
> > +	u8 rx_byte;
> > +
> 
> Missing documentation of mutex scope. What does it protect?
> 

The mutex should protect concurrent access to rx_err and rx_byte. It
would be not be required in the good case: a write is initiated solely
by the w1-callbacks in 'w1_uart_serdev_tx_rx' and a completion is used
to wait for the result of serdev-receive.

However, in case the UART is not configured as a loop, a serdev-receive
may occur when w1_uart_serdev_tx_rx evaluates rx_err and rx_byte in
w1_uart_serdev_tx_rx, so it is protected - however, I will try to find
a better way to detect such an error.

In addition, the w1-callbacks should also return during a 'remove' (with
the mutex_try_lock) - see comment on that below.

> > +	struct mutex mutex;
> > +};
> > +
> 
> ...
> 
> > +/*
> > + * Send one byte (tx_byte) and read one byte (rx_byte) via serdev.
> > + */
> > +static int w1_uart_serdev_tx_rx(struct w1_uart_device *w1dev,
> > +				const struct w1_uart_config *w1cfg, u8 *rx_byte)
> > +{
> > +	struct serdev_device *serdev = w1dev->serdev;
> > +	int ret;
> > +
> > +	serdev_device_write_flush(serdev);
> > +	serdev_device_set_baudrate(serdev, w1cfg->baudrate);
> > +
> > +	/* write and immediately read one byte */
> > +	reinit_completion(&w1dev->rx_byte_received);
> > +	ret = serdev_device_write_buf(serdev, &w1cfg->tx_byte, 1);
> > +	if (ret != 1)
> > +		return -EIO;
> > +	ret = wait_for_completion_interruptible_timeout(
> > +		&w1dev->rx_byte_received, W1_UART_TIMEOUT);
> > +	if (ret <= 0)
> > +		return -EIO;
> > +
> > +	/* locking could fail during driver remove or when serdev is
> 
> It's not netdev, so:
> /*
>  *
> 

Ok.

> > +	 * unexpectedly in the receive callback.
> > +	 */
> > +	if (!mutex_trylock(&w1dev->mutex))
> > +		return -EIO;
> > +
> > +	ret = w1dev->rx_err;
> > +	if (ret == 0)
> > +		*rx_byte = w1dev->rx_byte;
> > +
> > +	if (w1cfg->delay_us > 0)
> > +		fsleep(w1cfg->delay_us);
> > +
> > +	mutex_unlock(&w1dev->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t w1_uart_serdev_receive_buf(struct serdev_device *serdev,
> > +					  const u8 *buf, size_t count)
> > +{
> > +	struct w1_uart_device *w1dev = serdev_device_get_drvdata(serdev);
> > +
> > +	mutex_lock(&w1dev->mutex);
> > +
> > +	/* sent a single byte and receive one single byte */
> > +	if (count == 1) {
> > +		w1dev->rx_byte = buf[0];
> > +		w1dev->rx_err = 0;
> > +	} else {
> > +		w1dev->rx_err = -EIO;
> > +	}
> > +
> > +	mutex_unlock(&w1dev->mutex);
> > +	complete(&w1dev->rx_byte_received);
> > +
> > +	return count;
> > +}
> > +
> > +static const struct serdev_device_ops w1_uart_serdev_ops = {
> > +	.receive_buf = w1_uart_serdev_receive_buf,
> > +	.write_wakeup = serdev_device_write_wakeup,
> > +};
> > +
> > +/*
> > + * 1-wire reset and presence detect: A present slave will manipulate
> > + * the received byte by pulling the 1-Wire low.
> > + */
> > +static u8 w1_uart_reset_bus(void *data)
> > +{
> > +	struct w1_uart_device *w1dev = data;
> > +	const struct w1_uart_config *w1cfg = &w1dev->cfg_reset;
> > +	int ret;
> > +	u8 val;
> > +
> > +	ret = w1_uart_serdev_tx_rx(w1dev, w1cfg, &val);
> > +	if (ret < 0)
> > +		return -1;
> > +
> > +	/* Device present (0) or no device (1) */
> > +	return val != w1cfg->tx_byte ? 0 : 1;
> > +}
> > +
> > +/*
> > + * 1-Wire read and write cycle: Only the read-0 manipulates the
> > + * received byte, all others left the line untouched.
> > + */
> > +static u8 w1_uart_touch_bit(void *data, u8 bit)
> > +{
> > +	struct w1_uart_device *w1dev = data;
> > +	const struct w1_uart_config *w1cfg = bit ? &w1dev->cfg_touch_1 :
> > +						   &w1dev->cfg_touch_0;
> > +	int ret;
> > +	u8 val;
> > +
> > +	ret = w1_uart_serdev_tx_rx(w1dev, w1cfg, &val);
> > +
> > +	/* return inactive bus state on error */
> > +	if (ret < 0)
> > +		return 1;
> > +
> > +	return val == w1cfg->tx_byte ? 1 : 0;
> > +}
> > +
> > +static int w1_uart_probe(struct serdev_device *serdev)
> > +{
> > +	struct device *dev = &serdev->dev;
> > +	struct w1_uart_device *w1dev;
> > +	int ret;
> > +
> > +	w1dev = devm_kzalloc(dev, sizeof(*w1dev), GFP_KERNEL);
> > +	if (!w1dev)
> > +		return -ENOMEM;
> > +	w1dev->bus.data = w1dev;
> > +	w1dev->bus.reset_bus = w1_uart_reset_bus;
> > +	w1dev->bus.touch_bit = w1_uart_touch_bit;
> > +	w1dev->serdev = serdev;
> > +
> > +	init_completion(&w1dev->rx_byte_received);
> > +	mutex_init(&w1dev->mutex);
> > +
> > +	ret = w1_uart_serdev_open(w1dev);
> > +	if (ret < 0)
> > +		return ret;
> > +	serdev_device_set_drvdata(serdev, w1dev);
> > +	serdev_device_set_client_ops(serdev, &w1_uart_serdev_ops);
> > +
> > +	return w1_add_master_device(&w1dev->bus);
> > +}
> > +
> > +static void w1_uart_remove(struct serdev_device *serdev)
> > +{
> > +	struct w1_uart_device *w1dev = serdev_device_get_drvdata(serdev);
> > +
> > +	mutex_lock(&w1dev->mutex);
> > +
> > +	w1_remove_master_device(&w1dev->bus);
> > +
> > +	mutex_unlock(&w1dev->mutex);
> 
> This is still suspicious. You do not have serdev_device_close and you
> want to protect from concurrent access but it looks insufficient.
> 
> This code assumes that:
> 
> w1_uart_remove()
>   <-- here concurrent read/write might start
>   mutex_lock()
>   w1_remove_master_device()
>   mutex_unlock()
>   <-- now w1_uart_serdev_tx_rx() or w1_uart_serdev_receive_buf() can be
> executed, but device is removed. So what's the point of the mutex here?
> 
> What exactly is protected by the mutex? So far it looks like only some
> contents of w1dev, but it does not matter, because it that memory is
> still valid at this point.
> 
> After describing what is protected we can think whether it is really
> protected...
> 
> 
> > 
> 
> Best regards,
> Krzysztof
> 

Yes, it is still suspicious, sorry..

After w1_uart_remove, serdev is closed and w1dev is released. Therefore
the w1-callback (w1_uart_serdev_tx_rx) must be finished before returning
from w1_uart_remove. That was the intention with the lock and trylock.

I thought that after w1_remove_master_device, the w1-callback
(w1_uart_serdev_tx_rx) is finished which is not the case. I will check
the working of w1_remove_master_device, probably it requires a lock to
a mutex from w1-bus.

Many thanks for your review and pointing the things out!

Kind regards,
Christoph