| Message ID | 9d8830150475bc4d4dde2fa1f5163aef82a35477.1458347578.git.mchehab@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/18/2016 06:42 PM, Mauro Carvalho Chehab wrote: > Now that the media_device can be used by multiple drivers, > via devres, we need to be sure that it will be dropped only > when all drivers stop using it. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> Looks good. Tested it doing bind/unbind, rmmod/modprobe on both au0828 and snd-usb-audio drivers. Also generated graphs when after bind/unbind, rmmod/modprobe. Graphs look good. Tested-by: Shuah Khan <shuahkh@osg.samsung.com> thanks, -- Shuah > --- > > v2: The kref is now used only when media_device is allocated via > the media_device*_devress. This warrants that other drivers won't be > affected, and that we can keep media_device_cleanup() balanced with > media_device_init(). > > drivers/media/media-device.c | 117 +++++++++++++++++++++++++-------- > drivers/media/usb/au0828/au0828-core.c | 3 +- > include/media/media-device.h | 28 ++++++++ > sound/usb/media.c | 3 +- > 4 files changed, 118 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index c32fa15cc76e..4a97d92a7e7d 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) > } > EXPORT_SYMBOL_GPL(media_device_init); > > -void media_device_cleanup(struct media_device *mdev) > +static void __media_device_cleanup(struct media_device *mdev) > { > ida_destroy(&mdev->entity_internal_idx); > mdev->entity_internal_idx_max = 0; > media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > +} > + > +void media_device_cleanup(struct media_device *mdev) > +{ > + __media_device_cleanup(mdev); > mutex_destroy(&mdev->graph_mutex); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct media_device *mdev, > { > int ret; > > + /* Check if mdev was ever registered at all */ > + mutex_lock(&mdev->graph_mutex); > + > /* Register the device node. */ > mdev->devnode.fops = &media_device_fops; > mdev->devnode.parent = mdev->dev; > @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct media_device *mdev, > > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - return ret; > + goto err; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > if (ret < 0) { > media_devnode_unregister(&mdev->devnode); > - return ret; > + goto err; > } > > dev_dbg(mdev->dev, "Media device registered\n"); > > - return 0; > +err: > + mutex_unlock(&mdev->graph_mutex); > + return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct media_device *mdev, > } > EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify); > > -void media_device_unregister(struct media_device *mdev) > +static void __media_device_unregister(struct media_device *mdev) > { > struct media_entity *entity; > struct media_entity *next; > struct media_interface *intf, *tmp_intf; > struct media_entity_notify *notify, *nextp; > > - if (mdev == NULL) > - return; > - > - mutex_lock(&mdev->graph_mutex); > - > - /* Check if mdev was ever registered at all */ > - if (!media_devnode_is_registered(&mdev->devnode)) { > - mutex_unlock(&mdev->graph_mutex); > - return; > - } > - > /* Remove all entities from the media device */ > list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > __media_device_unregister_entity(entity); > @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev) > kfree(intf); > } > > - mutex_unlock(&mdev->graph_mutex); > - > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + /* Check if mdev devnode was registered */ > + if (media_devnode_is_registered(&mdev->devnode)) { > + device_remove_file(&mdev->devnode.dev, &dev_attr_model); > + media_devnode_unregister(&mdev->devnode); > + } > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > + > +void media_device_unregister(struct media_device *mdev) > +{ > + if (mdev == NULL) > + return; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + mutex_unlock(&mdev->graph_mutex); > +} > EXPORT_SYMBOL_GPL(media_device_unregister); > > static void media_device_release_devres(struct device *dev, void *res) > { > } > > -struct media_device *media_device_get_devres(struct device *dev) > +static void do_media_device_unregister_devres(struct kref *kref) > { > + struct media_device_devres *mdev_devres; > struct media_device *mdev; > + int ret; > > - mdev = devres_find(dev, media_device_release_devres, NULL, NULL); > - if (mdev) > - return mdev; > + mdev_devres = container_of(kref, struct media_device_devres, kref); > > - mdev = devres_alloc(media_device_release_devres, > - sizeof(struct media_device), GFP_KERNEL); > - if (!mdev) > + if (!mdev_devres) > + return; > + > + mdev = &mdev_devres->mdev; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + __media_device_cleanup(mdev); > + mutex_unlock(&mdev->graph_mutex); > + mutex_destroy(&mdev->graph_mutex); > + > + ret = devres_destroy(mdev->dev, media_device_release_devres, > + NULL, NULL); > + pr_debug("%s: devres_destroy() returned %d\n", __func__, ret); > +} > + > +void media_device_unregister_devres(struct media_device *mdev) > +{ > + struct media_device_devres *mdev_devres; > + > + mdev_devres = container_of(mdev, struct media_device_devres, mdev); > + kref_put(&mdev_devres->kref, do_media_device_unregister_devres); > +} > +EXPORT_SYMBOL_GPL(media_device_unregister_devres); > + > +struct media_device *media_device_get_devres(struct device *dev) > +{ > + struct media_device_devres *mdev_devres, *ptr; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (mdev_devres) { > + kref_get(&mdev_devres->kref); > + return &mdev_devres->mdev; > + } > + > + mdev_devres = devres_alloc(media_device_release_devres, > + sizeof(struct media_device_devres), > + GFP_KERNEL); > + if (!mdev_devres) > return NULL; > - return devres_get(dev, mdev, NULL, NULL); > + > + ptr = devres_get(dev, mdev_devres, NULL, NULL); > + if (ptr) > + kref_init(&ptr->kref); > + else > + devres_free(mdev_devres); > + > + return &ptr->mdev; > } > EXPORT_SYMBOL_GPL(media_device_get_devres); > > struct media_device *media_device_find_devres(struct device *dev) > { > - return devres_find(dev, media_device_release_devres, NULL, NULL); > + struct media_device_devres *mdev_devres; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (!mdev_devres) > + return NULL; > + > + return &mdev_devres->mdev; > } > EXPORT_SYMBOL_GPL(media_device_find_devres); > > diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c > index 06da73f1ff22..060904ed8f20 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) > dev->media_dev->enable_source = NULL; > dev->media_dev->disable_source = NULL; > > - media_device_unregister(dev->media_dev); > - media_device_cleanup(dev->media_dev); > + media_device_unregister_devres(dev->media_dev); > dev->media_dev = NULL; > #endif > } > diff --git a/include/media/media-device.h b/include/media/media-device.h > index b21ef244ad3e..e59772ed8494 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -23,6 +23,7 @@ > #ifndef _MEDIA_DEVICE_H > #define _MEDIA_DEVICE_H > > +#include <linux/kref.h> > #include <linux/list.h> > #include <linux/mutex.h> > > @@ -382,6 +383,16 @@ struct media_device { > unsigned int notification); > }; > > +/** > + * struct media_device_devres - Media device device resource > + * @mdev: pointer to struct media_device > + * @kref: Object refcount > + */ > +struct media_device_devres { > + struct media_device mdev; > + struct kref kref; > +}; > + > /* We don't need to include pci.h or usb.h here */ > struct pci_dev; > struct usb_device; > @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct device *dev); > */ > struct media_device *media_device_find_devres(struct device *dev); > > +/** > + * media_device_unregister_devres) - Unregister media device allocated as > + * as device resource > + * > + * @dev: pointer to struct &device. > + * > + * Devices allocated via media_device_get_devres should be de-alocalted > + * and freed via this function. Callers should not call > + * media_device_unregister() nor media_device_cleanup() on devices > + * allocated via media_device_get_devres(). > + */ > +void media_device_unregister_devres(struct media_device *mdev); > + > /* Iterate over all entities. */ > #define media_device_for_each_entity(entity, mdev) \ > list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) > @@ -688,6 +712,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev) > return NULL; > } > > +static inline void media_device_unregister_devres(struct media_device *mdev) > +{ > +} > + > static inline void media_device_pci_init(struct media_device *mdev, > struct pci_dev *pci_dev, > char *name) > diff --git a/sound/usb/media.c b/sound/usb/media.c > index 93a50d01490c..f78955fd0d6e 100644 > --- a/sound/usb/media.c > +++ b/sound/usb/media.c > @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip) > media_snd_mixer_delete(chip); > > if (mdev) { > - if (media_devnode_is_registered(&mdev->devnode)) > - media_device_unregister(mdev); > + media_device_unregister_devres(mdev); > chip->media_dev = NULL; > } > } >
Hi Mauro, Thank you for the patch. On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: > Now that the media_device can be used by multiple drivers, > via devres, we need to be sure that it will be dropped only > when all drivers stop using it. I've discussed this with Shuah previously and I'm surprised to see that the problem hasn't been fixed : using devres for this purpose is just plain wrong. The empty media_device_release_devres() function should have given you a hint. What we need instead is a list of media devices indexed by struct device (for this use case) or by struct device_node (for DT use cases). It will both simplify the code and get rid of the devres abuse. Shuah, if I recall correctly you worked on implementing such a solution after our last discussion on the topic. Could you please update us on the status ? In the mean time, let's hold off on this patch, and merge a proper solution instead. > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > --- > > v2: The kref is now used only when media_device is allocated via > the media_device*_devress. This warrants that other drivers won't be > affected, and that we can keep media_device_cleanup() balanced with > media_device_init(). > > drivers/media/media-device.c | 117 ++++++++++++++++++++++-------- > drivers/media/usb/au0828/au0828-core.c | 3 +- > include/media/media-device.h | 28 ++++++++ > sound/usb/media.c | 3 +- > 4 files changed, 118 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index c32fa15cc76e..4a97d92a7e7d 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) > } > EXPORT_SYMBOL_GPL(media_device_init); > > -void media_device_cleanup(struct media_device *mdev) > +static void __media_device_cleanup(struct media_device *mdev) > { > ida_destroy(&mdev->entity_internal_idx); > mdev->entity_internal_idx_max = 0; > media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > +} > + > +void media_device_cleanup(struct media_device *mdev) > +{ > + __media_device_cleanup(mdev); > mutex_destroy(&mdev->graph_mutex); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct > media_device *mdev, { > int ret; > > + /* Check if mdev was ever registered at all */ > + mutex_lock(&mdev->graph_mutex); > + > /* Register the device node. */ > mdev->devnode.fops = &media_device_fops; > mdev->devnode.parent = mdev->dev; > @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct > media_device *mdev, > > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - return ret; > + goto err; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > if (ret < 0) { > media_devnode_unregister(&mdev->devnode); > - return ret; > + goto err; > } > > dev_dbg(mdev->dev, "Media device registered\n"); > > - return 0; > +err: > + mutex_unlock(&mdev->graph_mutex); > + return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct > media_device *mdev, } > EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify); > > -void media_device_unregister(struct media_device *mdev) > +static void __media_device_unregister(struct media_device *mdev) > { > struct media_entity *entity; > struct media_entity *next; > struct media_interface *intf, *tmp_intf; > struct media_entity_notify *notify, *nextp; > > - if (mdev == NULL) > - return; > - > - mutex_lock(&mdev->graph_mutex); > - > - /* Check if mdev was ever registered at all */ > - if (!media_devnode_is_registered(&mdev->devnode)) { > - mutex_unlock(&mdev->graph_mutex); > - return; > - } > - > /* Remove all entities from the media device */ > list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > __media_device_unregister_entity(entity); > @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device > *mdev) kfree(intf); > } > > - mutex_unlock(&mdev->graph_mutex); > - > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + /* Check if mdev devnode was registered */ > + if (media_devnode_is_registered(&mdev->devnode)) { > + device_remove_file(&mdev->devnode.dev, &dev_attr_model); > + media_devnode_unregister(&mdev->devnode); > + } > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > + > +void media_device_unregister(struct media_device *mdev) > +{ > + if (mdev == NULL) > + return; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + mutex_unlock(&mdev->graph_mutex); > +} > EXPORT_SYMBOL_GPL(media_device_unregister); > > static void media_device_release_devres(struct device *dev, void *res) > { > } > > -struct media_device *media_device_get_devres(struct device *dev) > +static void do_media_device_unregister_devres(struct kref *kref) > { > + struct media_device_devres *mdev_devres; > struct media_device *mdev; > + int ret; > > - mdev = devres_find(dev, media_device_release_devres, NULL, NULL); > - if (mdev) > - return mdev; > + mdev_devres = container_of(kref, struct media_device_devres, kref); > > - mdev = devres_alloc(media_device_release_devres, > - sizeof(struct media_device), GFP_KERNEL); > - if (!mdev) > + if (!mdev_devres) > + return; > + > + mdev = &mdev_devres->mdev; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + __media_device_cleanup(mdev); > + mutex_unlock(&mdev->graph_mutex); > + mutex_destroy(&mdev->graph_mutex); > + > + ret = devres_destroy(mdev->dev, media_device_release_devres, > + NULL, NULL); > + pr_debug("%s: devres_destroy() returned %d\n", __func__, ret); > +} > + > +void media_device_unregister_devres(struct media_device *mdev) > +{ > + struct media_device_devres *mdev_devres; > + > + mdev_devres = container_of(mdev, struct media_device_devres, mdev); > + kref_put(&mdev_devres->kref, do_media_device_unregister_devres); > +} > +EXPORT_SYMBOL_GPL(media_device_unregister_devres); > + > +struct media_device *media_device_get_devres(struct device *dev) > +{ > + struct media_device_devres *mdev_devres, *ptr; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (mdev_devres) { > + kref_get(&mdev_devres->kref); > + return &mdev_devres->mdev; > + } > + > + mdev_devres = devres_alloc(media_device_release_devres, > + sizeof(struct media_device_devres), > + GFP_KERNEL); > + if (!mdev_devres) > return NULL; > - return devres_get(dev, mdev, NULL, NULL); > + > + ptr = devres_get(dev, mdev_devres, NULL, NULL); > + if (ptr) > + kref_init(&ptr->kref); > + else > + devres_free(mdev_devres); > + > + return &ptr->mdev; > } > EXPORT_SYMBOL_GPL(media_device_get_devres); > > struct media_device *media_device_find_devres(struct device *dev) > { > - return devres_find(dev, media_device_release_devres, NULL, NULL); > + struct media_device_devres *mdev_devres; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (!mdev_devres) > + return NULL; > + > + return &mdev_devres->mdev; > } > EXPORT_SYMBOL_GPL(media_device_find_devres); > > diff --git a/drivers/media/usb/au0828/au0828-core.c > b/drivers/media/usb/au0828/au0828-core.c index 06da73f1ff22..060904ed8f20 > 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct > au0828_dev *dev) dev->media_dev->enable_source = NULL; > dev->media_dev->disable_source = NULL; > > - media_device_unregister(dev->media_dev); > - media_device_cleanup(dev->media_dev); > + media_device_unregister_devres(dev->media_dev); > dev->media_dev = NULL; > #endif > } > diff --git a/include/media/media-device.h b/include/media/media-device.h > index b21ef244ad3e..e59772ed8494 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -23,6 +23,7 @@ > #ifndef _MEDIA_DEVICE_H > #define _MEDIA_DEVICE_H > > +#include <linux/kref.h> > #include <linux/list.h> > #include <linux/mutex.h> > > @@ -382,6 +383,16 @@ struct media_device { > unsigned int notification); > }; > > +/** > + * struct media_device_devres - Media device device resource > + * @mdev: pointer to struct media_device > + * @kref: Object refcount > + */ > +struct media_device_devres { > + struct media_device mdev; > + struct kref kref; > +}; > + > /* We don't need to include pci.h or usb.h here */ > struct pci_dev; > struct usb_device; > @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct > device *dev); */ > struct media_device *media_device_find_devres(struct device *dev); > > +/** > + * media_device_unregister_devres) - Unregister media device allocated as > + * as device resource > + * > + * @dev: pointer to struct &device. > + * > + * Devices allocated via media_device_get_devres should be de-alocalted > + * and freed via this function. Callers should not call > + * media_device_unregister() nor media_device_cleanup() on devices > + * allocated via media_device_get_devres(). > + */ > +void media_device_unregister_devres(struct media_device *mdev); > + > /* Iterate over all entities. */ > #define media_device_for_each_entity(entity, mdev) \ > list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) > @@ -688,6 +712,10 @@ static inline struct media_device > *media_device_find_devres(struct device *dev) return NULL; > } > > +static inline void media_device_unregister_devres(struct media_device > *mdev) > +{ > +} > + > static inline void media_device_pci_init(struct media_device *mdev, > struct pci_dev *pci_dev, > char *name) > diff --git a/sound/usb/media.c b/sound/usb/media.c > index 93a50d01490c..f78955fd0d6e 100644 > --- a/sound/usb/media.c > +++ b/sound/usb/media.c > @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip) > media_snd_mixer_delete(chip); > > if (mdev) { > - if (media_devnode_is_registered(&mdev->devnode)) > - media_device_unregister(mdev); > + media_device_unregister_devres(mdev); > chip->media_dev = NULL; > } > }
Em Mon, 21 Mar 2016 13:10:33 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > Thank you for the patch. > > On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: > > Now that the media_device can be used by multiple drivers, > > via devres, we need to be sure that it will be dropped only > > when all drivers stop using it. > > I've discussed this with Shuah previously and I'm surprised to see that the > problem hasn't been fixed : using devres for this purpose is just plain wrong. I didn't follow your discussions with Shuah. I'm pretty sure I didn't see any such reply to the /22 patch series. For sure there are other approaches, although I wouldn't say that this approach is plain wrong. It was actually suggested by Greg KH at the USB summit, back in 2011: https://lkml.org/lkml/2011/8/21/61 It works fine in the cases like the ones Shuah is currently addressing: USB devices that have multiple interfaces handled by independent drivers. Going further, right now, as far as I'm aware of, there are only two use cases for a driver-independent media_device struct in the media subsystem (on the upstream Kernel): - USB devices with USB Audio Class: au0828 and em28xx drivers, plus snd-usb-audio; - bt878/bt879 PCI devices, where the DVB driver is independent from the V4L2 one (affects bt87x and bttv drivers). The devres approach fits well for both use cases. Ok, there are a plenty of OOT SoC drivers that might benefit of some other solution, but we should care about them only if/when they got upstreamed. > The empty media_device_release_devres() function should have given you a hint. > > What we need instead is a list of media devices indexed by struct device (for > this use case) or by struct device_node (for DT use cases). It will both > simplify the code and get rid of the devres abuse. Yeah, Shuah's approach should be changed to a different one, in order to work for DT use cases. It would be good to have a real DT use case for us to validate the solution, before we start implementing something in the wild. Still, it would very likely need a kref there, in order to destroy the media controller struct only after all drivers stop using it. > Shuah, if I recall correctly you worked on implementing such a solution after > our last discussion on the topic. Could you please update us on the status ? I saw a Shuah's email proposing to discuss this at the media summit. > In the mean time, let's hold off on this patch, and merge a proper solution > instead. Well, we should send a fix for the current issues for Kernel 4.6. As the number of drivers that would be using this internal API is small (right now, only 2 drivers), replacing devres by some other strategy in the future should be easy. 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
On 03/21/2016 05:10 AM, Laurent Pinchart wrote: > Hi Mauro, > > Thank you for the patch. > > On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: >> Now that the media_device can be used by multiple drivers, >> via devres, we need to be sure that it will be dropped only >> when all drivers stop using it. > > I've discussed this with Shuah previously and I'm surprised to see that the > problem hasn't been fixed : using devres for this purpose is just plain wrong. > The empty media_device_release_devres() function should have given you a hint. > > What we need instead is a list of media devices indexed by struct device (for > this use case) or by struct device_node (for DT use cases). It will both > simplify the code and get rid of the devres abuse. > > Shuah, if I recall correctly you worked on implementing such a solution after > our last discussion on the topic. Could you please update us on the status ? > It is work in progress. I have a working prototype for au0828 which is an easier case. I am working on resolving a couple of issues to differentiate media devices allocated using the global media device list vs. the ones embedded in the driver data structures. We have many of those. I had to put this work on the back burner to get the au0882 and snd-usb-audio wrapped up. I can get the RFC patches on top of the au0882 and snd-usb-audio. We can discuss them at the upcoming media summit. > In the mean time, let's hold off on this patch, and merge a proper solution > instead. I think Mauro's restructure helps us with such differentiation and it will be easy enough to change out devres to get media get API. thanks, -- Shuah
On 03/21/2016 05:58 AM, Mauro Carvalho Chehab wrote: > Em Mon, 21 Mar 2016 13:10:33 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > >> Hi Mauro, >> >> Thank you for the patch. >> >> On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: >>> Now that the media_device can be used by multiple drivers, >>> via devres, we need to be sure that it will be dropped only >>> when all drivers stop using it. >> >> I've discussed this with Shuah previously and I'm surprised to see that the >> problem hasn't been fixed : using devres for this purpose is just plain wrong. > > I didn't follow your discussions with Shuah. I'm pretty sure I didn't > see any such reply to the /22 patch series. > > For sure there are other approaches, although I wouldn't say that this > approach is plain wrong. It was actually suggested by Greg KH at the > USB summit, back in 2011: > https://lkml.org/lkml/2011/8/21/61 > > It works fine in the cases like the ones Shuah is currently addressing: > USB devices that have multiple interfaces handled by independent drivers. > > Going further, right now, as far as I'm aware of, there are only two use > cases for a driver-independent media_device struct in the media subsystem > (on the upstream Kernel): > > - USB devices with USB Audio Class: au0828 and em28xx drivers, > plus snd-usb-audio; > > - bt878/bt879 PCI devices, where the DVB driver is independent > from the V4L2 one (affects bt87x and bttv drivers). > > The devres approach fits well for both use cases. > > Ok, there are a plenty of OOT SoC drivers that might benefit of some > other solution, but we should care about them only if/when they got > upstreamed. > >> The empty media_device_release_devres() function should have given you a hint. >> >> What we need instead is a list of media devices indexed by struct device (for >> this use case) or by struct device_node (for DT use cases). It will both >> simplify the code and get rid of the devres abuse. > > Yeah, Shuah's approach should be changed to a different one, in order to > work for DT use cases. It would be good to have a real DT use case for us > to validate the solution, before we start implementing something in the > wild. > > Still, it would very likely need a kref there, in order to destroy the > media controller struct only after all drivers stop using it. > >> Shuah, if I recall correctly you worked on implementing such a solution after >> our last discussion on the topic. Could you please update us on the status ? > > I saw a Shuah's email proposing to discuss this at the media summit. Right. Now that the USB devices with USB Audio Class work is in upstream, I have a bit of breathing room. :) I have a working solution for the media get api, ready for RFC. I didn't want to add that in the middle of Media Controller Next Gen and the au0828 and snd-usb-audio work. We have had several issues we had to address and fix. I will send RFC patches for the media get API which is close to getting done. > >> In the mean time, let's hold off on this patch, and merge a proper solution >> instead. > > Well, we should send a fix for the current issues for Kernel 4.6. We can't hold off on this patch. Also this patch will make a good base for the RFC series and changing our devres with media get. We still have the same issues to address, such as how do we differentiate how the media device is allocated. This is necessary when it is time to release the media device in the unregister path. > > As the number of drivers that would be using this internal API is small > (right now, only 2 drivers), replacing devres by some other strategy > in the future should be easy. > Yes. Please see above. It is a very simple change out. I have it in the prototype already. thanks, -- Shuah
On 03/18/2016 06:42 PM, Mauro Carvalho Chehab wrote: > Now that the media_device can be used by multiple drivers, > via devres, we need to be sure that it will be dropped only > when all drivers stop using it. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > --- > > v2: The kref is now used only when media_device is allocated via > the media_device*_devress. This warrants that other drivers won't be > affected, and that we can keep media_device_cleanup() balanced with > media_device_init(). Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind snd_usb_audio loop 1000 times. Didn't see any lock warnings on a KASAN enabled kernel (lock testing enabled). No use-after-free errors during these runs. Ran device removal test and rmmod and modprobe tests on both drivers. Generated graph after the runs and the graph looks good. Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com> Tested-by: Shuah Khan <shuahkh@osg.samsung.com> thanks, -- Shuah > > drivers/media/media-device.c | 117 +++++++++++++++++++++++++-------- > drivers/media/usb/au0828/au0828-core.c | 3 +- > include/media/media-device.h | 28 ++++++++ > sound/usb/media.c | 3 +- > 4 files changed, 118 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index c32fa15cc76e..4a97d92a7e7d 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) > } > EXPORT_SYMBOL_GPL(media_device_init); > > -void media_device_cleanup(struct media_device *mdev) > +static void __media_device_cleanup(struct media_device *mdev) > { > ida_destroy(&mdev->entity_internal_idx); > mdev->entity_internal_idx_max = 0; > media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > +} > + > +void media_device_cleanup(struct media_device *mdev) > +{ > + __media_device_cleanup(mdev); > mutex_destroy(&mdev->graph_mutex); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct media_device *mdev, > { > int ret; > > + /* Check if mdev was ever registered at all */ > + mutex_lock(&mdev->graph_mutex); > + > /* Register the device node. */ > mdev->devnode.fops = &media_device_fops; > mdev->devnode.parent = mdev->dev; > @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct media_device *mdev, > > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - return ret; > + goto err; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > if (ret < 0) { > media_devnode_unregister(&mdev->devnode); > - return ret; > + goto err; > } > > dev_dbg(mdev->dev, "Media device registered\n"); > > - return 0; > +err: > + mutex_unlock(&mdev->graph_mutex); > + return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct media_device *mdev, > } > EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify); > > -void media_device_unregister(struct media_device *mdev) > +static void __media_device_unregister(struct media_device *mdev) > { > struct media_entity *entity; > struct media_entity *next; > struct media_interface *intf, *tmp_intf; > struct media_entity_notify *notify, *nextp; > > - if (mdev == NULL) > - return; > - > - mutex_lock(&mdev->graph_mutex); > - > - /* Check if mdev was ever registered at all */ > - if (!media_devnode_is_registered(&mdev->devnode)) { > - mutex_unlock(&mdev->graph_mutex); > - return; > - } > - > /* Remove all entities from the media device */ > list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > __media_device_unregister_entity(entity); > @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev) > kfree(intf); > } > > - mutex_unlock(&mdev->graph_mutex); > - > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + /* Check if mdev devnode was registered */ > + if (media_devnode_is_registered(&mdev->devnode)) { > + device_remove_file(&mdev->devnode.dev, &dev_attr_model); > + media_devnode_unregister(&mdev->devnode); > + } > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > + > +void media_device_unregister(struct media_device *mdev) > +{ > + if (mdev == NULL) > + return; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + mutex_unlock(&mdev->graph_mutex); > +} > EXPORT_SYMBOL_GPL(media_device_unregister); > > static void media_device_release_devres(struct device *dev, void *res) > { > } > > -struct media_device *media_device_get_devres(struct device *dev) > +static void do_media_device_unregister_devres(struct kref *kref) > { > + struct media_device_devres *mdev_devres; > struct media_device *mdev; > + int ret; > > - mdev = devres_find(dev, media_device_release_devres, NULL, NULL); > - if (mdev) > - return mdev; > + mdev_devres = container_of(kref, struct media_device_devres, kref); > > - mdev = devres_alloc(media_device_release_devres, > - sizeof(struct media_device), GFP_KERNEL); > - if (!mdev) > + if (!mdev_devres) > + return; > + > + mdev = &mdev_devres->mdev; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + __media_device_cleanup(mdev); > + mutex_unlock(&mdev->graph_mutex); > + mutex_destroy(&mdev->graph_mutex); > + > + ret = devres_destroy(mdev->dev, media_device_release_devres, > + NULL, NULL); > + pr_debug("%s: devres_destroy() returned %d\n", __func__, ret); > +} > + > +void media_device_unregister_devres(struct media_device *mdev) > +{ > + struct media_device_devres *mdev_devres; > + > + mdev_devres = container_of(mdev, struct media_device_devres, mdev); > + kref_put(&mdev_devres->kref, do_media_device_unregister_devres); > +} > +EXPORT_SYMBOL_GPL(media_device_unregister_devres); > + > +struct media_device *media_device_get_devres(struct device *dev) > +{ > + struct media_device_devres *mdev_devres, *ptr; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (mdev_devres) { > + kref_get(&mdev_devres->kref); > + return &mdev_devres->mdev; > + } > + > + mdev_devres = devres_alloc(media_device_release_devres, > + sizeof(struct media_device_devres), > + GFP_KERNEL); > + if (!mdev_devres) > return NULL; > - return devres_get(dev, mdev, NULL, NULL); > + > + ptr = devres_get(dev, mdev_devres, NULL, NULL); > + if (ptr) > + kref_init(&ptr->kref); > + else > + devres_free(mdev_devres); > + > + return &ptr->mdev; > } > EXPORT_SYMBOL_GPL(media_device_get_devres); > > struct media_device *media_device_find_devres(struct device *dev) > { > - return devres_find(dev, media_device_release_devres, NULL, NULL); > + struct media_device_devres *mdev_devres; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (!mdev_devres) > + return NULL; > + > + return &mdev_devres->mdev; > } > EXPORT_SYMBOL_GPL(media_device_find_devres); > > diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c > index 06da73f1ff22..060904ed8f20 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) > dev->media_dev->enable_source = NULL; > dev->media_dev->disable_source = NULL; > > - media_device_unregister(dev->media_dev); > - media_device_cleanup(dev->media_dev); > + media_device_unregister_devres(dev->media_dev); > dev->media_dev = NULL; > #endif > } > diff --git a/include/media/media-device.h b/include/media/media-device.h > index b21ef244ad3e..e59772ed8494 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -23,6 +23,7 @@ > #ifndef _MEDIA_DEVICE_H > #define _MEDIA_DEVICE_H > > +#include <linux/kref.h> > #include <linux/list.h> > #include <linux/mutex.h> > > @@ -382,6 +383,16 @@ struct media_device { > unsigned int notification); > }; > > +/** > + * struct media_device_devres - Media device device resource > + * @mdev: pointer to struct media_device > + * @kref: Object refcount > + */ > +struct media_device_devres { > + struct media_device mdev; > + struct kref kref; > +}; > + > /* We don't need to include pci.h or usb.h here */ > struct pci_dev; > struct usb_device; > @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct device *dev); > */ > struct media_device *media_device_find_devres(struct device *dev); > > +/** > + * media_device_unregister_devres) - Unregister media device allocated as > + * as device resource > + * > + * @dev: pointer to struct &device. > + * > + * Devices allocated via media_device_get_devres should be de-alocalted > + * and freed via this function. Callers should not call > + * media_device_unregister() nor media_device_cleanup() on devices > + * allocated via media_device_get_devres(). > + */ > +void media_device_unregister_devres(struct media_device *mdev); > + > /* Iterate over all entities. */ > #define media_device_for_each_entity(entity, mdev) \ > list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) > @@ -688,6 +712,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev) > return NULL; > } > > +static inline void media_device_unregister_devres(struct media_device *mdev) > +{ > +} > + > static inline void media_device_pci_init(struct media_device *mdev, > struct pci_dev *pci_dev, > char *name) > diff --git a/sound/usb/media.c b/sound/usb/media.c > index 93a50d01490c..f78955fd0d6e 100644 > --- a/sound/usb/media.c > +++ b/sound/usb/media.c > @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip) > media_snd_mixer_delete(chip); > > if (mdev) { > - if (media_devnode_is_registered(&mdev->devnode)) > - media_device_unregister(mdev); > + media_device_unregister_devres(mdev); > chip->media_dev = NULL; > } > } >
Hi Shuah, On Monday 21 Mar 2016 07:42:34 Shuah Khan wrote: > On 03/21/2016 05:10 AM, Laurent Pinchart wrote: > > On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: > >> Now that the media_device can be used by multiple drivers, > >> via devres, we need to be sure that it will be dropped only > >> when all drivers stop using it. > > > > I've discussed this with Shuah previously and I'm surprised to see that > > the problem hasn't been fixed : using devres for this purpose is just > > plain wrong. The empty media_device_release_devres() function should have > > given you a hint. > > > > What we need instead is a list of media devices indexed by struct device > > (for this use case) or by struct device_node (for DT use cases). It will > > both simplify the code and get rid of the devres abuse. > > > > Shuah, if I recall correctly you worked on implementing such a solution > > after our last discussion on the topic. Could you please update us on the > > status ? > It is work in progress. I have a working prototype for au0828 which is an > easier case. I am working on resolving a couple of issues to differentiate > media devices allocated using the global media device list vs. the ones > embedded in the driver data structures. We have many of those. > > I had to put this work on the back burner to get the au0882 and > snd-usb-audio wrapped up. I can get the RFC patches on top of the au0882 > and snd-usb-audio. We can discuss them at the upcoming media summit. > > > In the mean time, let's hold off on this patch, and merge a proper > > solution instead. > > I think Mauro's restructure helps us with such differentiation and it will > be easy enough to change out devres to get media get API. We have build too much technical debt already. I would really dislike seeing another hack being merged to fix partial problems when we know what needs to be done for a proper implementation. That's not the Linux upstream development I've known and grown to love, we don't pile up half-baked patches and hope that everything will be magically cleaned up later :-)
On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: > Now that the media_device can be used by multiple drivers, > via devres, we need to be sure that it will be dropped only > when all drivers stop using it. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > --- > > v2: The kref is now used only when media_device is allocated via > the media_device*_devress. This warrants that other drivers won't be > affected, and that we can keep media_device_cleanup() balanced with > media_device_init(). > > drivers/media/media-device.c | 117 ++++++++++++++++++++++-------- > drivers/media/usb/au0828/au0828-core.c | 3 +- > include/media/media-device.h | 28 ++++++++ > sound/usb/media.c | 3 +- > 4 files changed, 118 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index c32fa15cc76e..4a97d92a7e7d 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) > } > EXPORT_SYMBOL_GPL(media_device_init); > > -void media_device_cleanup(struct media_device *mdev) > +static void __media_device_cleanup(struct media_device *mdev) > { > ida_destroy(&mdev->entity_internal_idx); > mdev->entity_internal_idx_max = 0; > media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > +} > + > +void media_device_cleanup(struct media_device *mdev) > +{ > + __media_device_cleanup(mdev); > mutex_destroy(&mdev->graph_mutex); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct > media_device *mdev, { > int ret; > > + /* Check if mdev was ever registered at all */ This comment doesn't seem to apply to the next line, is it a leftover ? If so, please remove it. > + mutex_lock(&mdev->graph_mutex); > + > /* Register the device node. */ > mdev->devnode.fops = &media_device_fops; > mdev->devnode.parent = mdev->dev; > @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct > media_device *mdev, > > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - return ret; > + goto err; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > if (ret < 0) { > media_devnode_unregister(&mdev->devnode); > - return ret; > + goto err; > } > > dev_dbg(mdev->dev, "Media device registered\n"); > > - return 0; > +err: > + mutex_unlock(&mdev->graph_mutex); > + return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct > media_device *mdev, } > EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify); > > -void media_device_unregister(struct media_device *mdev) > +static void __media_device_unregister(struct media_device *mdev) > { > struct media_entity *entity; > struct media_entity *next; > struct media_interface *intf, *tmp_intf; > struct media_entity_notify *notify, *nextp; > > - if (mdev == NULL) > - return; > - > - mutex_lock(&mdev->graph_mutex); > - > - /* Check if mdev was ever registered at all */ > - if (!media_devnode_is_registered(&mdev->devnode)) { > - mutex_unlock(&mdev->graph_mutex); > - return; > - } > - > /* Remove all entities from the media device */ > list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > __media_device_unregister_entity(entity); > @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device > *mdev) kfree(intf); > } > > - mutex_unlock(&mdev->graph_mutex); > - > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + /* Check if mdev devnode was registered */ > + if (media_devnode_is_registered(&mdev->devnode)) { > + device_remove_file(&mdev->devnode.dev, &dev_attr_model); > + media_devnode_unregister(&mdev->devnode); > + } > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > + > +void media_device_unregister(struct media_device *mdev) > +{ > + if (mdev == NULL) > + return; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + mutex_unlock(&mdev->graph_mutex); > +} > EXPORT_SYMBOL_GPL(media_device_unregister); > > static void media_device_release_devres(struct device *dev, void *res) > { > } > > -struct media_device *media_device_get_devres(struct device *dev) > +static void do_media_device_unregister_devres(struct kref *kref) > { > + struct media_device_devres *mdev_devres; > struct media_device *mdev; > + int ret; > > - mdev = devres_find(dev, media_device_release_devres, NULL, NULL); > - if (mdev) > - return mdev; > + mdev_devres = container_of(kref, struct media_device_devres, kref); > > - mdev = devres_alloc(media_device_release_devres, > - sizeof(struct media_device), GFP_KERNEL); > - if (!mdev) > + if (!mdev_devres) > + return; > + > + mdev = &mdev_devres->mdev; > + > + mutex_lock(&mdev->graph_mutex); > + __media_device_unregister(mdev); > + __media_device_cleanup(mdev); > + mutex_unlock(&mdev->graph_mutex); > + mutex_destroy(&mdev->graph_mutex); > + > + ret = devres_destroy(mdev->dev, media_device_release_devres, > + NULL, NULL); > + pr_debug("%s: devres_destroy() returned %d\n", __func__, ret); > +} > + > +void media_device_unregister_devres(struct media_device *mdev) > +{ > + struct media_device_devres *mdev_devres; > + > + mdev_devres = container_of(mdev, struct media_device_devres, mdev); > + kref_put(&mdev_devres->kref, do_media_device_unregister_devres); > +} > +EXPORT_SYMBOL_GPL(media_device_unregister_devres); > + > +struct media_device *media_device_get_devres(struct device *dev) > +{ > + struct media_device_devres *mdev_devres, *ptr; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (mdev_devres) { > + kref_get(&mdev_devres->kref); > + return &mdev_devres->mdev; > + } > + > + mdev_devres = devres_alloc(media_device_release_devres, > + sizeof(struct media_device_devres), > + GFP_KERNEL); > + if (!mdev_devres) > return NULL; > - return devres_get(dev, mdev, NULL, NULL); > + > + ptr = devres_get(dev, mdev_devres, NULL, NULL); > + if (ptr) > + kref_init(&ptr->kref); > + else > + devres_free(mdev_devres); > + > + return &ptr->mdev; > } > EXPORT_SYMBOL_GPL(media_device_get_devres); > > struct media_device *media_device_find_devres(struct device *dev) > { > - return devres_find(dev, media_device_release_devres, NULL, NULL); > + struct media_device_devres *mdev_devres; > + > + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); > + if (!mdev_devres) > + return NULL; > + > + return &mdev_devres->mdev; > } > EXPORT_SYMBOL_GPL(media_device_find_devres); > > diff --git a/drivers/media/usb/au0828/au0828-core.c > b/drivers/media/usb/au0828/au0828-core.c index 06da73f1ff22..060904ed8f20 > 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct > au0828_dev *dev) dev->media_dev->enable_source = NULL; > dev->media_dev->disable_source = NULL; > > - media_device_unregister(dev->media_dev); > - media_device_cleanup(dev->media_dev); > + media_device_unregister_devres(dev->media_dev); > dev->media_dev = NULL; > #endif > } > diff --git a/include/media/media-device.h b/include/media/media-device.h > index b21ef244ad3e..e59772ed8494 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -23,6 +23,7 @@ > #ifndef _MEDIA_DEVICE_H > #define _MEDIA_DEVICE_H > > +#include <linux/kref.h> > #include <linux/list.h> > #include <linux/mutex.h> > > @@ -382,6 +383,16 @@ struct media_device { > unsigned int notification); > }; > > +/** > + * struct media_device_devres - Media device device resource > + * @mdev: pointer to struct media_device > + * @kref: Object refcount > + */ > +struct media_device_devres { > + struct media_device mdev; > + struct kref kref; > +}; > + > /* We don't need to include pci.h or usb.h here */ > struct pci_dev; > struct usb_device; > @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct > device *dev); */ > struct media_device *media_device_find_devres(struct device *dev); > > +/** > + * media_device_unregister_devres) - Unregister media device allocated as > + * as device resource > + * > + * @dev: pointer to struct &device. > + * > + * Devices allocated via media_device_get_devres should be de-alocalted > + * and freed via this function. Callers should not call > + * media_device_unregister() nor media_device_cleanup() on devices > + * allocated via media_device_get_devres(). > + */ > +void media_device_unregister_devres(struct media_device *mdev); > + > /* Iterate over all entities. */ > #define media_device_for_each_entity(entity, mdev) \ > list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) > @@ -688,6 +712,10 @@ static inline struct media_device > *media_device_find_devres(struct device *dev) return NULL; > } > > +static inline void media_device_unregister_devres(struct media_device > *mdev) > +{ > +} > + > static inline void media_device_pci_init(struct media_device *mdev, > struct pci_dev *pci_dev, > char *name) > diff --git a/sound/usb/media.c b/sound/usb/media.c > index 93a50d01490c..f78955fd0d6e 100644 > --- a/sound/usb/media.c > +++ b/sound/usb/media.c > @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip) > media_snd_mixer_delete(chip); > > if (mdev) { > - if (media_devnode_is_registered(&mdev->devnode)) > - media_device_unregister(mdev); > + media_device_unregister_devres(mdev); > chip->media_dev = NULL; > } > }
Hi Laurent, Thanks for reviewing it. Em Wed, 23 Mar 2016 18:57:50 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > On Friday 18 Mar 2016 21:42:16 Mauro Carvalho Chehab wrote: > > Now that the media_device can be used by multiple drivers, > > via devres, we need to be sure that it will be dropped only > > when all drivers stop using it. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > --- > > > > v2: The kref is now used only when media_device is allocated via > > the media_device*_devress. This warrants that other drivers won't be > > affected, and that we can keep media_device_cleanup() balanced with > > media_device_init(). > > > > drivers/media/media-device.c | 117 ++++++++++++++++++++++-------- > > drivers/media/usb/au0828/au0828-core.c | 3 +- > > include/media/media-device.h | 28 ++++++++ > > sound/usb/media.c | 3 +- > > 4 files changed, 118 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > index c32fa15cc76e..4a97d92a7e7d 100644 > > --- a/drivers/media/media-device.c > > +++ b/drivers/media/media-device.c > > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) > > } > > EXPORT_SYMBOL_GPL(media_device_init); > > > > -void media_device_cleanup(struct media_device *mdev) > > +static void __media_device_cleanup(struct media_device *mdev) > > { > > ida_destroy(&mdev->entity_internal_idx); > > mdev->entity_internal_idx_max = 0; > > media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > > +} > > + > > +void media_device_cleanup(struct media_device *mdev) > > +{ > > + __media_device_cleanup(mdev); > > mutex_destroy(&mdev->graph_mutex); > > } > > EXPORT_SYMBOL_GPL(media_device_cleanup); > > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct > > media_device *mdev, { > > int ret; > > > > + /* Check if mdev was ever registered at all */ > > This comment doesn't seem to apply to the next line, is it a leftover ? If so, > please remove it. Yes, it is a left over. I'll remove it. Thanks for noticing it. Thanks, 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/media-device.c b/drivers/media/media-device.c index c32fa15cc76e..4a97d92a7e7d 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev) } EXPORT_SYMBOL_GPL(media_device_init); -void media_device_cleanup(struct media_device *mdev) +static void __media_device_cleanup(struct media_device *mdev) { ida_destroy(&mdev->entity_internal_idx); mdev->entity_internal_idx_max = 0; media_entity_graph_walk_cleanup(&mdev->pm_count_walk); +} + +void media_device_cleanup(struct media_device *mdev) +{ + __media_device_cleanup(mdev); mutex_destroy(&mdev->graph_mutex); } EXPORT_SYMBOL_GPL(media_device_cleanup); @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct media_device *mdev, { int ret; + /* Check if mdev was ever registered at all */ + mutex_lock(&mdev->graph_mutex); + /* Register the device node. */ mdev->devnode.fops = &media_device_fops; mdev->devnode.parent = mdev->dev; @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct media_device *mdev, ret = media_devnode_register(&mdev->devnode, owner); if (ret < 0) - return ret; + goto err; ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); if (ret < 0) { media_devnode_unregister(&mdev->devnode); - return ret; + goto err; } dev_dbg(mdev->dev, "Media device registered\n"); - return 0; +err: + mutex_unlock(&mdev->graph_mutex); + return ret; } EXPORT_SYMBOL_GPL(__media_device_register); @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct media_device *mdev, } EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify); -void media_device_unregister(struct media_device *mdev) +static void __media_device_unregister(struct media_device *mdev) { struct media_entity *entity; struct media_entity *next; struct media_interface *intf, *tmp_intf; struct media_entity_notify *notify, *nextp; - if (mdev == NULL) - return; - - mutex_lock(&mdev->graph_mutex); - - /* Check if mdev was ever registered at all */ - if (!media_devnode_is_registered(&mdev->devnode)) { - mutex_unlock(&mdev->graph_mutex); - return; - } - /* Remove all entities from the media device */ list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) __media_device_unregister_entity(entity); @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev) kfree(intf); } - mutex_unlock(&mdev->graph_mutex); - - device_remove_file(&mdev->devnode.dev, &dev_attr_model); - media_devnode_unregister(&mdev->devnode); + /* Check if mdev devnode was registered */ + if (media_devnode_is_registered(&mdev->devnode)) { + device_remove_file(&mdev->devnode.dev, &dev_attr_model); + media_devnode_unregister(&mdev->devnode); + } dev_dbg(mdev->dev, "Media device unregistered\n"); } + +void media_device_unregister(struct media_device *mdev) +{ + if (mdev == NULL) + return; + + mutex_lock(&mdev->graph_mutex); + __media_device_unregister(mdev); + mutex_unlock(&mdev->graph_mutex); +} EXPORT_SYMBOL_GPL(media_device_unregister); static void media_device_release_devres(struct device *dev, void *res) { } -struct media_device *media_device_get_devres(struct device *dev) +static void do_media_device_unregister_devres(struct kref *kref) { + struct media_device_devres *mdev_devres; struct media_device *mdev; + int ret; - mdev = devres_find(dev, media_device_release_devres, NULL, NULL); - if (mdev) - return mdev; + mdev_devres = container_of(kref, struct media_device_devres, kref); - mdev = devres_alloc(media_device_release_devres, - sizeof(struct media_device), GFP_KERNEL); - if (!mdev) + if (!mdev_devres) + return; + + mdev = &mdev_devres->mdev; + + mutex_lock(&mdev->graph_mutex); + __media_device_unregister(mdev); + __media_device_cleanup(mdev); + mutex_unlock(&mdev->graph_mutex); + mutex_destroy(&mdev->graph_mutex); + + ret = devres_destroy(mdev->dev, media_device_release_devres, + NULL, NULL); + pr_debug("%s: devres_destroy() returned %d\n", __func__, ret); +} + +void media_device_unregister_devres(struct media_device *mdev) +{ + struct media_device_devres *mdev_devres; + + mdev_devres = container_of(mdev, struct media_device_devres, mdev); + kref_put(&mdev_devres->kref, do_media_device_unregister_devres); +} +EXPORT_SYMBOL_GPL(media_device_unregister_devres); + +struct media_device *media_device_get_devres(struct device *dev) +{ + struct media_device_devres *mdev_devres, *ptr; + + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); + if (mdev_devres) { + kref_get(&mdev_devres->kref); + return &mdev_devres->mdev; + } + + mdev_devres = devres_alloc(media_device_release_devres, + sizeof(struct media_device_devres), + GFP_KERNEL); + if (!mdev_devres) return NULL; - return devres_get(dev, mdev, NULL, NULL); + + ptr = devres_get(dev, mdev_devres, NULL, NULL); + if (ptr) + kref_init(&ptr->kref); + else + devres_free(mdev_devres); + + return &ptr->mdev; } EXPORT_SYMBOL_GPL(media_device_get_devres); struct media_device *media_device_find_devres(struct device *dev) { - return devres_find(dev, media_device_release_devres, NULL, NULL); + struct media_device_devres *mdev_devres; + + mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL); + if (!mdev_devres) + return NULL; + + return &mdev_devres->mdev; } EXPORT_SYMBOL_GPL(media_device_find_devres); diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 06da73f1ff22..060904ed8f20 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) dev->media_dev->enable_source = NULL; dev->media_dev->disable_source = NULL; - media_device_unregister(dev->media_dev); - media_device_cleanup(dev->media_dev); + media_device_unregister_devres(dev->media_dev); dev->media_dev = NULL; #endif } diff --git a/include/media/media-device.h b/include/media/media-device.h index b21ef244ad3e..e59772ed8494 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -23,6 +23,7 @@ #ifndef _MEDIA_DEVICE_H #define _MEDIA_DEVICE_H +#include <linux/kref.h> #include <linux/list.h> #include <linux/mutex.h> @@ -382,6 +383,16 @@ struct media_device { unsigned int notification); }; +/** + * struct media_device_devres - Media device device resource + * @mdev: pointer to struct media_device + * @kref: Object refcount + */ +struct media_device_devres { + struct media_device mdev; + struct kref kref; +}; + /* We don't need to include pci.h or usb.h here */ struct pci_dev; struct usb_device; @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct device *dev); */ struct media_device *media_device_find_devres(struct device *dev); +/** + * media_device_unregister_devres) - Unregister media device allocated as + * as device resource + * + * @dev: pointer to struct &device. + * + * Devices allocated via media_device_get_devres should be de-alocalted + * and freed via this function. Callers should not call + * media_device_unregister() nor media_device_cleanup() on devices + * allocated via media_device_get_devres(). + */ +void media_device_unregister_devres(struct media_device *mdev); + /* Iterate over all entities. */ #define media_device_for_each_entity(entity, mdev) \ list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) @@ -688,6 +712,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev) return NULL; } +static inline void media_device_unregister_devres(struct media_device *mdev) +{ +} + static inline void media_device_pci_init(struct media_device *mdev, struct pci_dev *pci_dev, char *name) diff --git a/sound/usb/media.c b/sound/usb/media.c index 93a50d01490c..f78955fd0d6e 100644 --- a/sound/usb/media.c +++ b/sound/usb/media.c @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip) media_snd_mixer_delete(chip); if (mdev) { - if (media_devnode_is_registered(&mdev->devnode)) - media_device_unregister(mdev); + media_device_unregister_devres(mdev); chip->media_dev = NULL; } }
Now that the media_device can be used by multiple drivers, via devres, we need to be sure that it will be dropped only when all drivers stop using it. Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> --- v2: The kref is now used only when media_device is allocated via the media_device*_devress. This warrants that other drivers won't be affected, and that we can keep media_device_cleanup() balanced with media_device_init(). drivers/media/media-device.c | 117 +++++++++++++++++++++++++-------- drivers/media/usb/au0828/au0828-core.c | 3 +- include/media/media-device.h | 28 ++++++++ sound/usb/media.c | 3 +- 4 files changed, 118 insertions(+), 33 deletions(-)