lkml.org 
[lkml]   [2019]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
On Mon, Mar 11, 2019 at 05:51:15PM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() before return
> from sci_probe_single() in the error handling case.
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---

Sorry, I didn't really look at the code when I saw the v1 patch.

There are other error paths, but actually the whole approach is wrong.
Please, read my google plus post about error handling:

https://plus.google.com/u/0/106378716002406849458/posts/1Ud9JbaYnPr

But then the other rule I didn't mention in that post which applies
here is that the error handling should "mirror" the allocation code
so if you have:

if (foo) {
ret = allocate_one();
if (ret)
return ret;
}
ret = allocate_two();
if (ret)
goto free_one;

The error handling should mirror the "if (foo) " condition. Like this:

free_one:
if (foo)
free_one();

Even if you can do extra analysis and find that the "if (foo) " can
be removed, you should leave there, because the mirroring helps human
readers.

In this case, the code is doing:

drivers/tty/serial/sh-sci.c
3259 return -EBUSY;
3260
3261 mutex_lock(&sci_uart_registration_lock);
3262 if (!sci_uart_driver.state) {
^^^^^^^^^^^^^^^^^^^^^^
3263 ret = uart_register_driver(&sci_uart_driver);
3264 if (ret) {
3265 mutex_unlock(&sci_uart_registration_lock);
3266 return ret;
3267 }
3268 }
3269 mutex_unlock(&sci_uart_registration_lock);
3270

We would have to mirror the "if (!sci_uart_driver.state) {" code.

But actually, we can't.

The first driver to hit this code is supposed to load the
sci_uart_driver. We can't know if we are the last driver to stop using
the sci_uart_driver so we can't know if we can free it. This looks like
a very ugly hack to me. It should probably be using ref counters.

regards,
an carpenter

\
 
 \ /
  Last update: 2019-03-11 13:47    [W:0.043 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site