Am 15.01.2014 22:13, schrieb Frank Schäfer:
> Am 14.01.2014 18:36, schrieb Mauro Carvalho Chehab:
>> 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 <m.chehab@xxxxxxxxxxx>
>> ---
>>
>> v2:
>> - patch was rebased;
>> - as em28xx-audio close uses struct em28xx dev, add a kref in order
>> to track audio open/close.
>>
>> drivers/media/usb/em28xx/em28xx-audio.c | 8 +++++++-
>> 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 | 14 ++++++++------
>> drivers/media/usb/em28xx/em28xx.h | 9 +++++++--
>> 6 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>> index 45bea1adc11c..73eeeaf6551f 100644
>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>> @@ -265,6 +265,8 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
>>
>> dprintk("opening device and trying to acquire exclusive lock\n");
>>
>> + kref_get(&dev->ref);
>> +
> When snd_em28xx_capture_open() fails, there will never be a close()
> call, right ?
> So kref_get() needs to be called when we are sure that the function
> succeeds.
>
>> runtime->hw = snd_em28xx_hw_capture;
>> if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
>> int nonblock = !!(substream->f_flags & O_NONBLOCK);
>> @@ -330,6 +332,7 @@ static int snd_em28xx_pcm_close(struct snd_pcm_substream *substream)
>> substream->runtime->dma_area = NULL;
>> }
>> mutex_unlock(&dev->lock);
>> + kref_put(&dev->ref, em28xx_free_device);
>>
>> return 0;
>> }
>> @@ -886,6 +889,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
>> @@ -958,7 +963,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) */
>> @@ -977,6 +982,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 e08d65b2e352..8fc0a437054e 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);
> This reintroduces the sysfs group removal warnings.
> Just keep em28xx_release_resources() as is and add let
> em28xx_free_device() call the the two kfrees() only.
>
>> /*
>> * 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
>> @@ -3385,12 +3390,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>>
>> em28xx_close_extension(dev);
>>
>> - em28xx_release_resources(dev);
> Keep this call (see above).
>
>> -
>> - 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 881a813836eb..7df21e33a923 100644
>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>> @@ -1005,11 +1005,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);
> This one needs to be moved between the em28xx_infp() and the kzalloc()
> call above.
>
>> dev->dvb = dvb;
>> dvb->fe[0] = dvb->fe[1] = NULL;
>>
>> @@ -1437,6 +1437,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);
>> @@ -1489,6 +1490,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 3d54c04e5230..c98d784a2772 100644
>> --- a/drivers/media/usb/em28xx/em28xx-input.c
>> +++ b/drivers/media/usb/em28xx/em28xx-input.c
>> @@ -675,6 +675,8 @@ static int em28xx_ir_init(struct em28xx *dev)
>> return 0;
>> }
>>
>> + kref_get(&dev->ref);
>> +
>> if (dev->board.buttons)
>> em28xx_init_buttons(dev);
>>
>> @@ -817,7 +819,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);
>> @@ -825,6 +827,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 aabcafbdab46..f801af8d3f61 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -1837,6 +1837,7 @@ static int em28xx_v4l2_open(struct file *filp)
>> video_device_node_name(vdev), v4l2_type_names[fh_type],
>> dev->users);
>>
>> + kref_get(&dev->ref);
> The same as with open() in em28xx-alsa:
> kref_get() needs to be called when we are sure that the function succeeds.
>
>> if (mutex_lock_interruptible(&dev->lock))
>> return -ERESTARTSYS;
>> @@ -1926,9 +1927,8 @@ 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");
>> mutex_unlock(&dev->lock);
>> + kref_put(&dev->ref, em28xx_free_device);
>>
>> return 0;
>> }
>> @@ -1950,11 +1950,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);
>> @@ -1975,6 +1973,8 @@ static int em28xx_v4l2_close(struct file *filp)
>> exit:
>> dev->users--;
>> mutex_unlock(&dev->lock);
>> + kref_put(&dev->ref, em28xx_free_device);
>> +
>> return 0;
>> }
>>
>> @@ -2206,6 +2206,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 10b817245f7e..8b3438891bb3 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>
>> @@ -533,9 +534,11 @@ struct em28xx_i2c_bus {
>> enum em28xx_i2c_algo_type algo_type;
>> };
>>
>> -
>> /* main device struct */
>> struct em28xx {
>> + struct kref ref;
>> +
>> +
> One empty line is enough.
>
>> /* generic device properties */
>> char name[30]; /* name (including minor) of the device */
>> int model; /* index in the device_data struct */
>> @@ -708,6 +711,8 @@ struct em28xx {
>> struct em28xx_dvb *dvb;
>> };
>>
>> +#define kref_to_dev(d) container_of(d, struct em28xx, ref)
>> +
> I wonder why this macro isn't provided by kref.h.
>
>> struct em28xx_ops {
>> struct list_head next;
>> char *name;
>> @@ -765,7 +770,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've made several tests with my devices and I'm facing some issues with
> the current media-tree (e.g. a kernel panic with a hard lockup when
> reconnecting the laplace webcam).
> But none of them seems to be related to this change.
> Anyway, when we're finished with it, we need to stabilize things...
Ping !
Are you going to continue working on this patch ?
I've planned to come up with some follow-up changes... ;-)
Regards,
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
