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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

> 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.


Best regards,
Krzysztof