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
