[3/7] em28xx: Only deallocate struct em28xx after finishing all extensions

Commit Message

Mauro Carvalho Chehab Jan. 12, 2014, 11 p.m. UTC

We can't free struct em28xx while one of the extensions is still
using it.

So, add a kref() to control it, freeing it only after the
extensions fini calls.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
 drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
 drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
 drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
 drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
 drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
 6 files changed, 44 insertions(+), 28 deletions(-)

Comments

Frank Schäfer Jan. 13, 2014, 7:02 p.m. UTC | #1

On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
> We can't free struct em28xx while one of the extensions is still
> using it.
>
> So, add a kref() to control it, freeing it only after the
> extensions fini calls.
>
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> ---
>   drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
>   drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
>   drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
>   drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
>   drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
>   drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
>   6 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> index 97d9105e6830..8e959dae8358 100644
> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
>   
>   	em28xx_info("Binding audio extension\n");
>   
> +	kref_get(&dev->ref);
> +
>   	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
>   			 "Rechberger\n");
>   	printk(KERN_INFO
> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>   	if (dev == NULL)
>   		return 0;
>   
> -	if (dev->has_alsa_audio != 1) {
> +	if (!dev->has_alsa_audio) {
>   		/* This device does not support the extension (in this case
>   		   the device is expecting the snd-usb-audio module or
>   		   doesn't have analog audio support at all) */
> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>   		dev->adev.sndcard = NULL;
>   	}
>   
> +	kref_put(&dev->ref, em28xx_free_device);
>   	return 0;
>   }
>   
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 3b332d527ccb..df92f417634a 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
>   	flush_work(&dev->request_module_wk);
>   }
>   
> -/*
> - * em28xx_release_resources()
> - * unregisters the v4l2,i2c and usb devices
> - * called when the device gets disconnected or at module unload
> -*/
> -void em28xx_release_resources(struct em28xx *dev)
> +/**
> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
> + *
> + * @ref: struct kref for em28xx device
> + *
> + * This is called when all extensions and em28xx core unregisters a device
> + */
> +void em28xx_free_device(struct kref *ref)
>   {
> -	/*FIXME: I2C IR should be disconnected */
> +	struct em28xx *dev = kref_to_dev(ref);
>   
> -	mutex_lock(&dev->lock);
> +	em28xx_info("Freeing device\n");
>   
>   	if (dev->def_i2c_bus)
>   		em28xx_i2c_unregister(dev, 1);
> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
>   	/* Mark device as unused */
>   	clear_bit(dev->devno, &em28xx_devused);
>   
> -	mutex_unlock(&dev->lock);
> -};
> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
> +	kfree(dev->alt_max_pkt_size_isoc);
> +	kfree(dev);
> +}
> +EXPORT_SYMBOL_GPL(em28xx_free_device);
>   
>   /*
>    * em28xx_init_dev()
> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>   			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
>   	}
>   
> +	kref_init(&dev->ref);
> +
>   	request_modules(dev);
>   
>   	/* Should be the last thing to do, to avoid newer udev's to
> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>   
>   	em28xx_close_extension(dev);
>   
> -	em28xx_release_resources(dev);
> -
> -	if (!dev->users) {
> -		kfree(dev->alt_max_pkt_size_isoc);
> -		kfree(dev);
> -	}
> +	kref_put(&dev->ref, em28xx_free_device);
>   }
>   
>   static struct usb_driver em28xx_usb_driver = {
> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> index 5ea563e3f0e4..8674ae5fce06 100644
> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
>   	em28xx_info("Binding DVB extension\n");
>   
>   	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
> -
>   	if (dvb == NULL) {
>   		em28xx_info("em28xx_dvb: memory allocation failed\n");
>   		return -ENOMEM;
>   	}
> +	kref_get(&dev->ref);
>   	dev->dvb = dvb;
>   	dvb->fe[0] = dvb->fe[1] = NULL;
>   
> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
>   	dvb->adapter.mfe_shared = mfe_shared;
>   
>   	em28xx_info("DVB extension successfully initialized\n");
> +
>   ret:
>   	em28xx_set_mode(dev, EM28XX_SUSPEND);
>   	mutex_unlock(&dev->lock);
> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
>   		dev->dvb = NULL;
>   	}
>   
> +	kref_put(&dev->ref, em28xx_free_device);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index 61c061f3a476..33388b5922a0 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
>   		return 0;
>   	}
>   
> +	kref_get(&dev->ref);
> +
>   	if (dev->board.buttons)
>   		em28xx_init_buttons(dev);
>   
> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
>   
>   	/* skip detach on non attached boards */
>   	if (!ir)
> -		return 0;
> +		goto ref_put;
>   
>   	if (ir->rc)
>   		rc_unregister_device(ir->rc);
> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
>   	/* done */
>   	kfree(ir);
>   	dev->ir = NULL;
> +
> +ref_put:
> +	kref_put(&dev->ref, em28xx_free_device);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 587ff3fe9402..dc10cec772ba 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>   	v4l2_ctrl_handler_free(&dev->ctrl_handler);
>   	v4l2_device_unregister(&dev->v4l2_dev);
>   
> -	if (dev->users)
> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
> +	kref_put(&dev->ref, em28xx_free_device);
>   
>   	return 0;
>   }
> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
>   	mutex_lock(&dev->lock);
>   
>   	if (dev->users == 1) {
> -		/* free the remaining resources if device is disconnected */
> -		if (dev->disconnected) {
> -			kfree(dev->alt_max_pkt_size_isoc);
> +		/* No sense to try to write to the device */
> +		if (dev->disconnected)
>   			goto exit;
> -		}
>   
>   		/* Save some power by putting tuner to sleep */
>   		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>   
>   	em28xx_info("Registering V4L2 extension\n");
>   
> +	kref_get(&dev->ref);
> +
>   	mutex_lock(&dev->lock);
>   
>   	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 5d5d1b6f0294..d38c08e4da60 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -32,6 +32,7 @@
>   #include <linux/workqueue.h>
>   #include <linux/i2c.h>
>   #include <linux/mutex.h>
> +#include <linux/kref.h>
>   #include <linux/videodev2.h>
>   
>   #include <media/videobuf2-vmalloc.h>
> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
>   	enum em28xx_i2c_algo_type algo_type;
>   };
>   
> -
>   /* main device struct */
>   struct em28xx {
> +	struct kref ref;
> +
> +
>   	/* generic device properties */
>   	char name[30];		/* name (including minor) of the device */
>   	int model;		/* index in the device_data struct */
> @@ -706,6 +709,8 @@ struct em28xx {
>   	struct em28xx_dvb *dvb;
>   };
>   
> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
> +
>   struct em28xx_ops {
>   	struct list_head next;
>   	char *name;
> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
>   extern struct usb_device_id em28xx_id_table[];
>   int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
>   void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
> -void em28xx_release_resources(struct em28xx *dev);
> +void em28xx_free_device(struct kref *ref);
>   
>   /* Provided by em28xx-camera.c */
>   int em28xx_detect_sensor(struct em28xx *dev);
I welcome this patch and the general approach looks good.
I had started working on the same issue, but it's not that trivial.

At first glance there seem to be several issues, but I will need to 
review this patch in more detail and also make some tests.
Unfortunately, I don't have much time this evening, So could you please 
hold it back another day ?
I hope I can review the other remaining patch of this series (patch 5/7) 
later this evening.

Regards,
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Jan. 13, 2014, 7:23 p.m. UTC | #2

Em Mon, 13 Jan 2014 20:02:19 +0100
Frank Schäfer <[e-mail]> escreveu:

> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
> > We can't free struct em28xx while one of the extensions is still
> > using it.
> >
> > So, add a kref() to control it, freeing it only after the
> > extensions fini calls.
> >
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > ---
> >   drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
> >   drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
> >   drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
> >   drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
> >   drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
> >   drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
> >   6 files changed, 44 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> > index 97d9105e6830..8e959dae8358 100644
> > --- a/drivers/media/usb/em28xx/em28xx-audio.c
> > +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> > @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
> >   
> >   	em28xx_info("Binding audio extension\n");
> >   
> > +	kref_get(&dev->ref);
> > +
> >   	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
> >   			 "Rechberger\n");
> >   	printk(KERN_INFO
> > @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >   	if (dev == NULL)
> >   		return 0;
> >   
> > -	if (dev->has_alsa_audio != 1) {
> > +	if (!dev->has_alsa_audio) {
> >   		/* This device does not support the extension (in this case
> >   		   the device is expecting the snd-usb-audio module or
> >   		   doesn't have analog audio support at all) */
> > @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >   		dev->adev.sndcard = NULL;
> >   	}
> >   
> > +	kref_put(&dev->ref, em28xx_free_device);
> >   	return 0;
> >   }
> >   
> > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> > index 3b332d527ccb..df92f417634a 100644
> > --- a/drivers/media/usb/em28xx/em28xx-cards.c
> > +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> > @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
> >   	flush_work(&dev->request_module_wk);
> >   }
> >   
> > -/*
> > - * em28xx_release_resources()
> > - * unregisters the v4l2,i2c and usb devices
> > - * called when the device gets disconnected or at module unload
> > -*/
> > -void em28xx_release_resources(struct em28xx *dev)
> > +/**
> > + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
> > + *
> > + * @ref: struct kref for em28xx device
> > + *
> > + * This is called when all extensions and em28xx core unregisters a device
> > + */
> > +void em28xx_free_device(struct kref *ref)
> >   {
> > -	/*FIXME: I2C IR should be disconnected */
> > +	struct em28xx *dev = kref_to_dev(ref);
> >   
> > -	mutex_lock(&dev->lock);
> > +	em28xx_info("Freeing device\n");
> >   
> >   	if (dev->def_i2c_bus)
> >   		em28xx_i2c_unregister(dev, 1);
> > @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
> >   	/* Mark device as unused */
> >   	clear_bit(dev->devno, &em28xx_devused);
> >   
> > -	mutex_unlock(&dev->lock);
> > -};
> > -EXPORT_SYMBOL_GPL(em28xx_release_resources);
> > +	kfree(dev->alt_max_pkt_size_isoc);
> > +	kfree(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(em28xx_free_device);
> >   
> >   /*
> >    * em28xx_init_dev()
> > @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> >   			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
> >   	}
> >   
> > +	kref_init(&dev->ref);
> > +
> >   	request_modules(dev);
> >   
> >   	/* Should be the last thing to do, to avoid newer udev's to
> > @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
> >   
> >   	em28xx_close_extension(dev);
> >   
> > -	em28xx_release_resources(dev);
> > -
> > -	if (!dev->users) {
> > -		kfree(dev->alt_max_pkt_size_isoc);
> > -		kfree(dev);
> > -	}
> > +	kref_put(&dev->ref, em28xx_free_device);
> >   }
> >   
> >   static struct usb_driver em28xx_usb_driver = {
> > diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> > index 5ea563e3f0e4..8674ae5fce06 100644
> > --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> > +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> > @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >   	em28xx_info("Binding DVB extension\n");
> >   
> >   	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
> > -
> >   	if (dvb == NULL) {
> >   		em28xx_info("em28xx_dvb: memory allocation failed\n");
> >   		return -ENOMEM;
> >   	}
> > +	kref_get(&dev->ref);
> >   	dev->dvb = dvb;
> >   	dvb->fe[0] = dvb->fe[1] = NULL;
> >   
> > @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >   	dvb->adapter.mfe_shared = mfe_shared;
> >   
> >   	em28xx_info("DVB extension successfully initialized\n");
> > +
> >   ret:
> >   	em28xx_set_mode(dev, EM28XX_SUSPEND);
> >   	mutex_unlock(&dev->lock);
> > @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
> >   		dev->dvb = NULL;
> >   	}
> >   
> > +	kref_put(&dev->ref, em28xx_free_device);
> > +
> >   	return 0;
> >   }
> >   
> > diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> > index 61c061f3a476..33388b5922a0 100644
> > --- a/drivers/media/usb/em28xx/em28xx-input.c
> > +++ b/drivers/media/usb/em28xx/em28xx-input.c
> > @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
> >   		return 0;
> >   	}
> >   
> > +	kref_get(&dev->ref);
> > +
> >   	if (dev->board.buttons)
> >   		em28xx_init_buttons(dev);
> >   
> > @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >   
> >   	/* skip detach on non attached boards */
> >   	if (!ir)
> > -		return 0;
> > +		goto ref_put;
> >   
> >   	if (ir->rc)
> >   		rc_unregister_device(ir->rc);
> > @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >   	/* done */
> >   	kfree(ir);
> >   	dev->ir = NULL;
> > +
> > +ref_put:
> > +	kref_put(&dev->ref, em28xx_free_device);
> > +
> >   	return 0;
> >   }
> >   
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index 587ff3fe9402..dc10cec772ba 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> >   	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> >   	v4l2_device_unregister(&dev->v4l2_dev);
> >   
> > -	if (dev->users)
> > -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
> > +	kref_put(&dev->ref, em28xx_free_device);
> >   
> >   	return 0;
> >   }
> > @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
> >   	mutex_lock(&dev->lock);
> >   
> >   	if (dev->users == 1) {
> > -		/* free the remaining resources if device is disconnected */
> > -		if (dev->disconnected) {
> > -			kfree(dev->alt_max_pkt_size_isoc);
> > +		/* No sense to try to write to the device */
> > +		if (dev->disconnected)
> >   			goto exit;
> > -		}
> >   
> >   		/* Save some power by putting tuner to sleep */
> >   		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> > @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> >   
> >   	em28xx_info("Registering V4L2 extension\n");
> >   
> > +	kref_get(&dev->ref);
> > +
> >   	mutex_lock(&dev->lock);
> >   
> >   	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> > diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> > index 5d5d1b6f0294..d38c08e4da60 100644
> > --- a/drivers/media/usb/em28xx/em28xx.h
> > +++ b/drivers/media/usb/em28xx/em28xx.h
> > @@ -32,6 +32,7 @@
> >   #include <linux/workqueue.h>
> >   #include <linux/i2c.h>
> >   #include <linux/mutex.h>
> > +#include <linux/kref.h>
> >   #include <linux/videodev2.h>
> >   
> >   #include <media/videobuf2-vmalloc.h>
> > @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
> >   	enum em28xx_i2c_algo_type algo_type;
> >   };
> >   
> > -
> >   /* main device struct */
> >   struct em28xx {
> > +	struct kref ref;
> > +
> > +
> >   	/* generic device properties */
> >   	char name[30];		/* name (including minor) of the device */
> >   	int model;		/* index in the device_data struct */
> > @@ -706,6 +709,8 @@ struct em28xx {
> >   	struct em28xx_dvb *dvb;
> >   };
> >   
> > +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
> > +
> >   struct em28xx_ops {
> >   	struct list_head next;
> >   	char *name;
> > @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
> >   extern struct usb_device_id em28xx_id_table[];
> >   int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
> >   void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
> > -void em28xx_release_resources(struct em28xx *dev);
> > +void em28xx_free_device(struct kref *ref);
> >   
> >   /* Provided by em28xx-camera.c */
> >   int em28xx_detect_sensor(struct em28xx *dev);
> I welcome this patch and the general approach looks good.
> I had started working on the same issue, but it's not that trivial.
> 
> At first glance there seem to be several issues, but I will need to 
> review this patch in more detail and also make some tests.
> Unfortunately, I don't have much time this evening, So could you please 
> hold it back another day ?
> I hope I can review the other remaining patch of this series (patch 5/7) 
> later this evening.

We're running out of time for 3.14. I think we should merge this patch
series, and your patch series for 3.14, to be together with the em28xx-v4l
split.

My plan is to merge the remaining patches for 3.14 today or, in the worse
case, tomorrow.

If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
series gets merged, but, if we miss the bus, I'm afraid that we'll end
by having more problems that will be hard to fix with trivial patches, due
to em28xx-v4l split changes that also affected the driver removal and device
close code.

FYI, I tested this code and also Antti with our devices, randomly removing
the devices while streaming, and this is now finally working.

I won't doubt that there are some cases that require fixes (and
it seems that em28xx-rc has one of such corner cases), but they'll likely
can be solved with somewhat short and trivial patches.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frank Schäfer Jan. 13, 2014, 9:55 p.m. UTC | #3

Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
> Em Mon, 13 Jan 2014 20:02:19 +0100
> Frank Schäfer <[e-mail]> escreveu:
>
>> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
>>> We can't free struct em28xx while one of the extensions is still
>>> using it.
>>>
>>> So, add a kref() to control it, freeing it only after the
>>> extensions fini calls.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>>> ---
>>>   drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
>>>   drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
>>>   drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
>>>   drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
>>>   drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
>>>   drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
>>>   6 files changed, 44 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>> index 97d9105e6830..8e959dae8358 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
>>>   
>>>   	em28xx_info("Binding audio extension\n");
>>>   
>>> +	kref_get(&dev->ref);
>>> +
>>>   	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
>>>   			 "Rechberger\n");
>>>   	printk(KERN_INFO
>>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>   	if (dev == NULL)
>>>   		return 0;
>>>   
>>> -	if (dev->has_alsa_audio != 1) {
>>> +	if (!dev->has_alsa_audio) {
>>>   		/* This device does not support the extension (in this case
>>>   		   the device is expecting the snd-usb-audio module or
>>>   		   doesn't have analog audio support at all) */
>>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>   		dev->adev.sndcard = NULL;
>>>   	}
>>>   
>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>   	return 0;
>>>   }
>>>   
>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>> index 3b332d527ccb..df92f417634a 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
>>>   	flush_work(&dev->request_module_wk);
>>>   }
>>>   
>>> -/*
>>> - * em28xx_release_resources()
>>> - * unregisters the v4l2,i2c and usb devices
>>> - * called when the device gets disconnected or at module unload
>>> -*/
>>> -void em28xx_release_resources(struct em28xx *dev)
>>> +/**
>>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
>>> + *
>>> + * @ref: struct kref for em28xx device
>>> + *
>>> + * This is called when all extensions and em28xx core unregisters a device
>>> + */
>>> +void em28xx_free_device(struct kref *ref)
>>>   {
>>> -	/*FIXME: I2C IR should be disconnected */
>>> +	struct em28xx *dev = kref_to_dev(ref);
>>>   
>>> -	mutex_lock(&dev->lock);
>>> +	em28xx_info("Freeing device\n");
>>>   
>>>   	if (dev->def_i2c_bus)
>>>   		em28xx_i2c_unregister(dev, 1);
>>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
>>>   	/* Mark device as unused */
>>>   	clear_bit(dev->devno, &em28xx_devused);
>>>   
>>> -	mutex_unlock(&dev->lock);
>>> -};
>>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
>>> +	kfree(dev->alt_max_pkt_size_isoc);
>>> +	kfree(dev);
>>> +}
>>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
>>>   
>>>   /*
>>>    * em28xx_init_dev()
>>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>>   			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
>>>   	}
>>>   
>>> +	kref_init(&dev->ref);
>>> +
>>>   	request_modules(dev);
>>>   
>>>   	/* Should be the last thing to do, to avoid newer udev's to
>>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>>>   
>>>   	em28xx_close_extension(dev);
>>>   
>>> -	em28xx_release_resources(dev);
>>> -
>>> -	if (!dev->users) {
>>> -		kfree(dev->alt_max_pkt_size_isoc);
>>> -		kfree(dev);
>>> -	}
>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>   }
>>>   
>>>   static struct usb_driver em28xx_usb_driver = {
>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>> index 5ea563e3f0e4..8674ae5fce06 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>   	em28xx_info("Binding DVB extension\n");
>>>   
>>>   	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
>>> -
>>>   	if (dvb == NULL) {
>>>   		em28xx_info("em28xx_dvb: memory allocation failed\n");
>>>   		return -ENOMEM;
>>>   	}
>>> +	kref_get(&dev->ref);
>>>   	dev->dvb = dvb;
>>>   	dvb->fe[0] = dvb->fe[1] = NULL;
>>>   
>>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>   	dvb->adapter.mfe_shared = mfe_shared;
>>>   
>>>   	em28xx_info("DVB extension successfully initialized\n");
>>> +
>>>   ret:
>>>   	em28xx_set_mode(dev, EM28XX_SUSPEND);
>>>   	mutex_unlock(&dev->lock);
>>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
>>>   		dev->dvb = NULL;
>>>   	}
>>>   
>>> +	kref_put(&dev->ref, em28xx_free_device);
>>> +
>>>   	return 0;
>>>   }
>>>   
>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>>> index 61c061f3a476..33388b5922a0 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
>>>   		return 0;
>>>   	}
>>>   
>>> +	kref_get(&dev->ref);
>>> +
>>>   	if (dev->board.buttons)
>>>   		em28xx_init_buttons(dev);
>>>   
>>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>   
>>>   	/* skip detach on non attached boards */
>>>   	if (!ir)
>>> -		return 0;
>>> +		goto ref_put;
>>>   
>>>   	if (ir->rc)
>>>   		rc_unregister_device(ir->rc);
>>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>   	/* done */
>>>   	kfree(ir);
>>>   	dev->ir = NULL;
>>> +
>>> +ref_put:
>>> +	kref_put(&dev->ref, em28xx_free_device);
>>> +
>>>   	return 0;
>>>   }
>>>   
>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>> index 587ff3fe9402..dc10cec772ba 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>>>   	v4l2_ctrl_handler_free(&dev->ctrl_handler);
>>>   	v4l2_device_unregister(&dev->v4l2_dev);
>>>   
>>> -	if (dev->users)
>>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
>>>   	mutex_lock(&dev->lock);
>>>   
>>>   	if (dev->users == 1) {
>>> -		/* free the remaining resources if device is disconnected */
>>> -		if (dev->disconnected) {
>>> -			kfree(dev->alt_max_pkt_size_isoc);
>>> +		/* No sense to try to write to the device */
>>> +		if (dev->disconnected)
>>>   			goto exit;
>>> -		}
>>>   
>>>   		/* Save some power by putting tuner to sleep */
>>>   		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>>   
>>>   	em28xx_info("Registering V4L2 extension\n");
>>>   
>>> +	kref_get(&dev->ref);
>>> +
>>>   	mutex_lock(&dev->lock);
>>>   
>>>   	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>> index 5d5d1b6f0294..d38c08e4da60 100644
>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>> @@ -32,6 +32,7 @@
>>>   #include <linux/workqueue.h>
>>>   #include <linux/i2c.h>
>>>   #include <linux/mutex.h>
>>> +#include <linux/kref.h>
>>>   #include <linux/videodev2.h>
>>>   
>>>   #include <media/videobuf2-vmalloc.h>
>>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
>>>   	enum em28xx_i2c_algo_type algo_type;
>>>   };
>>>   
>>> -
>>>   /* main device struct */
>>>   struct em28xx {
>>> +	struct kref ref;
>>> +
>>> +
>>>   	/* generic device properties */
>>>   	char name[30];		/* name (including minor) of the device */
>>>   	int model;		/* index in the device_data struct */
>>> @@ -706,6 +709,8 @@ struct em28xx {
>>>   	struct em28xx_dvb *dvb;
>>>   };
>>>   
>>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
>>> +
>>>   struct em28xx_ops {
>>>   	struct list_head next;
>>>   	char *name;
>>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
>>>   extern struct usb_device_id em28xx_id_table[];
>>>   int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
>>>   void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
>>> -void em28xx_release_resources(struct em28xx *dev);
>>> +void em28xx_free_device(struct kref *ref);
>>>   
>>>   /* Provided by em28xx-camera.c */
>>>   int em28xx_detect_sensor(struct em28xx *dev);
>> I welcome this patch and the general approach looks good.
>> I had started working on the same issue, but it's not that trivial.
>>
>> At first glance there seem to be several issues, but I will need to 
>> review this patch in more detail and also make some tests.
>> Unfortunately, I don't have much time this evening, So could you please 
>> hold it back another day ?
>> I hope I can review the other remaining patch of this series (patch 5/7) 
>> later this evening.
> We're running out of time for 3.14. I think we should merge this patch
> series, and your patch series for 3.14, to be together with the em28xx-v4l
> split.
>
> My plan is to merge the remaining patches for 3.14 today or, in the worse
> case, tomorrow.
>
> If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
> series gets merged, but, if we miss the bus, I'm afraid that we'll end
> by having more problems that will be hard to fix with trivial patches, due
> to em28xx-v4l split changes that also affected the driver removal and device
> close code.
>
> FYI, I tested this code and also Antti with our devices, randomly removing
> the devices while streaming, and this is now finally working.
>
> I won't doubt that there are some cases that require fixes (and
> it seems that em28xx-rc has one of such corner cases), but they'll likely
> can be solved with somewhat short and trivial patches.
>
> Cheers,
> Mauro

This is a very critical patch and exactly the kind of change that should
_never_ be hurried !
FAICS it has some severe issues and it's not clear how easy it will be
to fix it within the the 3.14-rc cycle.
As long as it's not ready, don't merge it for 3.14.

em28xx resources releasing is broken since ... well... at least 2 years.
3.14 will already be much better and nobody will care if this remaining
issue is addressed a kernel release later.

Be warned !


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Jan. 14, 2014, 1:10 p.m. UTC | #4

Em Mon, 13 Jan 2014 22:55:36 +0100
Frank Schäfer <[e-mail]> escreveu:

> Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
> > Em Mon, 13 Jan 2014 20:02:19 +0100
> > Frank Schäfer <[e-mail]> escreveu:
> >
> >> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
> >>> We can't free struct em28xx while one of the extensions is still
> >>> using it.
> >>>
> >>> So, add a kref() to control it, freeing it only after the
> >>> extensions fini calls.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> >>> ---
> >>>   drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
> >>>   drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
> >>>   drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
> >>>   drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
> >>>   drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
> >>>   drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
> >>>   6 files changed, 44 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> >>> index 97d9105e6830..8e959dae8358 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> >>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
> >>>   
> >>>   	em28xx_info("Binding audio extension\n");
> >>>   
> >>> +	kref_get(&dev->ref);
> >>> +
> >>>   	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
> >>>   			 "Rechberger\n");
> >>>   	printk(KERN_INFO
> >>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >>>   	if (dev == NULL)
> >>>   		return 0;
> >>>   
> >>> -	if (dev->has_alsa_audio != 1) {
> >>> +	if (!dev->has_alsa_audio) {
> >>>   		/* This device does not support the extension (in this case
> >>>   		   the device is expecting the snd-usb-audio module or
> >>>   		   doesn't have analog audio support at all) */
> >>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >>>   		dev->adev.sndcard = NULL;
> >>>   	}
> >>>   
> >>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>   	return 0;
> >>>   }
> >>>   
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >>> index 3b332d527ccb..df92f417634a 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
> >>>   	flush_work(&dev->request_module_wk);
> >>>   }
> >>>   
> >>> -/*
> >>> - * em28xx_release_resources()
> >>> - * unregisters the v4l2,i2c and usb devices
> >>> - * called when the device gets disconnected or at module unload
> >>> -*/
> >>> -void em28xx_release_resources(struct em28xx *dev)
> >>> +/**
> >>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
> >>> + *
> >>> + * @ref: struct kref for em28xx device
> >>> + *
> >>> + * This is called when all extensions and em28xx core unregisters a device
> >>> + */
> >>> +void em28xx_free_device(struct kref *ref)
> >>>   {
> >>> -	/*FIXME: I2C IR should be disconnected */
> >>> +	struct em28xx *dev = kref_to_dev(ref);
> >>>   
> >>> -	mutex_lock(&dev->lock);
> >>> +	em28xx_info("Freeing device\n");
> >>>   
> >>>   	if (dev->def_i2c_bus)
> >>>   		em28xx_i2c_unregister(dev, 1);
> >>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
> >>>   	/* Mark device as unused */
> >>>   	clear_bit(dev->devno, &em28xx_devused);
> >>>   
> >>> -	mutex_unlock(&dev->lock);
> >>> -};
> >>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
> >>> +	kfree(dev->alt_max_pkt_size_isoc);
> >>> +	kfree(dev);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
> >>>   
> >>>   /*
> >>>    * em28xx_init_dev()
> >>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> >>>   			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
> >>>   	}
> >>>   
> >>> +	kref_init(&dev->ref);
> >>> +
> >>>   	request_modules(dev);
> >>>   
> >>>   	/* Should be the last thing to do, to avoid newer udev's to
> >>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
> >>>   
> >>>   	em28xx_close_extension(dev);
> >>>   
> >>> -	em28xx_release_resources(dev);
> >>> -
> >>> -	if (!dev->users) {
> >>> -		kfree(dev->alt_max_pkt_size_isoc);
> >>> -		kfree(dev);
> >>> -	}
> >>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>   }
> >>>   
> >>>   static struct usb_driver em28xx_usb_driver = {
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>> index 5ea563e3f0e4..8674ae5fce06 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >>>   	em28xx_info("Binding DVB extension\n");
> >>>   
> >>>   	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
> >>> -
> >>>   	if (dvb == NULL) {
> >>>   		em28xx_info("em28xx_dvb: memory allocation failed\n");
> >>>   		return -ENOMEM;
> >>>   	}
> >>> +	kref_get(&dev->ref);
> >>>   	dev->dvb = dvb;
> >>>   	dvb->fe[0] = dvb->fe[1] = NULL;
> >>>   
> >>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >>>   	dvb->adapter.mfe_shared = mfe_shared;
> >>>   
> >>>   	em28xx_info("DVB extension successfully initialized\n");
> >>> +
> >>>   ret:
> >>>   	em28xx_set_mode(dev, EM28XX_SUSPEND);
> >>>   	mutex_unlock(&dev->lock);
> >>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
> >>>   		dev->dvb = NULL;
> >>>   	}
> >>>   
> >>> +	kref_put(&dev->ref, em28xx_free_device);
> >>> +
> >>>   	return 0;
> >>>   }
> >>>   
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> >>> index 61c061f3a476..33388b5922a0 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-input.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> >>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
> >>>   		return 0;
> >>>   	}
> >>>   
> >>> +	kref_get(&dev->ref);
> >>> +
> >>>   	if (dev->board.buttons)
> >>>   		em28xx_init_buttons(dev);
> >>>   
> >>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >>>   
> >>>   	/* skip detach on non attached boards */
> >>>   	if (!ir)
> >>> -		return 0;
> >>> +		goto ref_put;
> >>>   
> >>>   	if (ir->rc)
> >>>   		rc_unregister_device(ir->rc);
> >>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >>>   	/* done */
> >>>   	kfree(ir);
> >>>   	dev->ir = NULL;
> >>> +
> >>> +ref_put:
> >>> +	kref_put(&dev->ref, em28xx_free_device);
> >>> +
> >>>   	return 0;
> >>>   }
> >>>   
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> >>> index 587ff3fe9402..dc10cec772ba 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-video.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> >>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> >>>   	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> >>>   	v4l2_device_unregister(&dev->v4l2_dev);
> >>>   
> >>> -	if (dev->users)
> >>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
> >>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>   
> >>>   	return 0;
> >>>   }
> >>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
> >>>   	mutex_lock(&dev->lock);
> >>>   
> >>>   	if (dev->users == 1) {
> >>> -		/* free the remaining resources if device is disconnected */
> >>> -		if (dev->disconnected) {
> >>> -			kfree(dev->alt_max_pkt_size_isoc);
> >>> +		/* No sense to try to write to the device */
> >>> +		if (dev->disconnected)
> >>>   			goto exit;
> >>> -		}
> >>>   
> >>>   		/* Save some power by putting tuner to sleep */
> >>>   		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> >>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> >>>   
> >>>   	em28xx_info("Registering V4L2 extension\n");
> >>>   
> >>> +	kref_get(&dev->ref);
> >>> +
> >>>   	mutex_lock(&dev->lock);
> >>>   
> >>>   	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> >>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> >>> index 5d5d1b6f0294..d38c08e4da60 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx.h
> >>> +++ b/drivers/media/usb/em28xx/em28xx.h
> >>> @@ -32,6 +32,7 @@
> >>>   #include <linux/workqueue.h>
> >>>   #include <linux/i2c.h>
> >>>   #include <linux/mutex.h>
> >>> +#include <linux/kref.h>
> >>>   #include <linux/videodev2.h>
> >>>   
> >>>   #include <media/videobuf2-vmalloc.h>
> >>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
> >>>   	enum em28xx_i2c_algo_type algo_type;
> >>>   };
> >>>   
> >>> -
> >>>   /* main device struct */
> >>>   struct em28xx {
> >>> +	struct kref ref;
> >>> +
> >>> +
> >>>   	/* generic device properties */
> >>>   	char name[30];		/* name (including minor) of the device */
> >>>   	int model;		/* index in the device_data struct */
> >>> @@ -706,6 +709,8 @@ struct em28xx {
> >>>   	struct em28xx_dvb *dvb;
> >>>   };
> >>>   
> >>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
> >>> +
> >>>   struct em28xx_ops {
> >>>   	struct list_head next;
> >>>   	char *name;
> >>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
> >>>   extern struct usb_device_id em28xx_id_table[];
> >>>   int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
> >>>   void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
> >>> -void em28xx_release_resources(struct em28xx *dev);
> >>> +void em28xx_free_device(struct kref *ref);
> >>>   
> >>>   /* Provided by em28xx-camera.c */
> >>>   int em28xx_detect_sensor(struct em28xx *dev);
> >> I welcome this patch and the general approach looks good.
> >> I had started working on the same issue, but it's not that trivial.
> >>
> >> At first glance there seem to be several issues, but I will need to 
> >> review this patch in more detail and also make some tests.
> >> Unfortunately, I don't have much time this evening, So could you please 
> >> hold it back another day ?
> >> I hope I can review the other remaining patch of this series (patch 5/7) 
> >> later this evening.
> > We're running out of time for 3.14. I think we should merge this patch
> > series, and your patch series for 3.14, to be together with the em28xx-v4l
> > split.
> >
> > My plan is to merge the remaining patches for 3.14 today or, in the worse
> > case, tomorrow.
> >
> > If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
> > series gets merged, but, if we miss the bus, I'm afraid that we'll end
> > by having more problems that will be hard to fix with trivial patches, due
> > to em28xx-v4l split changes that also affected the driver removal and device
> > close code.
> >
> > FYI, I tested this code and also Antti with our devices, randomly removing
> > the devices while streaming, and this is now finally working.
> >
> > I won't doubt that there are some cases that require fixes (and
> > it seems that em28xx-rc has one of such corner cases), but they'll likely
> > can be solved with somewhat short and trivial patches.
> >
> > Cheers,
> > Mauro
> 
> This is a very critical patch and exactly the kind of change that should
> _never_ be hurried !
> FAICS it has some severe issues and it's not clear how easy it will be
> to fix it within the the 3.14-rc cycle.
> As long as it's not ready, don't merge it for 3.14.

What issues? So far, you didn't point any. On both my tests and Antti's one,
with this series, there were significant improvements on removing existing
bugs with device removal.

> em28xx resources releasing is broken since ... well... at least 2 years.
> 3.14 will already be much better and nobody will care if this remaining
> issue is addressed a kernel release later.

Although I think that we're properly releasing resources, I'm a way less
concerned about keeping some leaked memory while releasing them than I am 
concerned that a device removal that would cause an OOPS, or a deadly
crash at the USB or ALSA stack that prevents other devices to be probed.

Due to em28xx-v4l calling em28xx_release_resources(), now both the
USB and ALSA stacks crashes if you remove a device while ALSA is streaming.

That happens because em28xx, I2C, etc will be freed by em28xx-v4l, but
those resources are still needed by em28xx-alsa. That makes that the
.fini code there to cause crash at ALSA stack, and, sometimes, at the
USB stack.

As nowadays lots of components depend on pulseaudio, the ALSA crash
causes pulse to stop work, likely keeping it into some dead lock status.
This makes the entire machine to become really slow, when it doesn't
crash.

This is a serious regression that should be fixed.

This patch series for sure fixes it. As I said, there are two independent
series of tests verifying that (both using several different em28xx 
devices).

> 
> Be warned !
> 
>

Frank Schäfer Jan. 14, 2014, 6:13 p.m. UTC | #5

On 14.01.2014 14:10, Mauro Carvalho Chehab wrote:
> Em Mon, 13 Jan 2014 22:55:36 +0100
> Frank Schäfer <[e-mail]> escreveu:
>
>> Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
>>> Em Mon, 13 Jan 2014 20:02:19 +0100
>>> Frank Schäfer <[e-mail]> escreveu:
>>>
>>>> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
>>>>> We can't free struct em28xx while one of the extensions is still
>>>>> using it.
>>>>>
>>>>> So, add a kref() to control it, freeing it only after the
>>>>> extensions fini calls.
>>>>>
>>>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>>>>> ---
>>>>>    drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
>>>>>    drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
>>>>>    drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
>>>>>    drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
>>>>>    drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
>>>>>    drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
>>>>>    6 files changed, 44 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> index 97d9105e6830..8e959dae8358 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
>>>>>    
>>>>>    	em28xx_info("Binding audio extension\n");
>>>>>    
>>>>> +	kref_get(&dev->ref);
>>>>> +
>>>>>    	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
>>>>>    			 "Rechberger\n");
>>>>>    	printk(KERN_INFO
>>>>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>>>    	if (dev == NULL)
>>>>>    		return 0;
>>>>>    
>>>>> -	if (dev->has_alsa_audio != 1) {
>>>>> +	if (!dev->has_alsa_audio) {
>>>>>    		/* This device does not support the extension (in this case
>>>>>    		   the device is expecting the snd-usb-audio module or
>>>>>    		   doesn't have analog audio support at all) */
>>>>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>>>    		dev->adev.sndcard = NULL;
>>>>>    	}
>>>>>    
>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>> index 3b332d527ccb..df92f417634a 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
>>>>>    	flush_work(&dev->request_module_wk);
>>>>>    }
>>>>>    
>>>>> -/*
>>>>> - * em28xx_release_resources()
>>>>> - * unregisters the v4l2,i2c and usb devices
>>>>> - * called when the device gets disconnected or at module unload
>>>>> -*/
>>>>> -void em28xx_release_resources(struct em28xx *dev)
>>>>> +/**
>>>>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
>>>>> + *
>>>>> + * @ref: struct kref for em28xx device
>>>>> + *
>>>>> + * This is called when all extensions and em28xx core unregisters a device
>>>>> + */
>>>>> +void em28xx_free_device(struct kref *ref)
>>>>>    {
>>>>> -	/*FIXME: I2C IR should be disconnected */
>>>>> +	struct em28xx *dev = kref_to_dev(ref);
>>>>>    
>>>>> -	mutex_lock(&dev->lock);
>>>>> +	em28xx_info("Freeing device\n");
>>>>>    
>>>>>    	if (dev->def_i2c_bus)
>>>>>    		em28xx_i2c_unregister(dev, 1);
>>>>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
>>>>>    	/* Mark device as unused */
>>>>>    	clear_bit(dev->devno, &em28xx_devused);
>>>>>    
>>>>> -	mutex_unlock(&dev->lock);
>>>>> -};
>>>>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
>>>>> +	kfree(dev->alt_max_pkt_size_isoc);
>>>>> +	kfree(dev);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
>>>>>    
>>>>>    /*
>>>>>     * em28xx_init_dev()
>>>>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>>>>    			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
>>>>>    	}
>>>>>    
>>>>> +	kref_init(&dev->ref);
>>>>> +
>>>>>    	request_modules(dev);
>>>>>    
>>>>>    	/* Should be the last thing to do, to avoid newer udev's to
>>>>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>>>>>    
>>>>>    	em28xx_close_extension(dev);
>>>>>    
>>>>> -	em28xx_release_resources(dev);
>>>>> -
>>>>> -	if (!dev->users) {
>>>>> -		kfree(dev->alt_max_pkt_size_isoc);
>>>>> -		kfree(dev);
>>>>> -	}
>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>    }
>>>>>    
>>>>>    static struct usb_driver em28xx_usb_driver = {
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>> index 5ea563e3f0e4..8674ae5fce06 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>>>    	em28xx_info("Binding DVB extension\n");
>>>>>    
>>>>>    	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
>>>>> -
>>>>>    	if (dvb == NULL) {
>>>>>    		em28xx_info("em28xx_dvb: memory allocation failed\n");
>>>>>    		return -ENOMEM;
>>>>>    	}
>>>>> +	kref_get(&dev->ref);
>>>>>    	dev->dvb = dvb;
>>>>>    	dvb->fe[0] = dvb->fe[1] = NULL;
>>>>>    
>>>>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>>>    	dvb->adapter.mfe_shared = mfe_shared;
>>>>>    
>>>>>    	em28xx_info("DVB extension successfully initialized\n");
>>>>> +
>>>>>    ret:
>>>>>    	em28xx_set_mode(dev, EM28XX_SUSPEND);
>>>>>    	mutex_unlock(&dev->lock);
>>>>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
>>>>>    		dev->dvb = NULL;
>>>>>    	}
>>>>>    
>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>> +
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>>>>> index 61c061f3a476..33388b5922a0 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>>>>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
>>>>>    		return 0;
>>>>>    	}
>>>>>    
>>>>> +	kref_get(&dev->ref);
>>>>> +
>>>>>    	if (dev->board.buttons)
>>>>>    		em28xx_init_buttons(dev);
>>>>>    
>>>>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>>>    
>>>>>    	/* skip detach on non attached boards */
>>>>>    	if (!ir)
>>>>> -		return 0;
>>>>> +		goto ref_put;
>>>>>    
>>>>>    	if (ir->rc)
>>>>>    		rc_unregister_device(ir->rc);
>>>>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>>>    	/* done */
>>>>>    	kfree(ir);
>>>>>    	dev->ir = NULL;
>>>>> +
>>>>> +ref_put:
>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>> +
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>>>> index 587ff3fe9402..dc10cec772ba 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>>>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>>>>>    	v4l2_ctrl_handler_free(&dev->ctrl_handler);
>>>>>    	v4l2_device_unregister(&dev->v4l2_dev);
>>>>>    
>>>>> -	if (dev->users)
>>>>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>    
>>>>>    	return 0;
>>>>>    }
>>>>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
>>>>>    	mutex_lock(&dev->lock);
>>>>>    
>>>>>    	if (dev->users == 1) {
>>>>> -		/* free the remaining resources if device is disconnected */
>>>>> -		if (dev->disconnected) {
>>>>> -			kfree(dev->alt_max_pkt_size_isoc);
>>>>> +		/* No sense to try to write to the device */
>>>>> +		if (dev->disconnected)
>>>>>    			goto exit;
>>>>> -		}
>>>>>    
>>>>>    		/* Save some power by putting tuner to sleep */
>>>>>    		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>>>>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>>>>    
>>>>>    	em28xx_info("Registering V4L2 extension\n");
>>>>>    
>>>>> +	kref_get(&dev->ref);
>>>>> +
>>>>>    	mutex_lock(&dev->lock);
>>>>>    
>>>>>    	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>>>> index 5d5d1b6f0294..d38c08e4da60 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>>>> @@ -32,6 +32,7 @@
>>>>>    #include <linux/workqueue.h>
>>>>>    #include <linux/i2c.h>
>>>>>    #include <linux/mutex.h>
>>>>> +#include <linux/kref.h>
>>>>>    #include <linux/videodev2.h>
>>>>>    
>>>>>    #include <media/videobuf2-vmalloc.h>
>>>>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
>>>>>    	enum em28xx_i2c_algo_type algo_type;
>>>>>    };
>>>>>    
>>>>> -
>>>>>    /* main device struct */
>>>>>    struct em28xx {
>>>>> +	struct kref ref;
>>>>> +
>>>>> +
>>>>>    	/* generic device properties */
>>>>>    	char name[30];		/* name (including minor) of the device */
>>>>>    	int model;		/* index in the device_data struct */
>>>>> @@ -706,6 +709,8 @@ struct em28xx {
>>>>>    	struct em28xx_dvb *dvb;
>>>>>    };
>>>>>    
>>>>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
>>>>> +
>>>>>    struct em28xx_ops {
>>>>>    	struct list_head next;
>>>>>    	char *name;
>>>>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
>>>>>    extern struct usb_device_id em28xx_id_table[];
>>>>>    int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
>>>>>    void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
>>>>> -void em28xx_release_resources(struct em28xx *dev);
>>>>> +void em28xx_free_device(struct kref *ref);
>>>>>    
>>>>>    /* Provided by em28xx-camera.c */
>>>>>    int em28xx_detect_sensor(struct em28xx *dev);
>>>> I welcome this patch and the general approach looks good.
>>>> I had started working on the same issue, but it's not that trivial.
>>>>
>>>> At first glance there seem to be several issues, but I will need to
>>>> review this patch in more detail and also make some tests.
>>>> Unfortunately, I don't have much time this evening, So could you please
>>>> hold it back another day ?
>>>> I hope I can review the other remaining patch of this series (patch 5/7)
>>>> later this evening.
>>> We're running out of time for 3.14. I think we should merge this patch
>>> series, and your patch series for 3.14, to be together with the em28xx-v4l
>>> split.
>>>
>>> My plan is to merge the remaining patches for 3.14 today or, in the worse
>>> case, tomorrow.
>>>
>>> If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
>>> series gets merged, but, if we miss the bus, I'm afraid that we'll end
>>> by having more problems that will be hard to fix with trivial patches, due
>>> to em28xx-v4l split changes that also affected the driver removal and device
>>> close code.
>>>
>>> FYI, I tested this code and also Antti with our devices, randomly removing
>>> the devices while streaming, and this is now finally working.
>>>
>>> I won't doubt that there are some cases that require fixes (and
>>> it seems that em28xx-rc has one of such corner cases), but they'll likely
>>> can be solved with somewhat short and trivial patches.
>>>
>>> Cheers,
>>> Mauro
>> This is a very critical patch and exactly the kind of change that should
>> _never_ be hurried !
>> FAICS it has some severe issues and it's not clear how easy it will be
>> to fix it within the the 3.14-rc cycle.
>> As long as it's not ready, don't merge it for 3.14.
> What issues? So far, you didn't point any.
I already stated that I didn't have the time yet to review and test it 
in detail, but I'm going to do that as soon as possible.
If you can't wait, there's nothing I can do, sorry.

At first glance it seems there are at least 2 issues:
1.) use after freeing in v4l-extension (happens when the device is 
closed after the usb disconnect)
2.) error paths in the init() functions ?


