Re: em28xx DEADLOCK reported by lock debug

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Em Sun, 22 Dec 2013 14:51:53 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> 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.

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.

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/ 

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.

Regards,
Mauro
-- 

Cheers,
Mauro