| Message ID | 54975937478803ef4883e9caecb8af0ef282e35c.1480384155.git.shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shuah, On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote: > Protect enable and disable source handler checks and calls from dvb-core > and v4l2-core. Hold graph_mutex to check if enable and disable source > handlers are present and invoke them while holding the mutex. This change > ensures these handlers will not be removed while they are being checked > and invoked. > > au08282 enable and disable source handlers are changed to not hold the > graph_mutex. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/dvb-core/dvb_frontend.c | 24 ++++++++++++++++++------ > drivers/media/usb/au0828/au0828-core.c | 17 +++++------------ > drivers/media/v4l2-core/v4l2-mc.c | 26 ++++++++++++++++++-------- > 3 files changed, 41 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > index 01511e5..2f09c7e 100644 > --- a/drivers/media/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb-core/dvb_frontend.c > @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > fepriv->voltage = -1; > > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) { > - ret = fe->dvb->mdev->enable_source(dvbdev->entity, > + if (fe->dvb->mdev) { > + mutex_lock(&fe->dvb->mdev->graph_mutex); > + if (fe->dvb->mdev->enable_source) > + ret = fe->dvb->mdev->enable_source( > + dvbdev->entity, > &fepriv->pipe); > + mutex_unlock(&fe->dvb->mdev->graph_mutex); You have to make sure the media device actually will stay aronud while it is being accessed. In this case, when dvb_frontend_open() runs, it will proceed to access the media device without knowing whether it's going to stay around or not. Without doing so, it may well be in the process of being removed by au0828_unregister_media_device() at the same time. The approach I took in my patchset was that the device that requires the media device will acquire a reference to it, this way the media device will stick around as long as other data structures have references to it. The current set did not yet implement this to dvb devices but I can add that. Then there's no even a need for the frontend driver to acquire the graph lock just to call the enable_source() callback. > if (ret) { > dev_err(fe->dvb->device, > "Tuner is busy. Error %d\n", ret); > @@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > > err3: > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > - if (fe->dvb->mdev && fe->dvb->mdev->disable_source) > - fe->dvb->mdev->disable_source(dvbdev->entity); > + if (fe->dvb->mdev) { > + mutex_lock(&fe->dvb->mdev->graph_mutex); > + if (fe->dvb->mdev->disable_source) > + fe->dvb->mdev->disable_source(dvbdev->entity); > + mutex_unlock(&fe->dvb->mdev->graph_mutex); > + } > err2: > #endif > dvb_generic_release(inode, file); > @@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file) > if (dvbdev->users == -1) { > wake_up(&fepriv->wait_queue); > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > - if (fe->dvb->mdev && fe->dvb->mdev->disable_source) > - fe->dvb->mdev->disable_source(dvbdev->entity); > + if (fe->dvb->mdev) { > + mutex_lock(&fe->dvb->mdev->graph_mutex); > + if (fe->dvb->mdev->disable_source) > + fe->dvb->mdev->disable_source(dvbdev->entity); > + mutex_unlock(&fe->dvb->mdev->graph_mutex); > + } > #endif > if (fe->exit != DVB_FE_NO_EXIT) > wake_up(&dvbdev->wait_queue); > diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c > index a1f696a..bfd6482 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity *new, > } > } > > +/* Callers should hold graph_mutex */ > static int au0828_enable_source(struct media_entity *entity, > struct media_pipeline *pipe) > { > @@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity *entity, > if (!mdev) > return -ENODEV; > > - mutex_lock(&mdev->graph_mutex); > - > dev = mdev->source_priv; > > /* > @@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity *entity, > dev->active_source->name, dev->active_sink->name, > dev->active_link_owner->name, ret); > end: > - mutex_unlock(&mdev->graph_mutex); > pr_debug("au0828_enable_source() end %s %d %d\n", > entity->name, entity->function, ret); > return ret; > } > > +/* Callers should hold graph_mutex */ > static void au0828_disable_source(struct media_entity *entity) > { > int ret = 0; > @@ -436,13 +435,10 @@ static void au0828_disable_source(struct media_entity *entity) > if (!mdev) > return; > > - mutex_lock(&mdev->graph_mutex); > dev = mdev->source_priv; > > - if (!dev->active_link) { > - ret = -ENODEV; > - goto end; > - } > + if (!dev->active_link) > + return; > > /* link is active - stop pipeline from source (tuner) */ > if (dev->active_link->sink->entity == dev->active_sink && > @@ -452,7 +448,7 @@ static void au0828_disable_source(struct media_entity *entity) > * has active pipeline > */ > if (dev->active_link_owner != entity) > - goto end; > + return; > __media_entity_pipeline_stop(entity); > ret = __media_entity_setup_link(dev->active_link, 0); > if (ret) > @@ -467,9 +463,6 @@ static void au0828_disable_source(struct media_entity *entity) > dev->active_source = NULL; > dev->active_sink = NULL; > } > - > -end: > - mutex_unlock(&mdev->graph_mutex); > } > #endif > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c > index 8bef433..b169d24 100644 > --- a/drivers/media/v4l2-core/v4l2-mc.c > +++ b/drivers/media/v4l2-core/v4l2-mc.c > @@ -198,14 +198,20 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph); > int v4l_enable_media_source(struct video_device *vdev) > { > struct media_device *mdev = vdev->entity.graph_obj.mdev; > - int ret; > + int ret = 0, err; > > - if (!mdev || !mdev->enable_source) > + if (!mdev) > return 0; > - ret = mdev->enable_source(&vdev->entity, &vdev->pipe); > - if (ret) > - return -EBUSY; > - return 0; > + > + mutex_lock(&mdev->graph_mutex); > + if (!mdev->enable_source) > + goto end; > + err = mdev->enable_source(&vdev->entity, &vdev->pipe); > + if (err) > + ret = -EBUSY; > +end: > + mutex_unlock(&mdev->graph_mutex); > + return ret; > } > EXPORT_SYMBOL_GPL(v4l_enable_media_source); > > @@ -213,8 +219,12 @@ void v4l_disable_media_source(struct video_device *vdev) > { > struct media_device *mdev = vdev->entity.graph_obj.mdev; > > - if (mdev && mdev->disable_source) > - mdev->disable_source(&vdev->entity); > + if (mdev) { > + mutex_lock(&mdev->graph_mutex); > + if (mdev->disable_source) > + mdev->disable_source(&vdev->entity); > + mutex_unlock(&mdev->graph_mutex); > + } > } > EXPORT_SYMBOL_GPL(v4l_disable_media_source); >
On 11/29/2016 02:22 AM, Sakari Ailus wrote: > Hi Shuah, > > On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote: >> Protect enable and disable source handler checks and calls from dvb-core >> and v4l2-core. Hold graph_mutex to check if enable and disable source >> handlers are present and invoke them while holding the mutex. This change >> ensures these handlers will not be removed while they are being checked >> and invoked. >> >> au08282 enable and disable source handlers are changed to not hold the >> graph_mutex. >> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/media/dvb-core/dvb_frontend.c | 24 ++++++++++++++++++------ >> drivers/media/usb/au0828/au0828-core.c | 17 +++++------------ >> drivers/media/v4l2-core/v4l2-mc.c | 26 ++++++++++++++++++-------- >> 3 files changed, 41 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c >> index 01511e5..2f09c7e 100644 >> --- a/drivers/media/dvb-core/dvb_frontend.c >> +++ b/drivers/media/dvb-core/dvb_frontend.c >> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) >> fepriv->voltage = -1; >> >> #ifdef CONFIG_MEDIA_CONTROLLER_DVB >> - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) { >> - ret = fe->dvb->mdev->enable_source(dvbdev->entity, >> + if (fe->dvb->mdev) { >> + mutex_lock(&fe->dvb->mdev->graph_mutex); >> + if (fe->dvb->mdev->enable_source) >> + ret = fe->dvb->mdev->enable_source( >> + dvbdev->entity, >> &fepriv->pipe); >> + mutex_unlock(&fe->dvb->mdev->graph_mutex); > > You have to make sure the media device actually will stay aronud while it is > being accessed. In this case, when dvb_frontend_open() runs, it will proceed > to access the media device without knowing whether it's going to stay around > or not. Without doing so, it may well be in the process of being removed by > au0828_unregister_media_device() at the same time. Right. What this is trying to protect is just the check for enable_source and disable handlers before calling them. > > The approach I took in my patchset was that the device that requires the > media device will acquire a reference to it, this way the media device will > stick around as long as other data structures have references to it. The > current set did not yet implement this to dvb devices but I can add that. > Then there's no even a need for the frontend driver to acquire the graph > lock just to call the enable_source() callback. Taking reference to media_device alone will not solve this problem of enable and disable handlers going away. au0828_unregister_media_device() will clear the handlers and then call media_device_unregister() and it also does media_device_cleanup(). Your patch set and media dev allocator api I did solve the problem of media_device not going away, however, it doesn't fix this race where callers of enable and disable source handlers checking for them and calling them while the driver might be clearing them. So here is the scenario these patches fix. Say user app starts and during start of video streaming v4l2 checks to see if enable source handler is defined. This check is done without holding the graph_mutex. If unbind happens to be in progress, au0828 could clear enable and disable source handlers. So these could race. I am not how large this window is, but could happen. If graph_mutex protects the check for enable source handler not being null, then it has to be released before calling enable source handler as shown below: if (mdev) { mutex_lock(&mdev->graph_mutex); if (mdev->disable_source) { mutex_unlock(&mdev->graph_mutex); mdev->disable_source(&vdev->entity); } else mutex_unlock(&mdev->graph_mutex); } The above will leave another window for handlers to be cleared. That is why it would make sense for the caller to hold the lock and the call enable and disable source handlers. We do need a way to protect enable and disable handler access and the call itself. I am using the same graph_mutex for both, hence I decided to have the caller hold the lock. Hope this helps. thanks, -- Shuah -- 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
Hi Shuah, On Tue, Nov 29, 2016 at 10:41:51AM -0700, Shuah Khan wrote: > On 11/29/2016 02:22 AM, Sakari Ailus wrote: > > Hi Shuah, > > > > On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote: > >> Protect enable and disable source handler checks and calls from dvb-core > >> and v4l2-core. Hold graph_mutex to check if enable and disable source > >> handlers are present and invoke them while holding the mutex. This change > >> ensures these handlers will not be removed while they are being checked > >> and invoked. > >> > >> au08282 enable and disable source handlers are changed to not hold the > >> graph_mutex. > >> > >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > >> --- > >> drivers/media/dvb-core/dvb_frontend.c | 24 ++++++++++++++++++------ > >> drivers/media/usb/au0828/au0828-core.c | 17 +++++------------ > >> drivers/media/v4l2-core/v4l2-mc.c | 26 ++++++++++++++++++-------- > >> 3 files changed, 41 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > >> index 01511e5..2f09c7e 100644 > >> --- a/drivers/media/dvb-core/dvb_frontend.c > >> +++ b/drivers/media/dvb-core/dvb_frontend.c > >> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > >> fepriv->voltage = -1; > >> > >> #ifdef CONFIG_MEDIA_CONTROLLER_DVB > >> - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) { > >> - ret = fe->dvb->mdev->enable_source(dvbdev->entity, > >> + if (fe->dvb->mdev) { > >> + mutex_lock(&fe->dvb->mdev->graph_mutex); > >> + if (fe->dvb->mdev->enable_source) > >> + ret = fe->dvb->mdev->enable_source( > >> + dvbdev->entity, > >> &fepriv->pipe); > >> + mutex_unlock(&fe->dvb->mdev->graph_mutex); > > > > You have to make sure the media device actually will stay aronud while it is > > being accessed. In this case, when dvb_frontend_open() runs, it will proceed > > to access the media device without knowing whether it's going to stay around > > or not. Without doing so, it may well be in the process of being removed by > > au0828_unregister_media_device() at the same time. > > Right. What this is trying to protect is just the check for enable_source > and disable handlers before calling them. Yes, but that's not enough. The other handlers in the ops structure must stay there as long as the media device does. So we need to make sure it does. One, perhaps the only way to do that could be to obtain a reference to the device that first set those callbacks. > > > > > The approach I took in my patchset was that the device that requires the > > media device will acquire a reference to it, this way the media device will > > stick around as long as other data structures have references to it. The > > current set did not yet implement this to dvb devices but I can add that. > > Then there's no even a need for the frontend driver to acquire the graph > > lock just to call the enable_source() callback. > > Taking reference to media_device alone will not solve this problem of enable > and disable handlers going away. au0828_unregister_media_device() will clear > the handlers and then call media_device_unregister() and it also does > media_device_cleanup(). Your patch set and media dev allocator api I did solve Then, that should be applied to all the other callbacks in the ops structure as well. Not only to the callbacks that the au0828 driver needs. All the callbacks are really need to stay unchanged as long as the device may be in use. Acquiring the graph mutex is hardly a workable solution to fix this. > the problem of media_device not going away, however, it doesn't fix this race > where callers of enable and disable source handlers checking for them and calling > them while the driver might be clearing them. > > So here is the scenario these patches fix. Say user app starts > and during start of video streaming v4l2 checks to see if enable > source handler is defined. This check is done without holding the > graph_mutex. If unbind happens to be in progress, au0828 could > clear enable and disable source handlers. So these could race. > I am not how large this window is, but could happen. > > If graph_mutex protects the check for enable source handler not > being null, then it has to be released before calling enable source > handler as shown below: > > if (mdev) { > mutex_lock(&mdev->graph_mutex); > if (mdev->disable_source) { > mutex_unlock(&mdev->graph_mutex); > mdev->disable_source(&vdev->entity); > } else > mutex_unlock(&mdev->graph_mutex); > } > > The above will leave another window for handlers to be cleared. > That is why it would make sense for the caller to hold the lock > and the call enable and disable source handlers. > > We do need a way to protect enable and disable handler access and the > call itself. I am using the same graph_mutex for both, hence I decided > to have the caller hold the lock. > > Hope this helps.
Hi Sakari, On 12/01/2016 06:51 AM, Sakari Ailus wrote: > Hi Shuah, > > On Tue, Nov 29, 2016 at 10:41:51AM -0700, Shuah Khan wrote: >> On 11/29/2016 02:22 AM, Sakari Ailus wrote: >>> Hi Shuah, >>> >>> On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote: >>>> Protect enable and disable source handler checks and calls from dvb-core >>>> and v4l2-core. Hold graph_mutex to check if enable and disable source >>>> handlers are present and invoke them while holding the mutex. This change >>>> ensures these handlers will not be removed while they are being checked >>>> and invoked. >>>> >>>> au08282 enable and disable source handlers are changed to not hold the >>>> graph_mutex. >>>> >>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>>> --- >>>> drivers/media/dvb-core/dvb_frontend.c | 24 ++++++++++++++++++------ >>>> drivers/media/usb/au0828/au0828-core.c | 17 +++++------------ >>>> drivers/media/v4l2-core/v4l2-mc.c | 26 ++++++++++++++++++-------- >>>> 3 files changed, 41 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c >>>> index 01511e5..2f09c7e 100644 >>>> --- a/drivers/media/dvb-core/dvb_frontend.c >>>> +++ b/drivers/media/dvb-core/dvb_frontend.c >>>> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) >>>> fepriv->voltage = -1; >>>> >>>> #ifdef CONFIG_MEDIA_CONTROLLER_DVB >>>> - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) { >>>> - ret = fe->dvb->mdev->enable_source(dvbdev->entity, >>>> + if (fe->dvb->mdev) { >>>> + mutex_lock(&fe->dvb->mdev->graph_mutex); >>>> + if (fe->dvb->mdev->enable_source) >>>> + ret = fe->dvb->mdev->enable_source( >>>> + dvbdev->entity, >>>> &fepriv->pipe); >>>> + mutex_unlock(&fe->dvb->mdev->graph_mutex); >>> >>> You have to make sure the media device actually will stay aronud while it is >>> being accessed. In this case, when dvb_frontend_open() runs, it will proceed >>> to access the media device without knowing whether it's going to stay around >>> or not. Without doing so, it may well be in the process of being removed by >>> au0828_unregister_media_device() at the same time. >> >> Right. What this is trying to protect is just the check for enable_source >> and disable handlers before calling them. > > Yes, but that's not enough. > > The other handlers in the ops structure must stay there as long as the media > device does. So we need to make sure it does. One, perhaps the only way to > do that could be to obtain a reference to the device that first set those > callbacks. > >> >>> >>> The approach I took in my patchset was that the device that requires the >>> media device will acquire a reference to it, this way the media device will >>> stick around as long as other data structures have references to it. The >>> current set did not yet implement this to dvb devices but I can add that. >>> Then there's no even a need for the frontend driver to acquire the graph >>> lock just to call the enable_source() callback. >> >> Taking reference to media_device alone will not solve this problem of enable >> and disable handlers going away. au0828_unregister_media_device() will clear >> the handlers and then call media_device_unregister() and it also does >> media_device_cleanup(). Your patch set and media dev allocator api I did solve > > Then, that should be applied to all the other callbacks in the ops structure > as well. Not only to the callbacks that the au0828 driver needs. All the > callbacks are really need to stay unchanged as long as the device may be in > use. I agree with you that media_device should stick around until all the users go away. That is what the Media Dev Allocator API patch series does. When the media_device shared across two drivers, We are looking at two different lifetimes. media_device lufetimes (starts when the first driver creates it) These handlers (enable_source and disable_source) are tied to au02828 driver (bridge driver) lifetime, not the media_device lifetime. Hence, it is important for au0828 to clear them from the media_device when au0828 is going away via unbind, so these become invalid and don't get run. This similar to when the driver (au0828) goes away, it removes its graph nodes. Hence, it makes sense to use graph_mutex for this part of cleanup just like when a media graph node is deleted and/or added. I think you might be thinking about a simpler scenario where media_device lifetime is same as the driver lifetime + until the last app. released media_device reference. When you have two drivers ion mix, we also have lifetimes for graph nodes each driver owns as well as some handlers bridge driver provides to manage the graph nodes. Here is a visual: bridge driver: creates media_device (gets reference to it when it allocates) registers it with its graph adds enable_source and disable_source handlers second driver: finds media_device and gets reference to it adds its graph bridge driver and second driver share access to resources using enable_source and disable_source handlers bridge driver unbind: clears enable_source and disable_source handlers - when this driver unbinds, enable_source and disable_source handler lifetime ends gives up media_device reference second driver unbind: gives up media_device reference and no media_device gets unregistered and released if there are no other holds such as an application keeping media device open. In summary we have media_device lifetime itself and the lifetime of individual graph nodes and ops provided by one of the drivers. This patch and the media dev allocator patch series I sent out handle all of the above cases as tested on 4.9-rc7 base. thanks, -- Shuah > > Acquiring the graph mutex is hardly a workable solution to fix this. > >> the problem of media_device not going away, however, it doesn't fix this race >> where callers of enable and disable source handlers checking for them and calling >> them while the driver might be clearing them. >> >> So here is the scenario these patches fix. Say user app starts >> and during start of video streaming v4l2 checks to see if enable >> source handler is defined. This check is done without holding the >> graph_mutex. If unbind happens to be in progress, au0828 could >> clear enable and disable source handlers. So these could race. >> I am not how large this window is, but could happen. >> >> If graph_mutex protects the check for enable source handler not >> being null, then it has to be released before calling enable source >> handler as shown below: >> >> if (mdev) { >> mutex_lock(&mdev->graph_mutex); >> if (mdev->disable_source) { >> mutex_unlock(&mdev->graph_mutex); >> mdev->disable_source(&vdev->entity); >> } else >> mutex_unlock(&mdev->graph_mutex); >> } >> >> The above will leave another window for handlers to be cleared. >> That is why it would make sense for the caller to hold the lock >> and the call enable and disable source handlers. >> >> We do need a way to protect enable and disable handler access and the >> call itself. I am using the same graph_mutex for both, hence I decided >> to have the caller hold the lock. >> >> Hope this helps. > -- 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/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 01511e5..2f09c7e 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) fepriv->voltage = -1; #ifdef CONFIG_MEDIA_CONTROLLER_DVB - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) { - ret = fe->dvb->mdev->enable_source(dvbdev->entity, + if (fe->dvb->mdev) { + mutex_lock(&fe->dvb->mdev->graph_mutex); + if (fe->dvb->mdev->enable_source) + ret = fe->dvb->mdev->enable_source( + dvbdev->entity, &fepriv->pipe); + mutex_unlock(&fe->dvb->mdev->graph_mutex); if (ret) { dev_err(fe->dvb->device, "Tuner is busy. Error %d\n", ret); @@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) err3: #ifdef CONFIG_MEDIA_CONTROLLER_DVB - if (fe->dvb->mdev && fe->dvb->mdev->disable_source) - fe->dvb->mdev->disable_source(dvbdev->entity); + if (fe->dvb->mdev) { + mutex_lock(&fe->dvb->mdev->graph_mutex); + if (fe->dvb->mdev->disable_source) + fe->dvb->mdev->disable_source(dvbdev->entity); + mutex_unlock(&fe->dvb->mdev->graph_mutex); + } err2: #endif dvb_generic_release(inode, file); @@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file) if (dvbdev->users == -1) { wake_up(&fepriv->wait_queue); #ifdef CONFIG_MEDIA_CONTROLLER_DVB - if (fe->dvb->mdev && fe->dvb->mdev->disable_source) - fe->dvb->mdev->disable_source(dvbdev->entity); + if (fe->dvb->mdev) { + mutex_lock(&fe->dvb->mdev->graph_mutex); + if (fe->dvb->mdev->disable_source) + fe->dvb->mdev->disable_source(dvbdev->entity); + mutex_unlock(&fe->dvb->mdev->graph_mutex); + } #endif if (fe->exit != DVB_FE_NO_EXIT) wake_up(&dvbdev->wait_queue); diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index a1f696a..bfd6482 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity *new, } } +/* Callers should hold graph_mutex */ static int au0828_enable_source(struct media_entity *entity, struct media_pipeline *pipe) { @@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity *entity, if (!mdev) return -ENODEV; - mutex_lock(&mdev->graph_mutex); - dev = mdev->source_priv; /* @@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity *entity, dev->active_source->name, dev->active_sink->name, dev->active_link_owner->name, ret); end: - mutex_unlock(&mdev->graph_mutex); pr_debug("au0828_enable_source() end %s %d %d\n", entity->name, entity->function, ret); return ret; } +/* Callers should hold graph_mutex */ static void au0828_disable_source(struct media_entity *entity) { int ret = 0; @@ -436,13 +435,10 @@ static void au0828_disable_source(struct media_entity *entity) if (!mdev) return; - mutex_lock(&mdev->graph_mutex); dev = mdev->source_priv; - if (!dev->active_link) { - ret = -ENODEV; - goto end; - } + if (!dev->active_link) + return; /* link is active - stop pipeline from source (tuner) */ if (dev->active_link->sink->entity == dev->active_sink && @@ -452,7 +448,7 @@ static void au0828_disable_source(struct media_entity *entity) * has active pipeline */ if (dev->active_link_owner != entity) - goto end; + return; __media_entity_pipeline_stop(entity); ret = __media_entity_setup_link(dev->active_link, 0); if (ret) @@ -467,9 +463,6 @@ static void au0828_disable_source(struct media_entity *entity) dev->active_source = NULL; dev->active_sink = NULL; } - -end: - mutex_unlock(&mdev->graph_mutex); } #endif diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c index 8bef433..b169d24 100644 --- a/drivers/media/v4l2-core/v4l2-mc.c +++ b/drivers/media/v4l2-core/v4l2-mc.c @@ -198,14 +198,20 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph); int v4l_enable_media_source(struct video_device *vdev) { struct media_device *mdev = vdev->entity.graph_obj.mdev; - int ret; + int ret = 0, err; - if (!mdev || !mdev->enable_source) + if (!mdev) return 0; - ret = mdev->enable_source(&vdev->entity, &vdev->pipe); - if (ret) - return -EBUSY; - return 0; + + mutex_lock(&mdev->graph_mutex); + if (!mdev->enable_source) + goto end; + err = mdev->enable_source(&vdev->entity, &vdev->pipe); + if (err) + ret = -EBUSY; +end: + mutex_unlock(&mdev->graph_mutex); + return ret; } EXPORT_SYMBOL_GPL(v4l_enable_media_source); @@ -213,8 +219,12 @@ void v4l_disable_media_source(struct video_device *vdev) { struct media_device *mdev = vdev->entity.graph_obj.mdev; - if (mdev && mdev->disable_source) - mdev->disable_source(&vdev->entity); + if (mdev) { + mutex_lock(&mdev->graph_mutex); + if (mdev->disable_source) + mdev->disable_source(&vdev->entity); + mutex_unlock(&mdev->graph_mutex); + } } EXPORT_SYMBOL_GPL(v4l_disable_media_source);
Protect enable and disable source handler checks and calls from dvb-core and v4l2-core. Hold graph_mutex to check if enable and disable source handlers are present and invoke them while holding the mutex. This change ensures these handlers will not be removed while they are being checked and invoked. au08282 enable and disable source handlers are changed to not hold the graph_mutex. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/dvb-core/dvb_frontend.c | 24 ++++++++++++++++++------ drivers/media/usb/au0828/au0828-core.c | 17 +++++------------ drivers/media/v4l2-core/v4l2-mc.c | 26 ++++++++++++++++++-------- 3 files changed, 41 insertions(+), 26 deletions(-)