> On both my tests and Antti's one,
> with this series, there were significant improvements on removing existing
> bugs with device removal.
I'm talking about this specific patch here, not the whole series.
I have no objections against the rest of the series (well, with the 
exception of patch 5 at the moment).

>
>> em28xx resources releasing is broken since ... well... at least 2 years.
>> 3.14 will already be much better and nobody will care if this remaining
>> issue is addressed a kernel release later.
> Although I think that we're properly releasing resources, I'm a way less
> concerned about keeping some leaked memory while releasing them than I am
> concerned that a device removal that would cause an OOPS, or a deadly
> crash at the USB or ALSA stack that prevents other devices to be probed.
>
> Due to em28xx-v4l calling em28xx_release_resources(), now both the
> USB and ALSA stacks crashes if you remove a device while ALSA is streaming.
>
> That happens because em28xx, I2C, etc will be freed by em28xx-v4l, but
> those resources are still needed by em28xx-alsa. That makes that the
> .fini code there to cause crash at ALSA stack, and, sometimes, at the
> USB stack.

That's not true anymore, em28xx_release_resources() is now only called 
by the usb disconnect handler in the core _after_ all fini() functions 
have been called.
Maybe you tested that without my patch series ? See patch 7/8.

> As nowadays lots of components depend on pulseaudio, the ALSA crash
> causes pulse to stop work, likely keeping it into some dead lock status.
> This makes the entire machine to become really slow, when it doesn't
> crash.
>
> This is a serious regression that should be fixed.
>
> This patch series for sure fixes it. As I said, there are two independent
> series of tests verifying that (both using several different em28xx
> devices).
>
>> Be warned !
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Jan. 14, 2014, 6:55 p.m. UTC | #6

