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

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

On Thu, Feb 01, 2024 at 10:35:32AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 08:29, Christoph Winklhofer wrote:
> >>> +
> >>> +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
> 
> I did not even go to end of w1_uart_remove(). In my code above, that
> thread is still in w1_uart_remove(), just after unlocking mutex.
> 

Ok, I looked more closely at the underlying w1-code and think it is
sufficient to only call w1_remove_master_device() in w1_uart_remove.
This function waits until all slaves have finished their work, so
w1_uart_serdev_tx_rx finished too. The lock is not required.

Hence, I will use the mutex only to protected concurrent access of
serdev and w1-callbacks (for rx_byte and rx_err).

> > from w1_uart_remove. That was the intention with the lock and trylock.
> 
> Then it does not look really protected. To be honest, w1-gpio and other
> drivers also might have a race here. You can test it by adding long
> sleeps in read/write paths and then trying to unbind device. Maybe this
> should be fixed everywhere, but this mutex here brings more questions.
> 

I added a delay, unbind takes longer but works without problems when its
done during a temperature read.

IMO also the other drivers should be fine. From w1_int.c: The w1_master
is ref-counted and released when it is unused (2). In w1_slave_detach
(1), the slaves decrement this count, perform specific clean up (in
remove_slave) and remove sysfs entries.

w1_int.c:

void __w1_remove_master_device(struct w1_master *dev)
...
	list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
		mutex_unlock(&dev->list_mutex);
		w1_slave_detach(sl); 		(1)
...
	while (atomic_read(&dev->refcnt)) { 	(2)
...	
	}
...
	w1_free_dev(dev);

For example w1_therm waits in remove_slave until its w1-operations are
done. The other slave-drivers (e.g. w1_ds2405.c) use w1-operations in
their sysfs-callback and I suppose that sysfs-removal waits until the
sysfs-read is finished.

Kind regards,
Christoph