Am 22.12.2013 15:53, schrieb Mauro Carvalho Chehab:
> Em Sun, 22 Dec 2013 14:51:53 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
>
>> Am 21.12.2013 20:55, schrieb Antti Palosaari:
>>> On 21.12.2013 18:51, Frank Schäfer wrote:
>>>> Hi Antti,
>>>>
>>>> thank you for reporting this issue.
>>>>
>>>> Am 18.12.2013 17:04, schrieb Antti Palosaari:
>>>>> That same lock debug deadlock is still there (maybe ~4 times I report
>>>>> it during 2 years). Is that possible to fix easily at all?
>>>> Patches are always welcome. ;)
>>> haha, I cannot simply learn every driver I meet some problems...
>> Hint:
>>
>> If you report a bug ~4 times in 2 years but never get a reply, it
>> usually means
>> a) nobody cares
>> b) nobody has the resources (time, knowledge) to fix it.
>>
>> So you either have to live with this issue or to fix it yourself.
> It is the latter case: fixing it require lots of efforts.
Yes, I know. ;-)
> One way to fix would be to change em28xx_close_extension() to
> something like:
>
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index f6076a512e8f..d938e2bbd62f 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -1350,13 +1350,19 @@ void em28xx_init_extension(struct em28xx *dev)
>
> void em28xx_close_extension(struct em28xx *dev)
> {
> + int (*fini)(struct em28xx *) = NULL;
> const struct em28xx_ops *ops = NULL;
>
> mutex_lock(&em28xx_devlist_mutex);
> list_for_each_entry(ops, &em28xx_extension_devlist, next) {
> - if (ops->fini)
> - ops->fini(dev);
> + fini = ops->fini;
> }
> list_del(&dev->devlist);
> mutex_unlock(&em28xx_devlist_mutex);
> +
> + if (fini) {
> + mutex_lock(&dev->lock);
> + fini(dev);
> + mutex_unlock(&dev->lock);
> + }
> }
>
> Please note that the above is not 100% correct, as one device may have
> more than one extension.
>
> Then, it should be sure that on every place that em28xx_close_extension()
> is called, dev->lock is not taken.
>
> As an alternative, eventually the extension list could be moved to the
> struct em28xx, but a device list is still needed, in order to handle
> extension module removal.
>
> Another way that would probably be better is to convert the em28xx
> code that handles extension (extension here is dvb, rc, alsa) to use
> krefs, And add a kref free code that would call ops->fini. Note that,
> in this case, dev itself would also need to be a kref.
>
> I suspect that using kref would would be cleaner, but a change like that
> would require to rewrite the extensions code.
I have zero knowledge about how the locking correctness stuff works, but
what about improving it ?
Shouldn't it notice that flush_work() waits until the work is done
before the lock is acquired ?
> Btw, there's a related RFC patchset that splits the V4L2 interface from
> em28xx, transforming it also into an extension. With such patch, a DVB
> only device should not call any v4l2 init code, nor require V4L2 to be
> enabled:
> https://patchwork.linuxtv.org/patch/17967/
Yes, I remember it and it would be a big step forward.
> The above RFC requires testing.
>
> I may be able to find some time to do work on it this end of the year,
> starting with the V4L2 split patchset, depending if I finish some other
> things already on my todo list.
I'm going to review the patch within the next days and do some testing.
Regards,
Frank
> Regards,
> Mauro
--
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