Em Tue, 14 Jan 2014 19:13:00 +0100
Frank Schäfer <[e-mail]> escreveu:

> On 14.01.2014 14:10, Mauro Carvalho Chehab wrote:
> > Em Mon, 13 Jan 2014 22:55:36 +0100
> > Frank Schäfer <[e-mail]> escreveu:
> >
> >> Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
> >>> Em Mon, 13 Jan 2014 20:02:19 +0100
> >>> Frank Schäfer <[e-mail]> escreveu:
> >>>
> >>>> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
> >>>>> We can't free struct em28xx while one of the extensions is still
> >>>>> using it.
> >>>>>
> >>>>> So, add a kref() to control it, freeing it only after the
> >>>>> extensions fini calls.
> >>>>>
> >>>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> >>>>> ---
> >>>>>    drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
> >>>>>    drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
> >>>>>    drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
> >>>>>    drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
> >>>>>    drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
> >>>>>    drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
> >>>>>    6 files changed, 44 insertions(+), 28 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> >>>>> index 97d9105e6830..8e959dae8358 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> >>>>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
> >>>>>    
> >>>>>    	em28xx_info("Binding audio extension\n");
> >>>>>    
> >>>>> +	kref_get(&dev->ref);
> >>>>> +
> >>>>>    	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
> >>>>>    			 "Rechberger\n");
> >>>>>    	printk(KERN_INFO
> >>>>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >>>>>    	if (dev == NULL)
> >>>>>    		return 0;
> >>>>>    
> >>>>> -	if (dev->has_alsa_audio != 1) {
> >>>>> +	if (!dev->has_alsa_audio) {
> >>>>>    		/* This device does not support the extension (in this case
> >>>>>    		   the device is expecting the snd-usb-audio module or
> >>>>>    		   doesn't have analog audio support at all) */
> >>>>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> >>>>>    		dev->adev.sndcard = NULL;
> >>>>>    	}
> >>>>>    
> >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>>>    	return 0;
> >>>>>    }
> >>>>>    
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>>> index 3b332d527ccb..df92f417634a 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
> >>>>>    	flush_work(&dev->request_module_wk);
> >>>>>    }
> >>>>>    
> >>>>> -/*
> >>>>> - * em28xx_release_resources()
> >>>>> - * unregisters the v4l2,i2c and usb devices
> >>>>> - * called when the device gets disconnected or at module unload
> >>>>> -*/
> >>>>> -void em28xx_release_resources(struct em28xx *dev)
> >>>>> +/**
> >>>>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
> >>>>> + *
> >>>>> + * @ref: struct kref for em28xx device
> >>>>> + *
> >>>>> + * This is called when all extensions and em28xx core unregisters a device
> >>>>> + */
> >>>>> +void em28xx_free_device(struct kref *ref)
> >>>>>    {
> >>>>> -	/*FIXME: I2C IR should be disconnected */
> >>>>> +	struct em28xx *dev = kref_to_dev(ref);
> >>>>>    
> >>>>> -	mutex_lock(&dev->lock);
> >>>>> +	em28xx_info("Freeing device\n");
> >>>>>    
> >>>>>    	if (dev->def_i2c_bus)
> >>>>>    		em28xx_i2c_unregister(dev, 1);
> >>>>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
> >>>>>    	/* Mark device as unused */
> >>>>>    	clear_bit(dev->devno, &em28xx_devused);
> >>>>>    
> >>>>> -	mutex_unlock(&dev->lock);
> >>>>> -};
> >>>>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
> >>>>> +	kfree(dev->alt_max_pkt_size_isoc);
> >>>>> +	kfree(dev);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
> >>>>>    
> >>>>>    /*
> >>>>>     * em28xx_init_dev()
> >>>>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> >>>>>    			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
> >>>>>    	}
> >>>>>    
> >>>>> +	kref_init(&dev->ref);
> >>>>> +
> >>>>>    	request_modules(dev);
> >>>>>    
> >>>>>    	/* Should be the last thing to do, to avoid newer udev's to
> >>>>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
> >>>>>    
> >>>>>    	em28xx_close_extension(dev);
> >>>>>    
> >>>>> -	em28xx_release_resources(dev);
> >>>>> -
> >>>>> -	if (!dev->users) {
> >>>>> -		kfree(dev->alt_max_pkt_size_isoc);
> >>>>> -		kfree(dev);
> >>>>> -	}
> >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>>>    }
> >>>>>    
> >>>>>    static struct usb_driver em28xx_usb_driver = {
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>>> index 5ea563e3f0e4..8674ae5fce06 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >>>>>    	em28xx_info("Binding DVB extension\n");
> >>>>>    
> >>>>>    	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
> >>>>> -
> >>>>>    	if (dvb == NULL) {
> >>>>>    		em28xx_info("em28xx_dvb: memory allocation failed\n");
> >>>>>    		return -ENOMEM;
> >>>>>    	}
> >>>>> +	kref_get(&dev->ref);
> >>>>>    	dev->dvb = dvb;
> >>>>>    	dvb->fe[0] = dvb->fe[1] = NULL;
> >>>>>    
> >>>>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >>>>>    	dvb->adapter.mfe_shared = mfe_shared;
> >>>>>    
> >>>>>    	em28xx_info("DVB extension successfully initialized\n");
> >>>>> +
> >>>>>    ret:
> >>>>>    	em28xx_set_mode(dev, EM28XX_SUSPEND);
> >>>>>    	mutex_unlock(&dev->lock);
> >>>>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
> >>>>>    		dev->dvb = NULL;
> >>>>>    	}
> >>>>>    
> >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>>> +
> >>>>>    	return 0;
> >>>>>    }
> >>>>>    
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> >>>>> index 61c061f3a476..33388b5922a0 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> >>>>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
> >>>>>    		return 0;
> >>>>>    	}
> >>>>>    
> >>>>> +	kref_get(&dev->ref);
> >>>>> +
> >>>>>    	if (dev->board.buttons)
> >>>>>    		em28xx_init_buttons(dev);
> >>>>>    
> >>>>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >>>>>    
> >>>>>    	/* skip detach on non attached boards */
> >>>>>    	if (!ir)
> >>>>> -		return 0;
> >>>>> +		goto ref_put;
> >>>>>    
> >>>>>    	if (ir->rc)
> >>>>>    		rc_unregister_device(ir->rc);
> >>>>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
> >>>>>    	/* done */
> >>>>>    	kfree(ir);
> >>>>>    	dev->ir = NULL;
> >>>>> +
> >>>>> +ref_put:
> >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>>> +
> >>>>>    	return 0;
> >>>>>    }
> >>>>>    
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> >>>>> index 587ff3fe9402..dc10cec772ba 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> >>>>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> >>>>>    	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> >>>>>    	v4l2_device_unregister(&dev->v4l2_dev);
> >>>>>    
> >>>>> -	if (dev->users)
> >>>>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
> >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> >>>>>    
> >>>>>    	return 0;
> >>>>>    }
> >>>>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
> >>>>>    	mutex_lock(&dev->lock);
> >>>>>    
> >>>>>    	if (dev->users == 1) {
> >>>>> -		/* free the remaining resources if device is disconnected */
> >>>>> -		if (dev->disconnected) {
> >>>>> -			kfree(dev->alt_max_pkt_size_isoc);
> >>>>> +		/* No sense to try to write to the device */
> >>>>> +		if (dev->disconnected)
> >>>>>    			goto exit;
> >>>>> -		}
> >>>>>    
> >>>>>    		/* Save some power by putting tuner to sleep */
> >>>>>    		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> >>>>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> >>>>>    
> >>>>>    	em28xx_info("Registering V4L2 extension\n");
> >>>>>    
> >>>>> +	kref_get(&dev->ref);
> >>>>> +
> >>>>>    	mutex_lock(&dev->lock);
> >>>>>    
> >>>>>    	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> >>>>> index 5d5d1b6f0294..d38c08e4da60 100644
> >>>>> --- a/drivers/media/usb/em28xx/em28xx.h
> >>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
> >>>>> @@ -32,6 +32,7 @@
> >>>>>    #include <linux/workqueue.h>
> >>>>>    #include <linux/i2c.h>
> >>>>>    #include <linux/mutex.h>
> >>>>> +#include <linux/kref.h>
> >>>>>    #include <linux/videodev2.h>
> >>>>>    
> >>>>>    #include <media/videobuf2-vmalloc.h>
> >>>>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
> >>>>>    	enum em28xx_i2c_algo_type algo_type;
> >>>>>    };
> >>>>>    
> >>>>> -
> >>>>>    /* main device struct */
> >>>>>    struct em28xx {
> >>>>> +	struct kref ref;
> >>>>> +
> >>>>> +
> >>>>>    	/* generic device properties */
> >>>>>    	char name[30];		/* name (including minor) of the device */
> >>>>>    	int model;		/* index in the device_data struct */
> >>>>> @@ -706,6 +709,8 @@ struct em28xx {
> >>>>>    	struct em28xx_dvb *dvb;
> >>>>>    };
> >>>>>    
> >>>>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
> >>>>> +
> >>>>>    struct em28xx_ops {
> >>>>>    	struct list_head next;
> >>>>>    	char *name;
> >>>>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
> >>>>>    extern struct usb_device_id em28xx_id_table[];
> >>>>>    int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
> >>>>>    void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
> >>>>> -void em28xx_release_resources(struct em28xx *dev);
> >>>>> +void em28xx_free_device(struct kref *ref);
> >>>>>    
> >>>>>    /* Provided by em28xx-camera.c */
> >>>>>    int em28xx_detect_sensor(struct em28xx *dev);
> >>>> I welcome this patch and the general approach looks good.
> >>>> I had started working on the same issue, but it's not that trivial.
> >>>>
> >>>> At first glance there seem to be several issues, but I will need to
> >>>> review this patch in more detail and also make some tests.
> >>>> Unfortunately, I don't have much time this evening, So could you please
> >>>> hold it back another day ?
> >>>> I hope I can review the other remaining patch of this series (patch 5/7)
> >>>> later this evening.
> >>> We're running out of time for 3.14. I think we should merge this patch
> >>> series, and your patch series for 3.14, to be together with the em28xx-v4l
> >>> split.
> >>>
> >>> My plan is to merge the remaining patches for 3.14 today or, in the worse
> >>> case, tomorrow.
> >>>
> >>> If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
> >>> series gets merged, but, if we miss the bus, I'm afraid that we'll end
> >>> by having more problems that will be hard to fix with trivial patches, due
> >>> to em28xx-v4l split changes that also affected the driver removal and device
> >>> close code.
> >>>
> >>> FYI, I tested this code and also Antti with our devices, randomly removing
> >>> the devices while streaming, and this is now finally working.
> >>>
> >>> I won't doubt that there are some cases that require fixes (and
> >>> it seems that em28xx-rc has one of such corner cases), but they'll likely
> >>> can be solved with somewhat short and trivial patches.
> >>>
> >>> Cheers,
> >>> Mauro
> >> This is a very critical patch and exactly the kind of change that should
> >> _never_ be hurried !
> >> FAICS it has some severe issues and it's not clear how easy it will be
> >> to fix it within the the 3.14-rc cycle.
> >> As long as it's not ready, don't merge it for 3.14.
> > What issues? So far, you didn't point any.
> I already stated that I didn't have the time yet to review and test it 
> in detail, but I'm going to do that as soon as possible.
> If you can't wait, there's nothing I can do, sorry.
> 
> At first glance it seems there are at least 2 issues:
> 1.) use after freeing in v4l-extension (happens when the device is 
> closed after the usb disconnect)

