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@xxxxxxxxx>
> >
> > 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
