Re: [PATCH v2] [media] media-device: use kref for media_device instance
Hi Laurent,
Thanks for reviewing it.
Em Wed, 23 Mar 2016 18:57:50 +0200
Laurent Pinchart 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
> > ---
> >
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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
> ---
>
> 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)
> -
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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 :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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
> ---
>
> 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
Tested-by: Shuah Khan
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)
> {
> }
>
Re: [PATCH v2] [media] media-device: use kref for media_device instance
On 03/21/2016 05:58 AM, Mauro Carvalho Chehab wrote: > Em Mon, 21 Mar 2016 13:10:33 +0200 > Laurent Pinchart 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 -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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 -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
Em Mon, 21 Mar 2016 13:10:33 +0200 Laurent Pinchart 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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
> ---
>
> 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;
Re: [PATCH v2] [media] media-device: use kref for media_device instance
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
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
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;
>
