Received-SPF: pass (google.com: domain of linux-kernel+bounces-50476-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 2 Feb 2024 20:23:30 +0100
From: Christoph Winklhofer <cj.winklhofer@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Rob Herring <robh@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v5 3/3] w1: add UART w1 bus driver
Message-ID: <Zb1BMswaaZTXzaFJ@cjw-notebook>
References: <20240126-w1-uart-v5-0-1d82bfdc2ae9@gmail.com>
 <20240126-w1-uart-v5-3-1d82bfdc2ae9@gmail.com>
 <092a9986-5ebb-483d-9911-37a93d7cb2dd@kernel.org>
 <ZbtIPo--1hfzNmho@cjw-notebook>
 <94075be0-b91c-4147-86b1-582b124e71a0@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <94075be0-b91c-4147-86b1-582b124e71a0@linaro.org>

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