That's basically what this patch fixes. Both V4L2 and ALSA handles it
well, if you warn the subsystem that a device will be removed.

If are there still any issues, we may add a kref_get() at device open,
an a kref_put() at device close on the affected sub-driver.

> 2.) error paths in the init() functions ?

It should be ok. Basically, kref_get() is called even if an error
occurs, and kref_put() is called even if init fails. Ok, this is an
overkill, but makes the patch simpler to review. We may fine tune it
on a latter patch.

I opted to call kref_get() as soon as possible, to avoid the risk of
the device to be removed while the extension is still being
initialized (yes, if you remove a device too quick, even before pulseaudio
starts opening the device), it used to crash.

> > On both my tests and Antti's one,
> > with this series, there were significant improvements on removing existing
> > bugs with device removal.
> I'm talking about this specific patch here, not the whole series.
> I have no objections against the rest of the series (well, with the 
> exception of patch 5 at the moment).

I see. Well, let me do some tests with all patches except 3 and 5 applied,
to see how it behaves.

> >
> >> em28xx resources releasing is broken since ... well... at least 2 years.
> >> 3.14 will already be much better and nobody will care if this remaining
> >> issue is addressed a kernel release later.
> > Although I think that we're properly releasing resources, I'm a way less
> > concerned about keeping some leaked memory while releasing them than I am
> > concerned that a device removal that would cause an OOPS, or a deadly
> > crash at the USB or ALSA stack that prevents other devices to be probed.
> >
> > Due to em28xx-v4l calling em28xx_release_resources(), now both the
> > USB and ALSA stacks crashes if you remove a device while ALSA is streaming.
> >
> > That happens because em28xx, I2C, etc will be freed by em28xx-v4l, but
> > those resources are still needed by em28xx-alsa. That makes that the
> > .fini code there to cause crash at ALSA stack, and, sometimes, at the
> > USB stack.
> 
> That's not true anymore, em28xx_release_resources() is now only called 
> by the usb disconnect handler in the core _after_ all fini() functions 
> have been called.
> Maybe you tested that without my patch series ? See patch 7/8.

No, we tested it with your full series applied:
	http://git.linuxtv.org/mchehab/experimental.git/shortlog/refs/heads/em28xx

> 
> > As nowadays lots of components depend on pulseaudio, the ALSA crash
> > causes pulse to stop work, likely keeping it into some dead lock status.
> > This makes the entire machine to become really slow, when it doesn't
> > crash.
> >
> > This is a serious regression that should be fixed.
> >
> > This patch series for sure fixes it. As I said, there are two independent
> > series of tests verifying that (both using several different em28xx
> > devices).
> >
> >> Be warned !
> >>
> >>
> >
>

Mauro Carvalho Chehab Jan. 14, 2014, 7:31 p.m. UTC | #7

Em Tue, 14 Jan 2014 16:55:12 -0200
Mauro Carvalho Chehab <[e-mail]> escreveu:

> Em Tue, 14 Jan 2014 19:13:00 +0100
> Frank Schäfer <[e-mail]> escreveu:
> 
> > On 14.01.2014 14:10, Mauro Carvalho Chehab wrote:
> > > Em Mon, 13 Jan 2014 22:55:36 +0100
> > > Frank Schäfer <[e-mail]> escreveu:
> > >
> > >> Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
> > >>> Em Mon, 13 Jan 2014 20:02:19 +0100
> > >>> Frank Schäfer <[e-mail]> escreveu:
> > >>>
> > >>>> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
> > >>>>> We can't free struct em28xx while one of the extensions is still
> > >>>>> using it.
> > >>>>>
> > >>>>> So, add a kref() to control it, freeing it only after the
> > >>>>> extensions fini calls.
> > >>>>>
> > >>>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > >>>>> ---
> > >>>>>    drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
> > >>>>>    drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
> > >>>>>    drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
> > >>>>>    drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
> > >>>>>    drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
> > >>>>>    drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
> > >>>>>    6 files changed, 44 insertions(+), 28 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> > >>>>> index 97d9105e6830..8e959dae8358 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> > >>>>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
> > >>>>>    
> > >>>>>    	em28xx_info("Binding audio extension\n");
> > >>>>>    
> > >>>>> +	kref_get(&dev->ref);
> > >>>>> +
> > >>>>>    	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
> > >>>>>    			 "Rechberger\n");
> > >>>>>    	printk(KERN_INFO
> > >>>>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> > >>>>>    	if (dev == NULL)
> > >>>>>    		return 0;
> > >>>>>    
> > >>>>> -	if (dev->has_alsa_audio != 1) {
> > >>>>> +	if (!dev->has_alsa_audio) {
> > >>>>>    		/* This device does not support the extension (in this case
> > >>>>>    		   the device is expecting the snd-usb-audio module or
> > >>>>>    		   doesn't have analog audio support at all) */
> > >>>>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
> > >>>>>    		dev->adev.sndcard = NULL;
> > >>>>>    	}
> > >>>>>    
> > >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> > >>>>>    	return 0;
> > >>>>>    }
> > >>>>>    
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> > >>>>> index 3b332d527ccb..df92f417634a 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> > >>>>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
> > >>>>>    	flush_work(&dev->request_module_wk);
> > >>>>>    }
> > >>>>>    
> > >>>>> -/*
> > >>>>> - * em28xx_release_resources()
> > >>>>> - * unregisters the v4l2,i2c and usb devices
> > >>>>> - * called when the device gets disconnected or at module unload
> > >>>>> -*/
> > >>>>> -void em28xx_release_resources(struct em28xx *dev)
> > >>>>> +/**
> > >>>>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
> > >>>>> + *
> > >>>>> + * @ref: struct kref for em28xx device
> > >>>>> + *
> > >>>>> + * This is called when all extensions and em28xx core unregisters a device
> > >>>>> + */
> > >>>>> +void em28xx_free_device(struct kref *ref)
> > >>>>>    {
> > >>>>> -	/*FIXME: I2C IR should be disconnected */
> > >>>>> +	struct em28xx *dev = kref_to_dev(ref);
> > >>>>>    
> > >>>>> -	mutex_lock(&dev->lock);
> > >>>>> +	em28xx_info("Freeing device\n");
> > >>>>>    
> > >>>>>    	if (dev->def_i2c_bus)
> > >>>>>    		em28xx_i2c_unregister(dev, 1);
> > >>>>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
> > >>>>>    	/* Mark device as unused */
> > >>>>>    	clear_bit(dev->devno, &em28xx_devused);
> > >>>>>    
> > >>>>> -	mutex_unlock(&dev->lock);
> > >>>>> -};
> > >>>>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
> > >>>>> +	kfree(dev->alt_max_pkt_size_isoc);
> > >>>>> +	kfree(dev);
> > >>>>> +}
> > >>>>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
> > >>>>>    
> > >>>>>    /*
> > >>>>>     * em28xx_init_dev()
> > >>>>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> > >>>>>    			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
> > >>>>>    	}
> > >>>>>    
> > >>>>> +	kref_init(&dev->ref);
> > >>>>> +
> > >>>>>    	request_modules(dev);
> > >>>>>    
> > >>>>>    	/* Should be the last thing to do, to avoid newer udev's to
> > >>>>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
> > >>>>>    
> > >>>>>    	em28xx_close_extension(dev);
> > >>>>>    
> > >>>>> -	em28xx_release_resources(dev);
> > >>>>> -
> > >>>>> -	if (!dev->users) {
> > >>>>> -		kfree(dev->alt_max_pkt_size_isoc);
> > >>>>> -		kfree(dev);
> > >>>>> -	}
> > >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> > >>>>>    }
> > >>>>>    
> > >>>>>    static struct usb_driver em28xx_usb_driver = {
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> > >>>>> index 5ea563e3f0e4..8674ae5fce06 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> > >>>>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
> > >>>>>    	em28xx_info("Binding DVB extension\n");
> > >>>>>    
> > >>>>>    	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
> > >>>>> -
> > >>>>>    	if (dvb == NULL) {
> > >>>>>    		em28xx_info("em28xx_dvb: memory allocation failed\n");
> > >>>>>    		return -ENOMEM;
> > >>>>>    	}
> > >>>>> +	kref_get(&dev->ref);
> > >>>>>    	dev->dvb = dvb;
> > >>>>>    	dvb->fe[0] = dvb->fe[1] = NULL;
> > >>>>>    
> > >>>>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
> > >>>>>    	dvb->adapter.mfe_shared = mfe_shared;
> > >>>>>    
> > >>>>>    	em28xx_info("DVB extension successfully initialized\n");
> > >>>>> +
> > >>>>>    ret:
> > >>>>>    	em28xx_set_mode(dev, EM28XX_SUSPEND);
> > >>>>>    	mutex_unlock(&dev->lock);
> > >>>>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
> > >>>>>    		dev->dvb = NULL;
> > >>>>>    	}
> > >>>>>    
> > >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> > >>>>> +
> > >>>>>    	return 0;
> > >>>>>    }
> > >>>>>    
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> > >>>>> index 61c061f3a476..33388b5922a0 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> > >>>>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
> > >>>>>    		return 0;
> > >>>>>    	}
> > >>>>>    
> > >>>>> +	kref_get(&dev->ref);
> > >>>>> +
> > >>>>>    	if (dev->board.buttons)
> > >>>>>    		em28xx_init_buttons(dev);
> > >>>>>    
> > >>>>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
> > >>>>>    
> > >>>>>    	/* skip detach on non attached boards */
> > >>>>>    	if (!ir)
> > >>>>> -		return 0;
> > >>>>> +		goto ref_put;
> > >>>>>    
> > >>>>>    	if (ir->rc)
> > >>>>>    		rc_unregister_device(ir->rc);
> > >>>>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
> > >>>>>    	/* done */
> > >>>>>    	kfree(ir);
> > >>>>>    	dev->ir = NULL;
> > >>>>> +
> > >>>>> +ref_put:
> > >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> > >>>>> +
> > >>>>>    	return 0;
> > >>>>>    }
> > >>>>>    
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > >>>>> index 587ff3fe9402..dc10cec772ba 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > >>>>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> > >>>>>    	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> > >>>>>    	v4l2_device_unregister(&dev->v4l2_dev);
> > >>>>>    
> > >>>>> -	if (dev->users)
> > >>>>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
> > >>>>> +	kref_put(&dev->ref, em28xx_free_device);
> > >>>>>    
> > >>>>>    	return 0;
> > >>>>>    }
> > >>>>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
> > >>>>>    	mutex_lock(&dev->lock);
> > >>>>>    
> > >>>>>    	if (dev->users == 1) {
> > >>>>> -		/* free the remaining resources if device is disconnected */
> > >>>>> -		if (dev->disconnected) {
> > >>>>> -			kfree(dev->alt_max_pkt_size_isoc);
> > >>>>> +		/* No sense to try to write to the device */
> > >>>>> +		if (dev->disconnected)
> > >>>>>    			goto exit;
> > >>>>> -		}
> > >>>>>    
> > >>>>>    		/* Save some power by putting tuner to sleep */
> > >>>>>    		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> > >>>>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > >>>>>    
> > >>>>>    	em28xx_info("Registering V4L2 extension\n");
> > >>>>>    
> > >>>>> +	kref_get(&dev->ref);
> > >>>>> +
> > >>>>>    	mutex_lock(&dev->lock);
> > >>>>>    
> > >>>>>    	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> > >>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> > >>>>> index 5d5d1b6f0294..d38c08e4da60 100644
> > >>>>> --- a/drivers/media/usb/em28xx/em28xx.h
> > >>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
> > >>>>> @@ -32,6 +32,7 @@
> > >>>>>    #include <linux/workqueue.h>
> > >>>>>    #include <linux/i2c.h>
> > >>>>>    #include <linux/mutex.h>
> > >>>>> +#include <linux/kref.h>
> > >>>>>    #include <linux/videodev2.h>
> > >>>>>    
> > >>>>>    #include <media/videobuf2-vmalloc.h>
> > >>>>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
> > >>>>>    	enum em28xx_i2c_algo_type algo_type;
> > >>>>>    };
> > >>>>>    
> > >>>>> -
> > >>>>>    /* main device struct */
> > >>>>>    struct em28xx {
> > >>>>> +	struct kref ref;
> > >>>>> +
> > >>>>> +
> > >>>>>    	/* generic device properties */
> > >>>>>    	char name[30];		/* name (including minor) of the device */
> > >>>>>    	int model;		/* index in the device_data struct */
> > >>>>> @@ -706,6 +709,8 @@ struct em28xx {
> > >>>>>    	struct em28xx_dvb *dvb;
> > >>>>>    };
> > >>>>>    
> > >>>>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
> > >>>>> +
> > >>>>>    struct em28xx_ops {
> > >>>>>    	struct list_head next;
> > >>>>>    	char *name;
> > >>>>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
> > >>>>>    extern struct usb_device_id em28xx_id_table[];
> > >>>>>    int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
> > >>>>>    void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
> > >>>>> -void em28xx_release_resources(struct em28xx *dev);
> > >>>>> +void em28xx_free_device(struct kref *ref);
> > >>>>>    
> > >>>>>    /* Provided by em28xx-camera.c */
> > >>>>>    int em28xx_detect_sensor(struct em28xx *dev);
> > >>>> I welcome this patch and the general approach looks good.
> > >>>> I had started working on the same issue, but it's not that trivial.
> > >>>>
> > >>>> At first glance there seem to be several issues, but I will need to
> > >>>> review this patch in more detail and also make some tests.
> > >>>> Unfortunately, I don't have much time this evening, So could you please
> > >>>> hold it back another day ?
> > >>>> I hope I can review the other remaining patch of this series (patch 5/7)
> > >>>> later this evening.
> > >>> We're running out of time for 3.14. I think we should merge this patch
> > >>> series, and your patch series for 3.14, to be together with the em28xx-v4l
> > >>> split.
> > >>>
> > >>> My plan is to merge the remaining patches for 3.14 today or, in the worse
> > >>> case, tomorrow.
> > >>>
> > >>> If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
> > >>> series gets merged, but, if we miss the bus, I'm afraid that we'll end
> > >>> by having more problems that will be hard to fix with trivial patches, due
> > >>> to em28xx-v4l split changes that also affected the driver removal and device
> > >>> close code.
> > >>>
> > >>> FYI, I tested this code and also Antti with our devices, randomly removing
> > >>> the devices while streaming, and this is now finally working.
> > >>>
> > >>> I won't doubt that there are some cases that require fixes (and
> > >>> it seems that em28xx-rc has one of such corner cases), but they'll likely
> > >>> can be solved with somewhat short and trivial patches.
> > >>>
> > >>> Cheers,
> > >>> Mauro
> > >> This is a very critical patch and exactly the kind of change that should
> > >> _never_ be hurried !
> > >> FAICS it has some severe issues and it's not clear how easy it will be
> > >> to fix it within the the 3.14-rc cycle.
> > >> As long as it's not ready, don't merge it for 3.14.
> > > What issues? So far, you didn't point any.
> > I already stated that I didn't have the time yet to review and test it 
> > in detail, but I'm going to do that as soon as possible.
> > If you can't wait, there's nothing I can do, sorry.
> > 
> > At first glance it seems there are at least 2 issues:
> > 1.) use after freeing in v4l-extension (happens when the device is 
> > closed after the usb disconnect)
> 
> That's basically what this patch fixes. Both V4L2 and ALSA handles it
> well, if you warn the subsystem that a device will be removed.
> 
> If are there still any issues, we may add a kref_get() at device open,
> an a kref_put() at device close on the affected sub-driver.
> 
> > 2.) error paths in the init() functions ?
> 
> It should be ok. Basically, kref_get() is called even if an error
> occurs, and kref_put() is called even if init fails. Ok, this is an
> overkill, but makes the patch simpler to review. We may fine tune it
> on a latter patch.
> 
> I opted to call kref_get() as soon as possible, to avoid the risk of
> the device to be removed while the extension is still being
> initialized (yes, if you remove a device too quick, even before pulseaudio
> starts opening the device), it used to crash.
> 
> > > On both my tests and Antti's one,
> > > with this series, there were significant improvements on removing existing
> > > bugs with device removal.
> > I'm talking about this specific patch here, not the whole series.
> > I have no objections against the rest of the series (well, with the 
> > exception of patch 5 at the moment).
> 
> I see. Well, let me do some tests with all patches except 3 and 5 applied,
> to see how it behaves.

Ok, patch 5 is not needed anymore.

However, after a series of removals and re-inserts, I got these:


[120982.699455] ------------[ cut here ]------------
[120982.699509] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
[120982.699539] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was 0000000d00000007. (prev=ffff880101dbfb48).
[120982.699566] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
[120982.701828]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
[120982.703581] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D      3.13.0-rc1+ #24
[120982.703610] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
[120982.703638]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
[120982.703794]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
[120982.703924]  ffff880101dbfb48 0000000000000282 0000000000000000 ffff88011f63bc08
[120982.704056] Call Trace:
[120982.704093]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
[120982.704129]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
[120982.704162]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
[120982.704197]  [<ffffffff81343bac>] __list_add+0xac/0xc0
[120982.704233]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
[120982.704432]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
[120982.704469]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
[120982.704505]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
[120982.704543]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
[120982.704581]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
[120982.704619]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120982.704658]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120982.704719]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
[120982.704756]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
[120982.704792]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120982.704829]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120982.704867]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
[120982.704903]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
[120982.704939]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
[120982.704974]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
[120982.705006]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
[120982.705039]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
[120982.705070] ---[ end trace 7e24c4fe20a76f18 ]---
[120985.899453] ------------[ cut here ]------------
[120985.899502] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
[120985.899531] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was           (null). (prev=ffff88018cfb0348).
[120985.899559] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
[120985.901761]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
[120985.903502] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
[120985.903530] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
[120985.903557]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
[120985.903686]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb0348 ffff88019ffce4f8
[120985.903826]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
[120985.903955] Call Trace:
[120985.903988]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
[120985.904021]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
[120985.904052]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
[120985.904083]  [<ffffffff81343bac>] __list_add+0xac/0xc0
[120985.904116]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
[120985.904295]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
[120985.904330]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
[120985.904363]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
[120985.904398]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
[120985.904433]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
[120985.904470]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.904504]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.904540]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
[120985.904575]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
[120985.904608]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.904641]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.904676]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
[120985.904710]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
[120985.904743]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
[120985.904780]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
[120985.904810]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
[120985.904843]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
[120985.904871] ---[ end trace 7e24c4fe20a76f19 ]---
[120985.904896] ------------[ cut here ]------------
[120985.904922] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:36 __list_add+0x8a/0xc0()
[120985.904953] list_add double add: new=ffff88018cfb0348, prev=ffff88018cfb0348, next=ffff88019ffce4f8.
[120985.904978] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
[120985.907016]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
[120985.908654] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
[120985.908680] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
[120985.908707]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
[120985.908988]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb0348 ffff88019ffce4f8
[120985.909110]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
[120985.909230] Call Trace:
[120985.909268]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
[120985.909305]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
[120985.909336]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
[120985.909380]  [<ffffffff81343b8a>] __list_add+0x8a/0xc0
[120985.909410]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
[120985.909442]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
[120985.909474]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
[120985.909505]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
[120985.909537]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
[120985.909570]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
[120985.909604]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.909636]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.909670]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
[120985.909702]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
[120985.909732]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.909764]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.909799]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
[120985.909830]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
[120985.909860]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
[120985.909890]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
[120985.909918]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
[120985.909948]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
[120985.909974] ---[ end trace 7e24c4fe20a76f1a ]---
[120985.910186] ------------[ cut here ]------------
[120985.910213] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
[120985.910236] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was ffff88018cfb0348. (prev=ffff88018cfb0348).
[120985.910257] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
[120985.912159]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
[120985.913642] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
[120985.913667] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
[120985.913691]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
[120985.913805]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
[120985.913920]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
[120985.914035] Call Trace:
[120985.914063]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
[120985.914093]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
[120985.914121]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
[120985.914150]  [<ffffffff81343bac>] __list_add+0xac/0xc0
[120985.914180]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
[120985.914353]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
[120985.914385]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
[120985.914416]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
[120985.914449]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
[120985.914482]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
[120985.914515]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.914548]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120985.914580]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
[120985.914612]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
[120985.914643]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.914675]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120985.914708]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
[120985.914740]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
[120985.914770]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
[120985.914799]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
[120985.914828]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
[120985.914857]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
[120985.914884] ---[ end trace 7e24c4fe20a76f1b ]---
[120991.576999] em2882/3 #0: submit of audio urb failed
[120991.781651] ------------[ cut here ]------------
[120991.781700] WARNING: CPU: 0 PID: 7979 at lib/list_debug.c:33 __list_add+0xac/0xc0()
[120991.781730] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was           (null). (prev=ffff88018cfb1b48).
[120991.781756] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
[120991.783977]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
[120991.785750] CPU: 0 PID: 7979 Comm: tvtime Tainted: G      D W    3.13.0-rc1+ #24
[120991.785779] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
[120991.785806]  0000000000000009 ffff88001254fb70 ffffffff816a03c6 ffff88001254fbb8
[120991.785938]  ffff88001254fba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
[120991.786102]  ffff88018cfb1b48 0000000000000282 0000000000000000 ffff88001254fc08
[120991.786233] Call Trace:
[120991.786268]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
[120991.786319]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
[120991.786359]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
[120991.786393]  [<ffffffff81343bac>] __list_add+0xac/0xc0
[120991.786429]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
[120991.786619]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
[120991.786656]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
[120991.786690]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
[120991.786728]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
[120991.786767]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
[120991.786804]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120991.786841]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
[120991.786880]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
[120991.786917]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
[120991.786952]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120991.786988]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
[120991.787027]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
[120991.787063]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
[120991.787098]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
[120991.787133]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
[120991.787165]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
[120991.787199]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
[120991.787230] ---[ end trace 7e24c4fe20a76f1c ]---

Frank Schäfer Jan. 14, 2014, 8:48 p.m. UTC | #8

Am 14.01.2014 19:55, schrieb Mauro Carvalho Chehab:
> Em Tue, 14 Jan 2014 19:13:00 +0100
> Frank Schäfer <[e-mail]> escreveu:
>
>> On 14.01.2014 14:10, Mauro Carvalho Chehab wrote:
>>> Em Mon, 13 Jan 2014 22:55:36 +0100
>>> Frank Schäfer <[e-mail]> escreveu:
>>>
>>>> Am 13.01.2014 20:23, schrieb Mauro Carvalho Chehab:
>>>>> Em Mon, 13 Jan 2014 20:02:19 +0100
>>>>> Frank Schäfer <[e-mail]> escreveu:
>>>>>
>>>>>> On 13.01.2014 00:00, Mauro Carvalho Chehab wrote:
>>>>>>> We can't free struct em28xx while one of the extensions is still
>>>>>>> using it.
>>>>>>>
>>>>>>> So, add a kref() to control it, freeing it only after the
>>>>>>> extensions fini calls.
>>>>>>>
>>>>>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>>>>>>> ---
>>>>>>>    drivers/media/usb/em28xx/em28xx-audio.c |  5 ++++-
>>>>>>>    drivers/media/usb/em28xx/em28xx-cards.c | 34 ++++++++++++++++-----------------
>>>>>>>    drivers/media/usb/em28xx/em28xx-dvb.c   |  5 ++++-
>>>>>>>    drivers/media/usb/em28xx/em28xx-input.c |  8 +++++++-
>>>>>>>    drivers/media/usb/em28xx/em28xx-video.c | 11 +++++------
>>>>>>>    drivers/media/usb/em28xx/em28xx.h       |  9 +++++++--
>>>>>>>    6 files changed, 44 insertions(+), 28 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>>> index 97d9105e6830..8e959dae8358 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>>> @@ -878,6 +878,8 @@ static int em28xx_audio_init(struct em28xx *dev)
>>>>>>>    
>>>>>>>    	em28xx_info("Binding audio extension\n");
>>>>>>>    
>>>>>>> +	kref_get(&dev->ref);
>>>>>>> +
>>>>>>>    	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
>>>>>>>    			 "Rechberger\n");
>>>>>>>    	printk(KERN_INFO
>>>>>>> @@ -949,7 +951,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>>>>>    	if (dev == NULL)
>>>>>>>    		return 0;
>>>>>>>    
>>>>>>> -	if (dev->has_alsa_audio != 1) {
>>>>>>> +	if (!dev->has_alsa_audio) {
>>>>>>>    		/* This device does not support the extension (in this case
>>>>>>>    		   the device is expecting the snd-usb-audio module or
>>>>>>>    		   doesn't have analog audio support at all) */
>>>>>>> @@ -963,6 +965,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>>>>>    		dev->adev.sndcard = NULL;
>>>>>>>    	}
>>>>>>>    
>>>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>>>    	return 0;
>>>>>>>    }
>>>>>>>    
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>>> index 3b332d527ccb..df92f417634a 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>>> @@ -2867,16 +2867,18 @@ static void flush_request_modules(struct em28xx *dev)
>>>>>>>    	flush_work(&dev->request_module_wk);
>>>>>>>    }
>>>>>>>    
>>>>>>> -/*
>>>>>>> - * em28xx_release_resources()
>>>>>>> - * unregisters the v4l2,i2c and usb devices
>>>>>>> - * called when the device gets disconnected or at module unload
>>>>>>> -*/
>>>>>>> -void em28xx_release_resources(struct em28xx *dev)
>>>>>>> +/**
>>>>>>> + * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
>>>>>>> + *
>>>>>>> + * @ref: struct kref for em28xx device
>>>>>>> + *
>>>>>>> + * This is called when all extensions and em28xx core unregisters a device
>>>>>>> + */
>>>>>>> +void em28xx_free_device(struct kref *ref)
>>>>>>>    {
>>>>>>> -	/*FIXME: I2C IR should be disconnected */
>>>>>>> +	struct em28xx *dev = kref_to_dev(ref);
>>>>>>>    
>>>>>>> -	mutex_lock(&dev->lock);
>>>>>>> +	em28xx_info("Freeing device\n");
>>>>>>>    
>>>>>>>    	if (dev->def_i2c_bus)
>>>>>>>    		em28xx_i2c_unregister(dev, 1);
>>>>>>> @@ -2887,9 +2889,10 @@ void em28xx_release_resources(struct em28xx *dev)
>>>>>>>    	/* Mark device as unused */
>>>>>>>    	clear_bit(dev->devno, &em28xx_devused);
>>>>>>>    
>>>>>>> -	mutex_unlock(&dev->lock);
>>>>>>> -};
>>>>>>> -EXPORT_SYMBOL_GPL(em28xx_release_resources);
>>>>>>> +	kfree(dev->alt_max_pkt_size_isoc);
>>>>>>> +	kfree(dev);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(em28xx_free_device);
>>>>>>>    
>>>>>>>    /*
>>>>>>>     * em28xx_init_dev()
>>>>>>> @@ -3342,6 +3345,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>>>>>>    			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
>>>>>>>    	}
>>>>>>>    
>>>>>>> +	kref_init(&dev->ref);
>>>>>>> +
>>>>>>>    	request_modules(dev);
>>>>>>>    
>>>>>>>    	/* Should be the last thing to do, to avoid newer udev's to
>>>>>>> @@ -3390,12 +3395,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>>>>>>>    
>>>>>>>    	em28xx_close_extension(dev);
>>>>>>>    
>>>>>>> -	em28xx_release_resources(dev);
>>>>>>> -
>>>>>>> -	if (!dev->users) {
>>>>>>> -		kfree(dev->alt_max_pkt_size_isoc);
>>>>>>> -		kfree(dev);
>>>>>>> -	}
>>>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>>>    }
>>>>>>>    
>>>>>>>    static struct usb_driver em28xx_usb_driver = {
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>>> index 5ea563e3f0e4..8674ae5fce06 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>>> @@ -1010,11 +1010,11 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>>>>>    	em28xx_info("Binding DVB extension\n");
>>>>>>>    
>>>>>>>    	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
>>>>>>> -
>>>>>>>    	if (dvb == NULL) {
>>>>>>>    		em28xx_info("em28xx_dvb: memory allocation failed\n");
>>>>>>>    		return -ENOMEM;
>>>>>>>    	}
>>>>>>> +	kref_get(&dev->ref);
>>>>>>>    	dev->dvb = dvb;
>>>>>>>    	dvb->fe[0] = dvb->fe[1] = NULL;
>>>>>>>    
>>>>>>> @@ -1442,6 +1442,7 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>>>>>>    	dvb->adapter.mfe_shared = mfe_shared;
>>>>>>>    
>>>>>>>    	em28xx_info("DVB extension successfully initialized\n");
>>>>>>> +
>>>>>>>    ret:
>>>>>>>    	em28xx_set_mode(dev, EM28XX_SUSPEND);
>>>>>>>    	mutex_unlock(&dev->lock);
>>>>>>> @@ -1492,6 +1493,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
>>>>>>>    		dev->dvb = NULL;
>>>>>>>    	}
>>>>>>>    
>>>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>>> +
>>>>>>>    	return 0;
>>>>>>>    }
>>>>>>>    
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
>>>>>>> index 61c061f3a476..33388b5922a0 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>>>>>>> @@ -676,6 +676,8 @@ static int em28xx_ir_init(struct em28xx *dev)
>>>>>>>    		return 0;
>>>>>>>    	}
>>>>>>>    
>>>>>>> +	kref_get(&dev->ref);
>>>>>>> +
>>>>>>>    	if (dev->board.buttons)
>>>>>>>    		em28xx_init_buttons(dev);
>>>>>>>    
>>>>>>> @@ -814,7 +816,7 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>>>>>    
>>>>>>>    	/* skip detach on non attached boards */
>>>>>>>    	if (!ir)
>>>>>>> -		return 0;
>>>>>>> +		goto ref_put;
>>>>>>>    
>>>>>>>    	if (ir->rc)
>>>>>>>    		rc_unregister_device(ir->rc);
>>>>>>> @@ -822,6 +824,10 @@ static int em28xx_ir_fini(struct em28xx *dev)
>>>>>>>    	/* done */
>>>>>>>    	kfree(ir);
>>>>>>>    	dev->ir = NULL;
>>>>>>> +
>>>>>>> +ref_put:
>>>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>>> +
>>>>>>>    	return 0;
>>>>>>>    }
>>>>>>>    
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>>>>>> index 587ff3fe9402..dc10cec772ba 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>>>>>> @@ -1922,8 +1922,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>>>>>>>    	v4l2_ctrl_handler_free(&dev->ctrl_handler);
>>>>>>>    	v4l2_device_unregister(&dev->v4l2_dev);
>>>>>>>    
>>>>>>> -	if (dev->users)
>>>>>>> -		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
>>>>>>> +	kref_put(&dev->ref, em28xx_free_device);
>>>>>>>    
>>>>>>>    	return 0;
>>>>>>>    }
>>>>>>> @@ -1945,11 +1944,9 @@ static int em28xx_v4l2_close(struct file *filp)
>>>>>>>    	mutex_lock(&dev->lock);
>>>>>>>    
>>>>>>>    	if (dev->users == 1) {
>>>>>>> -		/* free the remaining resources if device is disconnected */
>>>>>>> -		if (dev->disconnected) {
>>>>>>> -			kfree(dev->alt_max_pkt_size_isoc);
>>>>>>> +		/* No sense to try to write to the device */
>>>>>>> +		if (dev->disconnected)
>>>>>>>    			goto exit;
>>>>>>> -		}
>>>>>>>    
>>>>>>>    		/* Save some power by putting tuner to sleep */
>>>>>>>    		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>>>>>>> @@ -2201,6 +2198,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>>>>>>    
>>>>>>>    	em28xx_info("Registering V4L2 extension\n");
>>>>>>>    
>>>>>>> +	kref_get(&dev->ref);
>>>>>>> +
>>>>>>>    	mutex_lock(&dev->lock);
>>>>>>>    
>>>>>>>    	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
>>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>>>>>> index 5d5d1b6f0294..d38c08e4da60 100644
>>>>>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>>>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>    #include <linux/workqueue.h>
>>>>>>>    #include <linux/i2c.h>
>>>>>>>    #include <linux/mutex.h>
>>>>>>> +#include <linux/kref.h>
>>>>>>>    #include <linux/videodev2.h>
>>>>>>>    
>>>>>>>    #include <media/videobuf2-vmalloc.h>
>>>>>>> @@ -531,9 +532,11 @@ struct em28xx_i2c_bus {
>>>>>>>    	enum em28xx_i2c_algo_type algo_type;
>>>>>>>    };
>>>>>>>    
>>>>>>> -
>>>>>>>    /* main device struct */
>>>>>>>    struct em28xx {
>>>>>>> +	struct kref ref;
>>>>>>> +
>>>>>>> +
>>>>>>>    	/* generic device properties */
>>>>>>>    	char name[30];		/* name (including minor) of the device */
>>>>>>>    	int model;		/* index in the device_data struct */
>>>>>>> @@ -706,6 +709,8 @@ struct em28xx {
>>>>>>>    	struct em28xx_dvb *dvb;
>>>>>>>    };
>>>>>>>    
>>>>>>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
>>>>>>> +
>>>>>>>    struct em28xx_ops {
>>>>>>>    	struct list_head next;
>>>>>>>    	char *name;
>>>>>>> @@ -763,7 +768,7 @@ extern struct em28xx_board em28xx_boards[];
>>>>>>>    extern struct usb_device_id em28xx_id_table[];
>>>>>>>    int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
>>>>>>>    void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
>>>>>>> -void em28xx_release_resources(struct em28xx *dev);
>>>>>>> +void em28xx_free_device(struct kref *ref);
>>>>>>>    
>>>>>>>    /* Provided by em28xx-camera.c */
>>>>>>>    int em28xx_detect_sensor(struct em28xx *dev);
>>>>>> I welcome this patch and the general approach looks good.
>>>>>> I had started working on the same issue, but it's not that trivial.
>>>>>>
>>>>>> At first glance there seem to be several issues, but I will need to
>>>>>> review this patch in more detail and also make some tests.
>>>>>> Unfortunately, I don't have much time this evening, So could you please
>>>>>> hold it back another day ?
>>>>>> I hope I can review the other remaining patch of this series (patch 5/7)
>>>>>> later this evening.
>>>>> We're running out of time for 3.14. I think we should merge this patch
>>>>> series, and your patch series for 3.14, to be together with the em28xx-v4l
>>>>> split.
>>>>>
>>>>> My plan is to merge the remaining patches for 3.14 today or, in the worse
>>>>> case, tomorrow.
>>>>>
>>>>> If we slip on some bug, we'll still have the 3.14-rc cycle to fix, if the
>>>>> series gets merged, but, if we miss the bus, I'm afraid that we'll end
>>>>> by having more problems that will be hard to fix with trivial patches, due
>>>>> to em28xx-v4l split changes that also affected the driver removal and device
>>>>> close code.
>>>>>
>>>>> FYI, I tested this code and also Antti with our devices, randomly removing
>>>>> the devices while streaming, and this is now finally working.
>>>>>
>>>>> I won't doubt that there are some cases that require fixes (and
>>>>> it seems that em28xx-rc has one of such corner cases), but they'll likely
>>>>> can be solved with somewhat short and trivial patches.
>>>>>
>>>>> Cheers,
>>>>> Mauro
>>>> This is a very critical patch and exactly the kind of change that should
>>>> _never_ be hurried !
>>>> FAICS it has some severe issues and it's not clear how easy it will be
>>>> to fix it within the the 3.14-rc cycle.
>>>> As long as it's not ready, don't merge it for 3.14.
>>> What issues? So far, you didn't point any.
>> I already stated that I didn't have the time yet to review and test it 
>> in detail, but I'm going to do that as soon as possible.
>> If you can't wait, there's nothing I can do, sorry.
>>
>> At first glance it seems there are at least 2 issues:
>> 1.) use after freeing in v4l-extension (happens when the device is 
>> closed after the usb disconnect)
> That's basically what this patch fixes. Both V4L2 and ALSA handles it
> well, if you warn the subsystem that a device will be removed.
>
> If are there still any issues, we may add a kref_get() at device open,
> an a kref_put() at device close on the affected sub-driver.
Ok, I've tested it and I was right here.
This is what happens when closing a disconnected device:

[  144.045691] usb 1-8: USB disconnect, device number 7
[  144.047387] em2765 #0: disconnecting em2765 #0 video
[  144.047403] em2765 #0: V4L2 device video1 deregistered
[  144.050197] em2765 #0: Deregistering snapshot button
[  144.058336] em2765 #0: Freeing device
[  147.525267] : em28xx_v4l2_close() called
[  147.525287] : em28xx_videodevice_release() called


I will make some tests tomorrow, but here is a first suggestion how to
fix this:

Remove the kref_put() call from em28xx_v4l2_fini().
Instead, add the following lines to em28xx_videodevice_release():

if (dev->users == 0) {
        dev->users--; /* avoid multiple kref_put() calls when the
devices are unregistered and no device is open */
        kref_put(&dev->ref, em28xx_free_device);
}

That should fix it.

Interestingly no oops happens. I will have to take a closer look at this
tomorrow, but I suspect that the dev we obtain from struct file filp is
an outdated copy of the original device struct.
If that would be true and no bad things can happen in the close()
function we actually wouldn't need kref for the v4l extension at all.

Of course, the ideal solution would be, if we could just clear the
device struct at the end of the usb disconnect handler, because we can
be sure that the fini() functions have already made sure that dev isn't
used anymore.

Btw, what happens in em28xx-audio ?
Does the ALSA core also allow to call the close() function when the
device is already gone ?


>
>> 2.) error paths in the init() functions ?
> It should be ok. Basically, kref_get() is called even if an error
> occurs, and kref_put() is called even if init fails. Ok, this is an
> overkill, but makes the patch simpler to review. We may fine tune it
> on a latter patch.
>
> I opted to call kref_get() as soon as possible, to avoid the risk of
> the device to be removed while the extension is still being
> initialized (yes, if you remove a device too quick, even before pulseaudio
> starts opening the device), it used to crash.
I didn't have neough time to look at this part today.

