| Message ID | 20131222125306.671b9960@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 22.12.2013 15:53, schrieb Mauro Carvalho Chehab: > 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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + } }