ddbridge: error handling improvements
cover ddbridge: error handling improvements
From: Daniel Scheller <[email protected]>
Two commits which will improve the error handling when attaching of (tuner) frontends fail, which complements the recent fixes in the DVB core (esp. dvb_frontend.c), making sure that on failure there won't be any frontend drivers left with a usecount > 0 and thus can be unloaded without -f on rmmod.
Also, don't miserably fail and stop hard when a single frontend failed to attach as other frontends connected to the current (or even other) bridge(s) can still work perfectly fine, so rather initialise as much as possible. (If a single PCI device fails to init, the kernel doesn't stop probing everything else on the bus)
This goes ontop of the ddbridge-0.9.32 bump (see [1]) which should have been merged for kernel 4.15rc1 originally, but unfortunately wasn't. No idea (didn't test) if this applies without the 0.9.32 changes (and please don't try to find out to avoid any merge errors/ conflicts - thanks.).
[1] http://www.spinics.net/lists/linux-media/msg123707.html
Daniel Scheller (2):
[media] ddbridge: improve error handling logic on fe attach failures [media] ddbridge: don't break on single/last port attach failure
drivers/media/pci/ddbridge/ddbridge-core.c | 51 ++++++++++++++---------------- 1 file changed, 23 insertions(+), 28 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/2 ddbridge: improve error handling logic on fe attach failures
From: Daniel Scheller <[email protected]>
This change makes sure that demod frontends are always detached whenever a tuner frontend attach failed. Achieve this by moving the detach-on- failure logic at the end of dvb_input_attach(), and adding a goto to this block on every tuner attach failure case, so if an error occurs, there are no stray attached frontends left. As a side effect, this removes some duplicated code.
--- drivers/media/pci/ddbridge/ddbridge-core.c | 49 ++++++++++++++---------------- 1 file changed, 22 insertions(+), 27 deletions(-)
drivers/media/pci/ddbridge/ddbridge-core.c +22 -27
@@ -1449,48 +1449,43 @@ static int dvb_input_attach(struct ddb_input *input)if (demod_attach_stv0900 (input,0 ) <0 )return -ENODEV ;if (tuner_attach_stv6110 (input,0 ) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBS_ST_AA :if (demod_attach_stv0900 (input,1 ) <0 )return -ENODEV ;if (tuner_attach_stv6110 (input,1 ) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBS_STV0910 :if (demod_attach_stv0910 (input,0 ) <0 )return -ENODEV ;if (tuner_attach_stv6111 (input,0 ) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBS_STV0910_PR :if (demod_attach_stv0910 (input,1 ) <0 )return -ENODEV ;if (tuner_attach_stv6111 (input,1 ) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBS_STV0910_P :if (demod_attach_stv0910 (input,0 ) <0 )return -ENODEV ;if (tuner_attach_stv6111 (input,1 ) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBCT_TR :if (demod_attach_drxk (input) <0 )return -ENODEV ;if (tuner_attach_tda18271 (input) <0 ) -return -ENODEV ; + goto err_tuner;break ;case DDB_TUNER_DVBCT_ST :if (demod_attach_stv0367 (input) <0 )return -ENODEV ; -if (tuner_attach_tda18212 (input, port->type) <0 ) { -if (dvb->fe2) -dvb_frontend_detach (dvb->fe2); -if (dvb->fe) -dvb_frontend_detach (dvb->fe); -return -ENODEV ; - } +if (tuner_attach_tda18212 (input, port->type) <0 ) + goto err_tuner;break ;case DDB_TUNER_DVBC2T2I_SONY_P : if (input->port->dev->link[input->port->lnr].info->ts_quirks &
@@ -1509,13 +1504,8 @@ static int dvb_input_attach(struct ddb_input *input) par =1 ;if (demod_attach_cxd28xx (input, par, osc24) <0 )return -ENODEV ; -if (tuner_attach_tda18212 (input, port->type) <0 ) { -if (dvb->fe2) -dvb_frontend_detach (dvb->fe2); -if (dvb->fe) -dvb_frontend_detach (dvb->fe); -return -ENODEV ; - } +if (tuner_attach_tda18212 (input, port->type) <0 ) + goto err_tuner;break ;case DDB_TUNER_DVBC2T2I_SONY : osc24 =1 ;
@@ -1525,13 +1515,8 @@ static int dvb_input_attach(struct ddb_input *input)case DDB_TUNER_ISDBT_SONY :if (demod_attach_cxd28xx (input,0 , osc24) <0 )return -ENODEV ; -if (tuner_attach_tda18212 (input, port->type) <0 ) { -if (dvb->fe2) -dvb_frontend_detach (dvb->fe2); -if (dvb->fe) -dvb_frontend_detach (dvb->fe); -return -ENODEV ; - } +if (tuner_attach_tda18212 (input, port->type) <0 ) + goto err_tuner;break ;default :return 0 ;
@@ -1554,6 +1539,16 @@ static int dvb_input_attach(struct ddb_input *input) dvb->attached =0x31 ;return 0 ; + +err_tuner: +dev_warn (port->dev->dev,"tuner attach failed!\n" ); + +if (dvb->fe2) +dvb_frontend_detach (dvb->fe2); +if (dvb->fe) +dvb_frontend_detach (dvb->fe); + +return -ENODEV ; }static int port_has_encti (struct ddb_port *port)
Signature · 1 line
2.13.6
PATCH 2/2 ddbridge: don't break on single/last port attach failure
In reply to Daniel SchellerFrom: Daniel Scheller <[email protected]>
As all error handling improved quite a bit, don't stop attaching frontends if one of them failed, since - if other tuner modules are connected to the PCIe bridge - other hardware may just work, so lets not break on a single port failure, but rather initialise as much as possible. Ie. if there are issues with a C2T2-equipped PCIe bridge card which has additional DuoFlex modules connected and the bridge generally works, the DuoFlex tuners can still work fine. Also, this only had an effect anyway if the failed device/port was the last one being enumerated.
--- drivers/media/pci/ddbridge/ddbridge-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/pci/ddbridge/ddbridge-core.c +1 -1
@@ -1962,7 +1962,7 @@ int ddb_ports_attach(struct ddb *dev) }for (i =0 ; i < dev->port_num; i++) { port = &dev->port[i]; - ret =ddb_port_attach (port); +ddb_port_attach (port); }return ret; }
Signature · 1 line
2.13.6
Mauro Carvalho Chehab
maintainerRe: [PATCH 2/2] [media] ddbridge: don't break on single/last port attach failure
Em Wed, 6 Dec 2017 18:59:15 +0100
Daniel Scheller <[email protected]> escreveu:
Quoted 21 lines
From: Daniel Scheller <[email protected]>
As all error handling improved quite a bit, don't stop attaching frontends if one of them failed, since - if other tuner modules are connected to the PCIe bridge - other hardware may just work, so lets not break on a single port failure, but rather initialise as much as possible. Ie. if there are issues with a C2T2-equipped PCIe bridge card which has additional DuoFlex modules connected and the bridge generally works, the DuoFlex tuners can still work fine. Also, this only had an effect anyway if the failed device/port was the last one being enumerated.
Signed-off-by Daniel Scheller <[email protected]>--- drivers/media/pci/ddbridge/ddbridge-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/media/pci/ddbridge/ddbridge-core.c +0
-0
drivers/media/pci/ddbridge/ddbridge-core.c int ddb_ports_attach(struct ddb *dev) · line 1965
Quoted 5 lines
drivers/media/pci/ddbridge/ddbridge-core.c +1
-1@@ -1962,7 +1962,7 @@ int ddb_ports_attach(struct ddb *dev) }for (i =0 ; i < dev->port_num; i++) { port = &dev->port[i]; - ret =ddb_port_attach (port); +ddb_port_attach (port);
Nah, ignoring an error doesn't seem right. It should at least print that attach failed. Also, if all attaches fail, probably the best would be to just detach everything and go to the error handling code, as there's something serious happening.
Something like:
for (i = 0; i < dev->port_num; i++) {
port = &dev->port[i];
ret = ddb_port_attach(port);
if (ret) {
dev_warn(port->dev->dev, "attach failed\n");
err_ports++;
}
if (err_ports == dev->port_num)
return -ENODEV;Thanks,
Mauro
Re: [PATCH 2/2] [media] ddbridge: don't break on single/last port attach failure
Quoted 20 lines · On Wed, 13 Dec 2017 13:26:02 -0200 Mauro Carvalho Chehab <[email protected]>…
Em Wed, 6 Dec 2017 18:59:15 +0100
Daniel Scheller <[email protected]> escreveu:Quoted 17 lines
From: Daniel Scheller <[email protected]>
As all error handling improved quite a bit, don't stop attaching frontends if one of them failed, since - if other tuner modules are connected to the PCIe bridge - other hardware may just work, so lets not break on a single port failure, but rather initialise as much as possible. Ie. if there are issues with a C2T2-equipped PCIe bridge card which has additional DuoFlex modules connected and the bridge generally works, the DuoFlex tuners can still work fine. Also, this only had an effect anyway if the failed device/port was the last one being enumerated.
Signed-off-by Daniel Scheller <[email protected]>--- drivers/media/pci/ddbridge/ddbridge-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 13 lines
drivers/media/pci/ddbridge/ddbridge-core.c int ddb_ports_attach(struct ddb *dev) · line 1965
Quoted 10 lines
(quoted diff) +0
-0
b/drivers/media/pci/ddbridge/ddbridge-core.cindex11c5cae92408..b43c40e0bf73100644 ---a/drivers/media/pci/ddbridge/ddbridge-core.c+++b/drivers/media/pci/ddbridge/ddbridge-core.c@@ -1962,7 +1962,7 @@ intddb_ports_attach(struct ddb *dev)}drivers/media/pci/ddbridge/ddbridge-core.c +1
-1for (i =0 ; i < dev->port_num; i++) { port = &dev->port[i]; - ret =ddb_port_attach (port); +ddb_port_attach (port);Nah, ignoring an error doesn't seem right. It should at least print that attach failed.
This is already the case in ddb_port_attach() (if (ret < 0) dev_err(...)).
Quoted 3 lines
Also, if all attaches fail, probably the best
would be to just detach everything and go to the error handling code, as there's something serious happening.
Well, will recheck the whole error handling there then when already at it, as single port failures can still leave some half-initialised stuff behind until ddbridge gets unloaded.
Thanks for your review, comments and your proposal!
Best regards,
Daniel Scheller
Signature · 1 line
https://github.com/herrnst
Mauro Carvalho Chehab
maintainerRe: [PATCH 2/2] [media] ddbridge: don't break on single/last port attach failure
Em Wed, 13 Dec 2017 18:40:52 +0100
Daniel Scheller <[email protected]> escreveu:
Quoted 40 lines
Quoted 20 lines · On Wed, 13 Dec 2017 13:26:02 -0200 Mauro Carvalho Chehab <[email protected]>…
Em Wed, 6 Dec 2017 18:59:15 +0100
Daniel Scheller <[email protected]> escreveu:Quoted 17 lines
From: Daniel Scheller <[email protected]>
As all error handling improved quite a bit, don't stop attaching frontends if one of them failed, since - if other tuner modules are connected to the PCIe bridge - other hardware may just work, so lets not break on a single port failure, but rather initialise as much as possible. Ie. if there are issues with a C2T2-equipped PCIe bridge card which has additional DuoFlex modules connected and the bridge generally works, the DuoFlex tuners can still work fine. Also, this only had an effect anyway if the failed device/port was the last one being enumerated.
Signed-off-by Daniel Scheller <[email protected]>--- drivers/media/pci/ddbridge/ddbridge-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 13 lines
drivers/media/pci/ddbridge/ddbridge-core.c int ddb_ports_attach(struct ddb *dev) · line 1965
Quoted 10 lines
(quoted diff) +0
-0
b/drivers/media/pci/ddbridge/ddbridge-core.cindex11c5cae92408..b43c40e0bf73100644 ---a/drivers/media/pci/ddbridge/ddbridge-core.c+++b/drivers/media/pci/ddbridge/ddbridge-core.c@@ -1962,7 +1962,7 @@ intddb_ports_attach(struct ddb *dev)}drivers/media/pci/ddbridge/ddbridge-core.c +1
-1for (i =0 ; i < dev->port_num; i++) { port = &dev->port[i]; - ret =ddb_port_attach (port); +ddb_port_attach (port);Nah, ignoring an error doesn't seem right. It should at least print that attach failed.
This is already the case in
ddb_port_attach()(if (ret < 0)dev_err(...)).
Quoted 7 lines
Quoted 3 lines
Also, if all attaches fail, probably the best
would be to just detach everything and go to the error handling code, as there's something serious happening.Well, will recheck the whole error handling there then when already at it, as single port failures can still leave some half-initialised stuff behind until ddbridge gets unloaded.
If this is the case, then you need to fix also the unbind logic, to be sure that nothing gets left. The best is to compile your test Kernel with KASAN enabled, in order to see if the remove logic is OK.
Quoted 5 lines
Thanks for your review, comments and your proposal!
Best regards,
Daniel Scheller
Thanks,
Mauro
Re: [PATCH 2/2] [media] ddbridge: don't break on single/last port attach failure
Quoted 48 lines · On Wed, 13 Dec 2017 17:44:37 -0200 Mauro Carvalho Chehab <[email protected]>…
Em Wed, 13 Dec 2017 18:40:52 +0100
Daniel Scheller <[email protected]> escreveu:Quoted 45 lines
Quoted 21 lines · On Wed, 13 Dec 2017 13:26:02 -0200 Mauro Carvalho Chehab <[email protected]>…
Em Wed, 6 Dec 2017 18:59:15 +0100
Daniel Scheller <[email protected]> escreveu:Quoted 18 lines
From: Daniel Scheller <[email protected]>
As all error handling improved quite a bit, don't stop attaching frontends if one of them failed, since - if other tuner modules are connected to the PCIe bridge - other hardware may just work, so lets not break on a single port failure, but rather initialise as much as possible. Ie. if there are issues with a C2T2-equipped PCIe bridge card which has additional DuoFlex modules connected and the bridge generally works, the DuoFlex tuners can still work fine. Also, this only had an effect anyway if the failed device/port was the last one being enumerated.
Signed-off-by Daniel Scheller <[email protected]>--- drivers/media/pci/ddbridge/ddbridge-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 13 lines
drivers/media/pci/ddbridge/ddbridge-core.c int ddb_ports_attach(struct ddb *dev) · line 1965
Quoted 10 lines
(quoted diff) +0
-0
b/drivers/media/pci/ddbridge/ddbridge-core.cindex11c5cae92408..b43c40e0bf73100644 ---a/drivers/media/pci/ddbridge/ddbridge-core.c+++b/drivers/media/pci/ddbridge/ddbridge-core.c@@ -1962,7 +1962,7 @@ intddb_ports_attach(struct ddb *dev)}drivers/media/pci/ddbridge/ddbridge-core.c +1
-1for (i =0 ; i < dev->port_num; i++) { port = &dev->port[i]; - ret =ddb_port_attach (port); +ddb_port_attach (port);Nah, ignoring an error doesn't seem right. It should at least print that attach failed.
This is already the case in
ddb_port_attach()(if (ret < 0)dev_err(...)).Quoted 3 lines
Also, if all attaches fail, probably the best
would be to just detach everything and go to the error handling code, as there's something serious happening.
Quoted 8 lines
Quoted 3 lines
Well, will recheck the whole error handling there then when already at it, as single port failures can still leave some half-initialised stuff behind until ddbridge gets unloaded.
If this is the case, then you need to fix also the unbind logic, to be sure that nothing gets left. The best is to compile your test Kernel with KASAN enabled, in order to see if the remove logic is OK.
There's nothing wrong regarding memory corruption when this happens, the state machine in the driver keeps track of this, knows how far a port got, tears down exactly these resources, and doesn't blindly free things (use-after-free etc). On unload, everything is correctly removed from memory, the unbind/teardown logic works fine regarding this. The only real issue which also other drivers suffered from was improper un-refcounting but all this was completely fixed with the latest changes in core:dvb_frontend.c (frontend_free and related friends).
But that KASAN thing is a good hint for some other issue I'm having with another driver for which I've no idea yet how to track that down, thanks for that (yet some things to learn and discover).
Best regards,
Daniel Scheller
Signature · 1 line
https://github.com/herrnst