>
>>> On both my tests and Antti's one,
>>> with this series, there were significant improvements on removing existing
>>> bugs with device removal.
>> I'm talking about this specific patch here, not the whole series.
>> I have no objections against the rest of the series (well, with the 
>> exception of patch 5 at the moment).
> I see. Well, let me do some tests with all patches except 3 and 5 applied,
> to see how it behaves.
>
>>>> em28xx resources releasing is broken since ... well... at least 2 years.
>>>> 3.14 will already be much better and nobody will care if this remaining
>>>> issue is addressed a kernel release later.
>>> Although I think that we're properly releasing resources, I'm a way less
>>> concerned about keeping some leaked memory while releasing them than I am
>>> concerned that a device removal that would cause an OOPS, or a deadly
>>> crash at the USB or ALSA stack that prevents other devices to be probed.
>>>
>>> Due to em28xx-v4l calling em28xx_release_resources(), now both the
>>> USB and ALSA stacks crashes if you remove a device while ALSA is streaming.
>>>
>>> That happens because em28xx, I2C, etc will be freed by em28xx-v4l, but
>>> those resources are still needed by em28xx-alsa. That makes that the
>>> .fini code there to cause crash at ALSA stack, and, sometimes, at the
>>> USB stack.
>> That's not true anymore, em28xx_release_resources() is now only called 
>> by the usb disconnect handler in the core _after_ all fini() functions 
>> have been called.
>> Maybe you tested that without my patch series ? See patch 7/8.
> No, we tested it with your full series applied:
> 	http://git.linuxtv.org/mchehab/experimental.git/shortlog/refs/heads/em28xx
Ok, but the above is nevertheless not true anymore.
The only place where em28xx_release_resources is called() is
em28xx_usb_disconnect().

>
>>> As nowadays lots of components depend on pulseaudio, the ALSA crash
>>> causes pulse to stop work, likely keeping it into some dead lock status.
>>> This makes the entire machine to become really slow, when it doesn't
>>> crash.
>>>
>>> This is a serious regression that should be fixed.
>>>
>>> This patch series for sure fixes it. As I said, there are two independent
>>> series of tests verifying that (both using several different em28xx
>>> devices).
>>>
>>>> Be warned !
>>>>
>>>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frank Schäfer Jan. 14, 2014, 8:57 p.m. UTC | #9

Am 14.01.2014 20:31, schrieb Mauro Carvalho Chehab:
>
> Ok, patch 5 is not needed anymore.
>
> However, after a series of removals and re-inserts, I got these:
>
>
> [120982.699455] ------------[ cut here ]------------
> [120982.699509] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
> [120982.699539] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was 0000000d00000007. (prev=ffff880101dbfb48).
> [120982.699566] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
> [120982.701828]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
> [120982.703581] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D      3.13.0-rc1+ #24
> [120982.703610] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
> [120982.703638]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
> [120982.703794]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
> [120982.703924]  ffff880101dbfb48 0000000000000282 0000000000000000 ffff88011f63bc08
> [120982.704056] Call Trace:
> [120982.704093]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
> [120982.704129]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
> [120982.704162]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
> [120982.704197]  [<ffffffff81343bac>] __list_add+0xac/0xc0
> [120982.704233]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
> [120982.704432]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
> [120982.704469]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
> [120982.704505]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
> [120982.704543]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
> [120982.704581]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
> [120982.704619]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120982.704658]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120982.704719]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
> [120982.704756]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
> [120982.704792]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120982.704829]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120982.704867]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
> [120982.704903]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
> [120982.704939]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
> [120982.704974]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
> [120982.705006]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
> [120982.705039]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
> [120982.705070] ---[ end trace 7e24c4fe20a76f18 ]---
> [120985.899453] ------------[ cut here ]------------
> [120985.899502] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
> [120985.899531] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was           (null). (prev=ffff88018cfb0348).
> [120985.899559] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
> [120985.901761]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
> [120985.903502] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
> [120985.903530] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
> [120985.903557]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
> [120985.903686]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb0348 ffff88019ffce4f8
> [120985.903826]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
> [120985.903955] Call Trace:
> [120985.903988]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
> [120985.904021]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
> [120985.904052]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
> [120985.904083]  [<ffffffff81343bac>] __list_add+0xac/0xc0
> [120985.904116]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
> [120985.904295]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
> [120985.904330]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
> [120985.904363]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
> [120985.904398]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
> [120985.904433]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
> [120985.904470]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.904504]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.904540]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
> [120985.904575]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
> [120985.904608]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.904641]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.904676]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
> [120985.904710]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
> [120985.904743]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
> [120985.904780]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
> [120985.904810]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
> [120985.904843]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
> [120985.904871] ---[ end trace 7e24c4fe20a76f19 ]---
> [120985.904896] ------------[ cut here ]------------
> [120985.904922] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:36 __list_add+0x8a/0xc0()
> [120985.904953] list_add double add: new=ffff88018cfb0348, prev=ffff88018cfb0348, next=ffff88019ffce4f8.
> [120985.904978] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
> [120985.907016]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
> [120985.908654] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
> [120985.908680] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
> [120985.908707]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
> [120985.908988]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb0348 ffff88019ffce4f8
> [120985.909110]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
> [120985.909230] Call Trace:
> [120985.909268]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
> [120985.909305]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
> [120985.909336]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
> [120985.909380]  [<ffffffff81343b8a>] __list_add+0x8a/0xc0
> [120985.909410]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
> [120985.909442]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
> [120985.909474]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
> [120985.909505]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
> [120985.909537]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
> [120985.909570]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
> [120985.909604]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.909636]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.909670]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
> [120985.909702]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
> [120985.909732]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.909764]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.909799]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
> [120985.909830]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
> [120985.909860]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
> [120985.909890]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
> [120985.909918]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
> [120985.909948]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
> [120985.909974] ---[ end trace 7e24c4fe20a76f1a ]---
> [120985.910186] ------------[ cut here ]------------
> [120985.910213] WARNING: CPU: 0 PID: 7953 at lib/list_debug.c:33 __list_add+0xac/0xc0()
> [120985.910236] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was ffff88018cfb0348. (prev=ffff88018cfb0348).
> [120985.910257] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
> [120985.912159]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
> [120985.913642] CPU: 0 PID: 7953 Comm: xawtv Tainted: G      D W    3.13.0-rc1+ #24
> [120985.913667] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
> [120985.913691]  0000000000000009 ffff88011f63bb70 ffffffff816a03c6 ffff88011f63bbb8
> [120985.913805]  ffff88011f63bba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
> [120985.913920]  ffff88018cfb0348 0000000000000282 0000000000000000 ffff88011f63bc08
> [120985.914035] Call Trace:
> [120985.914063]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
> [120985.914093]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
> [120985.914121]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
> [120985.914150]  [<ffffffff81343bac>] __list_add+0xac/0xc0
> [120985.914180]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
> [120985.914353]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
> [120985.914385]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
> [120985.914416]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
> [120985.914449]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
> [120985.914482]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
> [120985.914515]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.914548]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120985.914580]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
> [120985.914612]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
> [120985.914643]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.914675]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120985.914708]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
> [120985.914740]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
> [120985.914770]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
> [120985.914799]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
> [120985.914828]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
> [120985.914857]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
> [120985.914884] ---[ end trace 7e24c4fe20a76f1b ]---
> [120991.576999] em2882/3 #0: submit of audio urb failed
> [120991.781651] ------------[ cut here ]------------
> [120991.781700] WARNING: CPU: 0 PID: 7979 at lib/list_debug.c:33 __list_add+0xac/0xc0()
> [120991.781730] list_add corruption. prev->next should be next (ffff88019ffce4f8), but was           (null). (prev=ffff88018cfb1b48).
> [120991.781756] Modules linked in: lgdt330x tuner_xc2028 tuner tvp5150 rc_hauppauge em28xx_rc rc_core xc5000 drxk em28xx_dvb dvb_core em28xx_alsa em28xx_v4l videobuf2_vmalloc videobuf2_memops videobuf2_core em28xx tveeprom v4l2_common videodev media netconsole usb_storage fuse ccm nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep arc4 vfat fat iwldvm mac80211 iwlwifi cfg80211 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel
> [120991.783977]  snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support btusb bluetooth joydev microcode serio_raw r8169 mii i2c_i801 lpc_ich mfd_core rfkill snd_page_alloc snd_timer snd mei_me soundcore mei shpchp nfsd auth_rpcgss nfs_acl lockd sunrpc nouveau i915 ttm i2c_algo_bit drm_kms_helper drm i2c_core mxm_wmi wmi video [last unloaded: videobuf2_memops]
> [120991.785750] CPU: 0 PID: 7979 Comm: tvtime Tainted: G      D W    3.13.0-rc1+ #24
> [120991.785779] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 550P5C/550P7C/SAMSUNG_NP1234567890, BIOS P04ABI.013.130220.dg 02/20/2013
> [120991.785806]  0000000000000009 ffff88001254fb70 ffffffff816a03c6 ffff88001254fbb8
> [120991.785938]  ffff88001254fba8 ffffffff8106aaad ffff88018cfb3f48 ffff88019ffce4f8
> [120991.786102]  ffff88018cfb1b48 0000000000000282 0000000000000000 ffff88001254fc08
> [120991.786233] Call Trace:
> [120991.786268]  [<ffffffff816a03c6>] dump_stack+0x45/0x56
> [120991.786319]  [<ffffffff8106aaad>] warn_slowpath_common+0x7d/0xa0
> [120991.786359]  [<ffffffff8106ab1c>] warn_slowpath_fmt+0x4c/0x50
> [120991.786393]  [<ffffffff81343bac>] __list_add+0xac/0xc0
> [120991.786429]  [<ffffffffa04fd70b>] buffer_queue+0x7b/0xb0 [em28xx_v4l]
> [120991.786619]  [<ffffffffa04862d4>] __enqueue_in_driver+0x74/0x80 [videobuf2_core]
> [120991.786656]  [<ffffffffa04878c0>] vb2_streamon+0xa0/0x140 [videobuf2_core]
> [120991.786690]  [<ffffffffa04879a8>] vb2_ioctl_streamon+0x48/0x50 [videobuf2_core]
> [120991.786728]  [<ffffffffa04b7a8a>] v4l_streamon+0x1a/0x20 [videodev]
> [120991.786767]  [<ffffffffa04bb644>] __video_do_ioctl+0x2a4/0x330 [videodev]
> [120991.786804]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120991.786841]  [<ffffffffa04bb3a0>] ? v4l_dbg_s_register+0x150/0x150 [videodev]
> [120991.786880]  [<ffffffffa04bcae0>] video_usercopy+0x240/0x5d0 [videodev]
> [120991.786917]  [<ffffffff810b811d>] ? trace_hardirqs_on+0xd/0x10
> [120991.786952]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120991.786988]  [<ffffffffa04b663b>] ? v4l2_ioctl+0x5b/0x160 [videodev]
> [120991.787027]  [<ffffffffa04bce85>] video_ioctl2+0x15/0x20 [videodev]
> [120991.787063]  [<ffffffffa04b6703>] v4l2_ioctl+0x123/0x160 [videodev]
> [120991.787098]  [<ffffffff811db8b0>] do_vfs_ioctl+0x300/0x520
> [120991.787133]  [<ffffffff812c7a96>] ? file_has_perm+0x86/0xa0
> [120991.787165]  [<ffffffff811dbb51>] SyS_ioctl+0x81/0xa0
> [120991.787199]  [<ffffffff816b1929>] system_call_fastpath+0x16/0x1b
> [120991.787230] ---[ end trace 7e24c4fe20a76f1c ]---
Nice. :/
Maybe it's time to check the videobuf2 handling.


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Jan. 14, 2014, 9:20 p.m. UTC | #10

