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@xxxxxxxxxx> > --- 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
