[1/3] hdpvr: fix up i2c device registration
Commit Message
We have to actually call i2c_new_device() once for each of the rx and tx
addresses. Also improve error-handling and device remove i2c cleanup.
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
drivers/media/video/hdpvr/hdpvr-core.c | 21 +++++++++++++++++----
drivers/media/video/hdpvr/hdpvr-i2c.c | 28 ++++++++++++++++++++--------
drivers/media/video/hdpvr/hdpvr.h | 6 +++++-
3 files changed, 42 insertions(+), 13 deletions(-)
Comments
On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> We have to actually call i2c_new_device() once for each of the rx and tx
> addresses. Also improve error-handling and device remove i2c cleanup.
>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
Reviewed-by: Andy Walls <awalls@md.metrocast.net>
I do have some comments, but the real show-stoppers are:
- '#if defined(..' for the i2c_del_adapter() in the error path
- A very unlikely race by using file scope data
See below.
> ---
> drivers/media/video/hdpvr/hdpvr-core.c | 21 +++++++++++++++++----
> drivers/media/video/hdpvr/hdpvr-i2c.c | 28 ++++++++++++++++++++--------
> drivers/media/video/hdpvr/hdpvr.h | 6 +++++-
> 3 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> index a6572e5..f617a23 100644
> --- a/drivers/media/video/hdpvr/hdpvr-core.c
> +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> @@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
> #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> retval = hdpvr_register_i2c_adapter(dev);
> if (retval < 0) {
> - v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
> + v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
> goto error;
> }
>
> - retval = hdpvr_register_i2c_ir(dev);
> - if (retval < 0)
> - v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> + hdpvr_register_ir_rx_i2c(dev);
> + if (!dev->i2c_rx) {
> + v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
> + goto reg_fail;
> + }
> +
> + hdpvr_register_ir_tx_i2c(dev);
> + if (!dev->i2c_tx) {
> + v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
> + goto reg_fail;
> + }
Once your driver is debugged and complete, there is really never a need
to save pointers to the i2c_clients. You might want to consider having
void hdpvr_register_ir_?x_i2c() instead return a pointer that you can
check against NULL.
> #endif
>
> /* let the user know what node this device is now attached to */
> @@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
> video_device_node_name(dev->video_dev));
> return 0;
>
> +reg_fail:
> + i2c_del_adapter(&dev->i2c_adapter);
Don't you need an '#if defined(CONFIG_I2C)...' here, in case the symbol
does not exist?
> error:
> if (dev) {
> /* Destroy single thread */
> @@ -424,6 +434,9 @@ static void hdpvr_disconnect(struct usb_interface *interface)
> mutex_lock(&dev->io_mutex);
> hdpvr_cancel_queue(dev);
> mutex_unlock(&dev->io_mutex);
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> + i2c_del_adapter(&dev->i2c_adapter);
> +#endif
> video_unregister_device(dev->video_dev);
> atomic_dec(&dev_nr);
> }
> diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
> index 89b71fa..e891bb0 100644
> --- a/drivers/media/video/hdpvr/hdpvr-i2c.c
> +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
> @@ -31,26 +31,38 @@
> #define Z8F0811_IR_RX_I2C_ADDR 0x71
>
>
> -static struct i2c_board_info hdpvr_i2c_board_info = {
> +static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
> I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
> +};
This does not need to be file scope. (In fact this creates a hard to
trigger race in the function below.)
It should be non-static function scope, as the I2C subsystem will make a
copy of the string and address byte and the platform data pointer when
you add the I2C device. See pvrusb2.
> +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev)
> +{
> + struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
> +
> + init_data->name = "HD-PVR";
> + hdpvr_ir_tx_i2c_board_info.platform_data = init_data;
If hdpvr_ir_tx_i2c_board_info is file scope and static, then you have a
race for the platform data pointer when two hdpvr's are instantiated
simultaneously. It'll be extremely rare, I know, but it is possible.
You should use a function scope copy of the board info.
The per device instance of the init_data is correct.
> + dev->i2c_tx = i2c_new_device(&dev->i2c_adapter,
> + &hdpvr_ir_tx_i2c_board_info);
> +}
Again maybe just have this function 'return i2c_new_device(...);'.
The caller can save away the i2c_client, if you want to keep a pointer
around.
> +static struct i2c_board_info hdpvr_ir_rx_i2c_board_info = {
> I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
> };
>
> -int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
> +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev)
> {
> - struct i2c_client *c;
> struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
>
> /* Our default information for ir-kbd-i2c.c to use */
> init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW;
> init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
> init_data->type = RC_TYPE_RC5;
> - init_data->name = "HD PVR";
> - hdpvr_i2c_board_info.platform_data = init_data;
> -
> - c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
> + init_data->name = "HD-PVR";
> + hdpvr_ir_rx_i2c_board_info.platform_data = init_data;
>
> - return (c == NULL) ? -ENODEV : 0;
> + dev->i2c_rx = i2c_new_device(&dev->i2c_adapter,
> + &hdpvr_ir_rx_i2c_board_info);
> }
Same comments as for Rx.
> static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
> diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
> index ee74e3b..41a579e 100644
> --- a/drivers/media/video/hdpvr/hdpvr.h
> +++ b/drivers/media/video/hdpvr/hdpvr.h
> @@ -108,6 +108,9 @@ struct hdpvr_device {
>
> /* I2C adapter */
> struct i2c_adapter i2c_adapter;
> + /* I2C clients */
> + struct i2c_client *i2c_rx;
> + struct i2c_client *i2c_tx;
I don't see where you are using these. If you need them, you may want
to rename them to i2c_ir_rx and i2c_ir_tx, if hdpvr will ever talk to
another device on the I2C bus.
> /* I2C lock */
> struct mutex i2c_mutex;
> /* I2C message buffer space */
> @@ -313,7 +316,8 @@ int hdpvr_cancel_queue(struct hdpvr_device *dev);
> /* i2c adapter registration */
> int hdpvr_register_i2c_adapter(struct hdpvr_device *dev);
>
> -int hdpvr_register_i2c_ir(struct hdpvr_device *dev);
> +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev);
> +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev);
>
> /*========================================================================*/
> /* buffer management */
Regards,
Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 21, 2011 at 08:34:58AM -0500, Andy Walls wrote:
> On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> > We have to actually call i2c_new_device() once for each of the rx and tx
> > addresses. Also improve error-handling and device remove i2c cleanup.
> >
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
>
> Reviewed-by: Andy Walls <awalls@md.metrocast.net>
>
> I do have some comments, but the real show-stoppers are:
>
> - '#if defined(..' for the i2c_del_adapter() in the error path
> - A very unlikely race by using file scope data
>
> See below.
>
> > ---
> > drivers/media/video/hdpvr/hdpvr-core.c | 21 +++++++++++++++++----
> > drivers/media/video/hdpvr/hdpvr-i2c.c | 28 ++++++++++++++++++++--------
> > drivers/media/video/hdpvr/hdpvr.h | 6 +++++-
> > 3 files changed, 42 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> > index a6572e5..f617a23 100644
> > --- a/drivers/media/video/hdpvr/hdpvr-core.c
> > +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> > @@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
> > #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> > retval = hdpvr_register_i2c_adapter(dev);
> > if (retval < 0) {
> > - v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
> > + v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
> > goto error;
> > }
> >
> > - retval = hdpvr_register_i2c_ir(dev);
> > - if (retval < 0)
> > - v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> > + hdpvr_register_ir_rx_i2c(dev);
> > + if (!dev->i2c_rx) {
> > + v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
> > + goto reg_fail;
> > + }
> > +
> > + hdpvr_register_ir_tx_i2c(dev);
> > + if (!dev->i2c_tx) {
> > + v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
> > + goto reg_fail;
> > + }
>
> Once your driver is debugged and complete, there is really never a need
> to save pointers to the i2c_clients. You might want to consider having
> void hdpvr_register_ir_?x_i2c() instead return a pointer that you can
> check against NULL.
Yeah, I'd saved them when I was thinking I might need to call
i2c_unregister_device on driver removal, then debated dropping them, but
left them in, just in case. If you're certain there's never a particularly
good reason to keep them around, I'll drop them. An earlier iteration of
this patch did return a struct i2c_client for hdpvr_probe to check for
NULL.
> > #endif
> >
> > /* let the user know what node this device is now attached to */
> > @@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
> > video_device_node_name(dev->video_dev));
> > return 0;
> >
> > +reg_fail:
> > + i2c_del_adapter(&dev->i2c_adapter);
>
> Don't you need an '#if defined(CONFIG_I2C)...' here, in case the symbol
> does not exist?
Crap, yeah, thought I was clever catching it in the remove path, missed it
here... :)
> > -static struct i2c_board_info hdpvr_i2c_board_info = {
> > +static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
> > I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
> > +};
>
> This does not need to be file scope. (In fact this creates a hard to
> trigger race in the function below.)
>
> It should be non-static function scope, as the I2C subsystem will make a
> copy of the string and address byte and the platform data pointer when
> you add the I2C device. See pvrusb2.
Okay, will go with code that looks more like pvrusb2.
Thanks for the review, will get a v2 together shortly, sanity-check and
post it...
@@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
retval = hdpvr_register_i2c_adapter(dev);
if (retval < 0) {
- v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
+ v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
goto error;
}
- retval = hdpvr_register_i2c_ir(dev);
- if (retval < 0)
- v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
+ hdpvr_register_ir_rx_i2c(dev);
+ if (!dev->i2c_rx) {
+ v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
+ goto reg_fail;
+ }
+
+ hdpvr_register_ir_tx_i2c(dev);
+ if (!dev->i2c_tx) {
+ v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
+ goto reg_fail;
+ }
#endif
/* let the user know what node this device is now attached to */
@@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
video_device_node_name(dev->video_dev));
return 0;
+reg_fail:
+ i2c_del_adapter(&dev->i2c_adapter);
error:
if (dev) {
/* Destroy single thread */
@@ -424,6 +434,9 @@ static void hdpvr_disconnect(struct usb_interface *interface)
mutex_lock(&dev->io_mutex);
hdpvr_cancel_queue(dev);
mutex_unlock(&dev->io_mutex);
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+ i2c_del_adapter(&dev->i2c_adapter);
+#endif
video_unregister_device(dev->video_dev);
atomic_dec(&dev_nr);
}
@@ -31,26 +31,38 @@
#define Z8F0811_IR_RX_I2C_ADDR 0x71
-static struct i2c_board_info hdpvr_i2c_board_info = {
+static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
+};
+
+void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev)
+{
+ struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
+
+ init_data->name = "HD-PVR";
+ hdpvr_ir_tx_i2c_board_info.platform_data = init_data;
+
+ dev->i2c_tx = i2c_new_device(&dev->i2c_adapter,
+ &hdpvr_ir_tx_i2c_board_info);
+}
+
+static struct i2c_board_info hdpvr_ir_rx_i2c_board_info = {
I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
};
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
+void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev)
{
- struct i2c_client *c;
struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
/* Our default information for ir-kbd-i2c.c to use */
init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW;
init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
init_data->type = RC_TYPE_RC5;
- init_data->name = "HD PVR";
- hdpvr_i2c_board_info.platform_data = init_data;
-
- c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
+ init_data->name = "HD-PVR";
+ hdpvr_ir_rx_i2c_board_info.platform_data = init_data;
- return (c == NULL) ? -ENODEV : 0;
+ dev->i2c_rx = i2c_new_device(&dev->i2c_adapter,
+ &hdpvr_ir_rx_i2c_board_info);
}
static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
@@ -108,6 +108,9 @@ struct hdpvr_device {
/* I2C adapter */
struct i2c_adapter i2c_adapter;
+ /* I2C clients */
+ struct i2c_client *i2c_rx;
+ struct i2c_client *i2c_tx;
/* I2C lock */
struct mutex i2c_mutex;
/* I2C message buffer space */
@@ -313,7 +316,8 @@ int hdpvr_cancel_queue(struct hdpvr_device *dev);
/* i2c adapter registration */
int hdpvr_register_i2c_adapter(struct hdpvr_device *dev);
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev);
+void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev);
+void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev);
/*========================================================================*/
/* buffer management */