Em Tue, 14 Jan 2014 21:48:16 +0100
Frank Schäfer <[e-mail]> escreveu:

> Am 14.01.2014 19:55, schrieb Mauro Carvalho Chehab:
> > Em Tue, 14 Jan 2014 19:13:00 +0100
> > Frank Schäfer <[e-mail]> escreveu:
> >
...
> >> At first glance it seems there are at least 2 issues:
> >> 1.) use after freeing in v4l-extension (happens when the device is 
> >> closed after the usb disconnect)
> > That's basically what this patch fixes. Both V4L2 and ALSA handles it
> > well, if you warn the subsystem that a device will be removed.
> >
> > If are there still any issues, we may add a kref_get() at device open,
> > an a kref_put() at device close on the affected sub-driver.
> Ok, I've tested it and I was right here.
> This is what happens when closing a disconnected device:
> 
> [  144.045691] usb 1-8: USB disconnect, device number 7
> [  144.047387] em2765 #0: disconnecting em2765 #0 video
> [  144.047403] em2765 #0: V4L2 device video1 deregistered
> [  144.050197] em2765 #0: Deregistering snapshot button
> [  144.058336] em2765 #0: Freeing device
> [  147.525267] : em28xx_v4l2_close() called
> [  147.525287] : em28xx_videodevice_release() called
> 
> 
> I will make some tests tomorrow, but here is a first suggestion how to
> fix this:
> 
> Remove the kref_put() call from em28xx_v4l2_fini().
> Instead, add the following lines to em28xx_videodevice_release():
> 
> if (dev->users == 0) {
>         dev->users--; /* avoid multiple kref_put() calls when the
> devices are unregistered and no device is open */
>         kref_put(&dev->ref, em28xx_free_device);
> }
> 
> That should fix it.

What I actually did on version 2 (already submitted) is that it is calling 
kref_get() at open, and kref_put() at close.

> Interestingly no oops happens. I will have to take a closer look at this
> tomorrow, but I suspect that the dev we obtain from struct file filp is
> an outdated copy of the original device struct.

Likely.

> If that would be true and no bad things can happen in the close()
> function we actually wouldn't need kref for the v4l extension at all.

Still, it will be writing on a deallocated buffer, and this can be
making some memory used by some other part of the Kernel dirty.

> Of course, the ideal solution would be, if we could just clear the
> device struct at the end of the usb disconnect handler, because we can
> be sure that the fini() functions have already made sure that dev isn't
> used anymore.
>
> Btw, what happens in em28xx-audio ?
> Does the ALSA core also allow to call the close() function when the
> device is already gone ?

I suspect so. This is what happens when I remove HVR-950 while both
audio and video are still streaming:

[ 4313.540907] usb 3-4: USB disconnect, device number 7
[ 4313.541280] em2882/3 #0: Disconnecting em2882/3 #0
[ 4313.541313] em2882/3 #0: Closing video extension
[ 4313.541352] em2882/3 #0: V4L2 device vbi0 deregistered
[ 4313.541635] em2882/3 #0: V4L2 device video0 deregistered
[ 4313.542188] em2882/3 #0: Closing audio extension

(I waited for ~5 secs before removing the driver)

[ 4317.470747] em2882/3 #0: couldn't setup AC97 register 2
[ 4317.470772] em2882/3 #0: couldn't setup AC97 register 4
[ 4317.470785] em2882/3 #0: couldn't setup AC97 register 6
[ 4317.470797] em2882/3 #0: couldn't setup AC97 register 54
[ 4317.470810] em2882/3 #0: couldn't setup AC97 register 56
[ 4317.470950] em2882/3 #0: Closing DVB extension
[ 4317.471890] xc2028 19-0061: destroying instance
[ 4317.471913] em2882/3 #0: Closing input extension
[ 4317.489374] em2882/3 #0: Freeing device

As the code now have a kref for open/close on both audio and video
extensions, that means that em28xx close was called after device
removal, as otherwise, we won't see the "Freeing device" print.

Frank Schäfer Jan. 15, 2014, 6:48 p.m. UTC | #11

Am 14.01.2014 22:20, schrieb Mauro Carvalho Chehab:
> Em Tue, 14 Jan 2014 21:48:16 +0100
> Frank Schäfer <[e-mail]> escreveu:
>
>> Am 14.01.2014 19:55, schrieb Mauro Carvalho Chehab:
>>> Em Tue, 14 Jan 2014 19:13:00 +0100
>>> Frank Schäfer <[e-mail]> escreveu:
>>>
> ...
>>>> At first glance it seems there are at least 2 issues:
>>>> 1.) use after freeing in v4l-extension (happens when the device is 
>>>> closed after the usb disconnect)
>>> That's basically what this patch fixes. Both V4L2 and ALSA handles it
>>> well, if you warn the subsystem that a device will be removed.
>>>
>>> If are there still any issues, we may add a kref_get() at device open,
>>> an a kref_put() at device close on the affected sub-driver.
>> Ok, I've tested it and I was right here.
>> This is what happens when closing a disconnected device:
>>
>> [  144.045691] usb 1-8: USB disconnect, device number 7
>> [  144.047387] em2765 #0: disconnecting em2765 #0 video
>> [  144.047403] em2765 #0: V4L2 device video1 deregistered
>> [  144.050197] em2765 #0: Deregistering snapshot button
>> [  144.058336] em2765 #0: Freeing device
>> [  147.525267] : em28xx_v4l2_close() called
>> [  147.525287] : em28xx_videodevice_release() called
>>
>>
>> I will make some tests tomorrow, but here is a first suggestion how to
>> fix this:
>>
>> Remove the kref_put() call from em28xx_v4l2_fini().
>> Instead, add the following lines to em28xx_videodevice_release():
>>
>> if (dev->users == 0) {
>>         dev->users--; /* avoid multiple kref_put() calls when the
>> devices are unregistered and no device is open */
>>         kref_put(&dev->ref, em28xx_free_device);
>> }
>>
>> That should fix it.
> What I actually did on version 2 (already submitted) is that it is calling 
> kref_get() at open, and kref_put() at close.
Yes, that's even better.

>
>> Interestingly no oops happens. I will have to take a closer look at this
>> tomorrow, but I suspect that the dev we obtain from struct file filp is
>> an outdated copy of the original device struct.
> Likely.
>
>> If that would be true and no bad things can happen in the close()
>> function we actually wouldn't need kref for the v4l extension at all.
> Still, it will be writing on a deallocated buffer, and this can be
> making some memory used by some other part of the Kernel dirty.
>
>> Of course, the ideal solution would be, if we could just clear the
>> device struct at the end of the usb disconnect handler, because we can
>> be sure that the fini() functions have already made sure that dev isn't
>> used anymore.
>>
>> Btw, what happens in em28xx-audio ?
>> Does the ALSA core also allow to call the close() function when the
>> device is already gone ?
> I suspect so. This is what happens when I remove HVR-950 while both
> audio and video are still streaming:
>
> [ 4313.540907] usb 3-4: USB disconnect, device number 7
> [ 4313.541280] em2882/3 #0: Disconnecting em2882/3 #0
> [ 4313.541313] em2882/3 #0: Closing video extension
> [ 4313.541352] em2882/3 #0: V4L2 device vbi0 deregistered
> [ 4313.541635] em2882/3 #0: V4L2 device video0 deregistered
> [ 4313.542188] em2882/3 #0: Closing audio extension
>
> (I waited for ~5 secs before removing the driver)
>
> [ 4317.470747] em2882/3 #0: couldn't setup AC97 register 2
> [ 4317.470772] em2882/3 #0: couldn't setup AC97 register 4
> [ 4317.470785] em2882/3 #0: couldn't setup AC97 register 6
> [ 4317.470797] em2882/3 #0: couldn't setup AC97 register 54
> [ 4317.470810] em2882/3 #0: couldn't setup AC97 register 56
> [ 4317.470950] em2882/3 #0: Closing DVB extension
> [ 4317.471890] xc2028 19-0061: destroying instance
> [ 4317.471913] em2882/3 #0: Closing input extension
> [ 4317.489374] em2882/3 #0: Freeing device
>
> As the code now have a kref for open/close on both audio and video
> extensions, that means that em28xx close was called after device
> removal, as otherwise, we won't see the "Freeing device" print.
Ok, thanks.
I will make further tests now.

Regards,
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 97d9105e6830..8e959dae8358 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -878,6 +878,8 @@  static int em28xx_audio_init(struct em28xx *dev)
 
 	em28xx_info("Binding audio extension\n");
 
+	kref_get(&dev->ref);
+
 	printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
 			 "Rechberger\n");
 	printk(KERN_INFO
@@ -949,7 +951,7 @@  static int em28xx_audio_fini(struct em28xx *dev)
 	if (dev == NULL)
 		return 0;
 
-	if (dev->has_alsa_audio != 1) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
@@ -963,6 +965,7 @@  static int em28xx_audio_fini(struct em28xx *dev)
 		dev->adev.sndcard = NULL;
 	}
 
+	kref_put(&dev->ref, em28xx_free_device);
 	return 0;
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 3b332d527ccb..df92f417634a 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2867,16 +2867,18 @@  static void flush_request_modules(struct em28xx *dev)
 	flush_work(&dev->request_module_wk);
 }
 
-/*
- * em28xx_release_resources()
- * unregisters the v4l2,i2c and usb devices
- * called when the device gets disconnected or at module unload
-*/
-void em28xx_release_resources(struct em28xx *dev)
+/**
+ * em28xx_release_resources() -  unregisters the v4l2,i2c and usb devices
+ *
+ * @ref: struct kref for em28xx device
+ *
+ * This is called when all extensions and em28xx core unregisters a device
+ */
+void em28xx_free_device(struct kref *ref)
 {
-	/*FIXME: I2C IR should be disconnected */
+	struct em28xx *dev = kref_to_dev(ref);
 
-	mutex_lock(&dev->lock);
+	em28xx_info("Freeing device\n");
 
 	if (dev->def_i2c_bus)
 		em28xx_i2c_unregister(dev, 1);
@@ -2887,9 +2889,10 @@  void em28xx_release_resources(struct em28xx *dev)
 	/* Mark device as unused */
 	clear_bit(dev->devno, &em28xx_devused);
 
-	mutex_unlock(&dev->lock);
-};
-EXPORT_SYMBOL_GPL(em28xx_release_resources);
+	kfree(dev->alt_max_pkt_size_isoc);
+	kfree(dev);
+}
+EXPORT_SYMBOL_GPL(em28xx_free_device);
 
 /*
  * em28xx_init_dev()
@@ -3342,6 +3345,8 @@  static int em28xx_usb_probe(struct usb_interface *interface,
 			    dev->dvb_xfer_bulk ? "bulk" : "isoc");
 	}
 
+	kref_init(&dev->ref);
+
 	request_modules(dev);
 
 	/* Should be the last thing to do, to avoid newer udev's to
@@ -3390,12 +3395,7 @@  static void em28xx_usb_disconnect(struct usb_interface *interface)
 
 	em28xx_close_extension(dev);
 
-	em28xx_release_resources(dev);
-
-	if (!dev->users) {
-		kfree(dev->alt_max_pkt_size_isoc);
-		kfree(dev);
-	}
+	kref_put(&dev->ref, em28xx_free_device);
 }
 
 static struct usb_driver em28xx_usb_driver = {
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 5ea563e3f0e4..8674ae5fce06 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -1010,11 +1010,11 @@  static int em28xx_dvb_init(struct em28xx *dev)
 	em28xx_info("Binding DVB extension\n");
 
 	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
-
 	if (dvb == NULL) {
 		em28xx_info("em28xx_dvb: memory allocation failed\n");
 		return -ENOMEM;
 	}
+	kref_get(&dev->ref);
 	dev->dvb = dvb;
 	dvb->fe[0] = dvb->fe[1] = NULL;
 
@@ -1442,6 +1442,7 @@  static int em28xx_dvb_init(struct em28xx *dev)
 	dvb->adapter.mfe_shared = mfe_shared;
 
 	em28xx_info("DVB extension successfully initialized\n");
+
 ret:
 	em28xx_set_mode(dev, EM28XX_SUSPEND);
 	mutex_unlock(&dev->lock);
@@ -1492,6 +1493,8 @@  static int em28xx_dvb_fini(struct em28xx *dev)
 		dev->dvb = NULL;
 	}
 
+	kref_put(&dev->ref, em28xx_free_device);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 61c061f3a476..33388b5922a0 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -676,6 +676,8 @@  static int em28xx_ir_init(struct em28xx *dev)
 		return 0;
 	}
 
+	kref_get(&dev->ref);
+
 	if (dev->board.buttons)
 		em28xx_init_buttons(dev);
 
@@ -814,7 +816,7 @@  static int em28xx_ir_fini(struct em28xx *dev)
 
 	/* skip detach on non attached boards */
 	if (!ir)
-		return 0;
+		goto ref_put;
 
 	if (ir->rc)
 		rc_unregister_device(ir->rc);
@@ -822,6 +824,10 @@  static int em28xx_ir_fini(struct em28xx *dev)
 	/* done */
 	kfree(ir);
 	dev->ir = NULL;
+
+ref_put:
+	kref_put(&dev->ref, em28xx_free_device);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 587ff3fe9402..dc10cec772ba 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1922,8 +1922,7 @@  static int em28xx_v4l2_fini(struct em28xx *dev)
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
 	v4l2_device_unregister(&dev->v4l2_dev);
 
-	if (dev->users)
-		em28xx_warn("Device is open ! Memory deallocation is deferred on last close.\n");
+	kref_put(&dev->ref, em28xx_free_device);
 
 	return 0;
 }
@@ -1945,11 +1944,9 @@  static int em28xx_v4l2_close(struct file *filp)
 	mutex_lock(&dev->lock);
 
 	if (dev->users == 1) {
-		/* free the remaining resources if device is disconnected */
-		if (dev->disconnected) {
-			kfree(dev->alt_max_pkt_size_isoc);
+		/* No sense to try to write to the device */
+		if (dev->disconnected)
 			goto exit;
-		}
 
 		/* Save some power by putting tuner to sleep */
 		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
@@ -2201,6 +2198,8 @@  static int em28xx_v4l2_init(struct em28xx *dev)
 
 	em28xx_info("Registering V4L2 extension\n");
 
+	kref_get(&dev->ref);
+
 	mutex_lock(&dev->lock);
 
 	ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 5d5d1b6f0294..d38c08e4da60 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -32,6 +32,7 @@ 
 #include <linux/workqueue.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
+#include <linux/kref.h>
 #include <linux/videodev2.h>
 
 #include <media/videobuf2-vmalloc.h>
@@ -531,9 +532,11 @@  struct em28xx_i2c_bus {
 	enum em28xx_i2c_algo_type algo_type;
 };
 
-
 /* main device struct */
 struct em28xx {
+	struct kref ref;
+
+
 	/* generic device properties */
 	char name[30];		/* name (including minor) of the device */
 	int model;		/* index in the device_data struct */
@@ -706,6 +709,8 @@  struct em28xx {
 	struct em28xx_dvb *dvb;
 };
 
+#define kref_to_dev(d) container_of(d, struct em28xx, ref)
+
 struct em28xx_ops {
 	struct list_head next;
 	char *name;
@@ -763,7 +768,7 @@  extern struct em28xx_board em28xx_boards[];
 extern struct usb_device_id em28xx_id_table[];
 int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
 void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl);
-void em28xx_release_resources(struct em28xx *dev);
+void em28xx_free_device(struct kref *ref);
 
 /* Provided by em28xx-camera.c */
 int em28xx_detect_sensor(struct em28xx *dev);