Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error
On Tue, 2009-04-07 at 11:31 +0200, Jean Delvare wrote: On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote: On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote: * Return actual error values as returned by the i2c subsystem, rather than 0 or 1. * If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net --- linux/drivers/media/video/cx18/cx18-i2c.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c 2009-03-01 16:09:09.0 +0100 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 18:45:18.0 +0200 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c /* init + register i2c algo-bit adapter */ int init_cx18_i2c(struct cx18 *cx) { - int i; + int i, err; CX18_DEBUG_I2C(i2c init\n); for (i = 0; i 2; i++) { @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx) cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL, core, reset, (u32) CX18_GPIO_RESET_I2C); - return i2c_bit_add_bus(cx-i2c_adap[0]) || - i2c_bit_add_bus(cx-i2c_adap[1]); + err = i2c_bit_add_bus(cx-i2c_adap[0]); + if (err) + goto err; + err = i2c_bit_add_bus(cx-i2c_adap[1]); + if (err) + goto err_del_bus_0; + return 0; + + err_del_bus_0: + i2c_del_adapter(cx-i2c_adap[0]); + err: + return err; } void exit_cx18_i2c(struct cx18 *cx) Reviewed-by: Andy Walls awa...@radix.net If it matters: Acked-by: Andy Walls awa...@radix.net Regards, Andy [Context edited for clarity.] Mauro, can you please apply this patch now? It is a bug fix not directly related to my ir-kbd-i2c work, it would be nice to have it applied quickly so that I don't have to carry the patch around. Thanks, -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error
On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote: On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote: * Return actual error values as returned by the i2c subsystem, rather than 0 or 1. * If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net --- linux/drivers/media/video/cx18/cx18-i2c.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c 2009-03-01 16:09:09.0 +0100 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 18:45:18.0 +0200 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c /* init + register i2c algo-bit adapter */ int init_cx18_i2c(struct cx18 *cx) { - int i; + int i, err; CX18_DEBUG_I2C(i2c init\n); for (i = 0; i 2; i++) { @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx) cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL, core, reset, (u32) CX18_GPIO_RESET_I2C); - return i2c_bit_add_bus(cx-i2c_adap[0]) || - i2c_bit_add_bus(cx-i2c_adap[1]); + err = i2c_bit_add_bus(cx-i2c_adap[0]); + if (err) + goto err; + err = i2c_bit_add_bus(cx-i2c_adap[1]); + if (err) + goto err_del_bus_0; + return 0; + + err_del_bus_0: + i2c_del_adapter(cx-i2c_adap[0]); + err: + return err; } void exit_cx18_i2c(struct cx18 *cx) Reviewed-by: Andy Walls awa...@radix.net [Context edited for clarity.] Mauro, can you please apply this patch now? It is a bug fix not directly related to my ir-kbd-i2c work, it would be nice to have it applied quickly so that I don't have to carry the patch around. Thanks, -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] cx18: Fix the handling of i2c bus registration error
* Return actual error values as returned by the i2c subsystem, rather than 0 or 1. * If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net --- linux/drivers/media/video/cx18/cx18-i2c.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c 2009-03-01 16:09:09.0 +0100 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 18:45:18.0 +0200 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c /* init + register i2c algo-bit adapter */ int init_cx18_i2c(struct cx18 *cx) { - int i; + int i, err; CX18_DEBUG_I2C(i2c init\n); for (i = 0; i 2; i++) { @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx) cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL, core, reset, (u32) CX18_GPIO_RESET_I2C); - return i2c_bit_add_bus(cx-i2c_adap[0]) || - i2c_bit_add_bus(cx-i2c_adap[1]); + err = i2c_bit_add_bus(cx-i2c_adap[0]); + if (err) + goto err; + err = i2c_bit_add_bus(cx-i2c_adap[1]); + if (err) + goto err_del_bus_0; + return 0; + + err_del_bus_0: + i2c_del_adapter(cx-i2c_adap[0]); + err: + return err; } void exit_cx18_i2c(struct cx18 *cx) -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error
On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote: * Return actual error values as returned by the i2c subsystem, rather than 0 or 1. * If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net Jean, Thanks for noticing this one and providing a patch. I have one comment below... --- linux/drivers/media/video/cx18/cx18-i2c.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c2009-03-01 16:09:09.0 +0100 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 18:45:18.0 +0200 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c /* init + register i2c algo-bit adapter */ int init_cx18_i2c(struct cx18 *cx) { - int i; + int i, err; CX18_DEBUG_I2C(i2c init\n); for (i = 0; i 2; i++) { @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx) cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL, core, reset, (u32) CX18_GPIO_RESET_I2C); - return i2c_bit_add_bus(cx-i2c_adap[0]) || - i2c_bit_add_bus(cx-i2c_adap[1]); + err = i2c_bit_add_bus(cx-i2c_adap[0]); if (err) return err; err = i2c_bit_add_bus(cx-i2c_adap[1]); if (err) i2c_del_adapter(cx-i2c_adap[0]); return err; This sequence saves a few lines of code and gets rid of the goto's compared to what you proposed below. + if (err) + goto err; + err = i2c_bit_add_bus(cx-i2c_adap[1]); + if (err) + goto err_del_bus_0; + return 0; + + err_del_bus_0: + i2c_del_adapter(cx-i2c_adap[0]); + err: + return err; } void exit_cx18_i2c(struct cx18 *cx) Reviewed-by: Andy Walls awa...@radix.net Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error
Hi Andy, Thanks for the fast review. On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote: On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote: * Return actual error values as returned by the i2c subsystem, rather than 0 or 1. * If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Hans Verkuil hverk...@xs4all.nl Cc: Andy Walls awa...@radix.net Jean, Thanks for noticing this one and providing a patch. I have one comment below... --- linux/drivers/media/video/cx18/cx18-i2c.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c 2009-03-01 16:09:09.0 +0100 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 18:45:18.0 +0200 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c /* init + register i2c algo-bit adapter */ int init_cx18_i2c(struct cx18 *cx) { - int i; + int i, err; CX18_DEBUG_I2C(i2c init\n); for (i = 0; i 2; i++) { @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx) cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL, core, reset, (u32) CX18_GPIO_RESET_I2C); - return i2c_bit_add_bus(cx-i2c_adap[0]) || - i2c_bit_add_bus(cx-i2c_adap[1]); + err = i2c_bit_add_bus(cx-i2c_adap[0]); if (err) return err; err = i2c_bit_add_bus(cx-i2c_adap[1]); if (err) i2c_del_adapter(cx-i2c_adap[0]); return err; This sequence saves a few lines of code and gets rid of the goto's compared to what you proposed below. Correct, actually my initial attempt looked like this. But then patch 3/6 adds code, which makes your solution 2 lines bigger, while my solution stays as is, so the difference between both becomes very thin. Some developers (including me) prefer to have a single error path, others hate gotos more than (potential) code duplication. I didn't know what you'd prefer as the driver maintainer. If you want me to use the variant without gotos, I can do that, no problem. + if (err) + goto err; + err = i2c_bit_add_bus(cx-i2c_adap[1]); + if (err) + goto err_del_bus_0; + return 0; + + err_del_bus_0: + i2c_del_adapter(cx-i2c_adap[0]); + err: + return err; } void exit_cx18_i2c(struct cx18 *cx) Reviewed-by: Andy Walls awa...@radix.net Thanks, -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error
On Sat, 2009-04-04 at 16:23 +0200, Jean Delvare wrote: Hi Andy, Thanks for the fast review. On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote: Correct, actually my initial attempt looked like this. But then patch 3/6 adds code, which makes your solution 2 lines bigger, while my solution stays as is, so the difference between both becomes very thin. Some developers (including me) prefer to have a single error path, others hate gotos more than (potential) code duplication. I didn't know what you'd prefer as the driver maintainer. If you want me to use the variant without gotos, I can do that, no problem. Meh, whichever way you like is fine for now. If I really decide to care about it, I'll muck with it when I get the hardware I2C masters working. I'll have to touch that section of code at that time anyway. Acked-by: Andy Walls awa...@radix.net Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html