Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320874 > unrolled thread

Re: [PATCH 29/31] media: track media device unregister in progress

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 29/31] media: track media device unregister in progress Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-01-28 18:10 +0100
    Re: [PATCH 29/31] media: track media device unregister in progress Shuah Khan <shuahkh@osg.samsung.com> - 2016-01-28 18:10 +0100
      Re: [PATCH 29/31] media: track media device unregister in progress Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-01-28 18:30 +0100
        Re: [PATCH 29/31] media: track media device unregister in progress Shuah Khan <shuahkh@osg.samsung.com> - 2016-01-28 21:50 +0100
          Re: [PATCH 29/31] media: track media device unregister in progress Shuah Khan <shuahkh@osg.samsung.com> - 2016-02-03 20:40 +0100

#1320874 — Re: [PATCH 29/31] media: track media device unregister in progress

Em Wed,  6 Jan 2016 13:27:18 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Add support to track media device unregister in progress
> state to prevent more than one driver entering unregister.
> This enables fixing the general protection faults while
> snd-usb-audio was cleaning up media resources for pcm
> streams and mixers. In this patch a new interface is added
> to return the unregister in progress state. Subsequent
> patches to snd-usb-audio and au0828-core use this interface
> to avoid entering unregister and attempting to unregister
> entities and remove devnodes while unregister is in progress.
> Media device unregister removes entities and interface nodes.

Hmm... isn't the spinlock enough to serialize it? It seems weird the
need of an extra bool here to warrant that this is really serialized.

> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/media-device.c |  5 ++++-
>  include/media/media-device.h | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 20c85a9..1bb9a5f 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -749,10 +749,13 @@ void media_device_unregister(struct media_device *mdev)
>  	spin_lock(&mdev->lock);
>  
>  	/* Check if mdev was ever registered at all */
> -	if (!media_devnode_is_registered(&mdev->devnode)) {
> +	/* check if unregister is in progress */
> +	if (!media_devnode_is_registered(&mdev->devnode) ||
> +	    mdev->unregister_in_progress) {
>  		spin_unlock(&mdev->lock);
>  		return;
>  	}
> +	mdev->unregister_in_progress = true;
>  
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 04b6c2e..0807292 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -332,6 +332,10 @@ struct media_device {
>  	spinlock_t lock;
>  	/* Serializes graph operations. */
>  	struct mutex graph_mutex;
> +	/* Tracks unregister in progress state to prevent
> +	 * more than one driver entering unregister
> +	*/
> +	bool unregister_in_progress;
>  
>  	/* Handlers to find source entity for the sink entity and
>  	 * check if it is available, and activate the link using
> @@ -365,6 +369,7 @@ struct media_device {
>  /* media_devnode to media_device */
>  #define to_media_device(node) container_of(node, struct media_device, devnode)
>  
> +
>  /**
>   * media_entity_enum_init - Initialise an entity enumeration
>   *
> @@ -553,6 +558,12 @@ struct media_device *media_device_get_devres(struct device *dev);
>   * @dev: pointer to struct &device.
>   */
>  struct media_device *media_device_find_devres(struct device *dev);
> +/* return unregister in progress state */
> +static inline bool media_device_is_unregister_in_progress(
> +					struct media_device *mdev)
> +{
> +	return mdev->unregister_in_progress;
> +}
>  
>  /* Iterate over all entities. */
>  #define media_device_for_each_entity(entity, mdev)			\
> @@ -569,6 +580,7 @@ struct media_device *media_device_find_devres(struct device *dev);
>  /* Iterate over all links. */
>  #define media_device_for_each_link(link, mdev)			\
>  	list_for_each_entry(link, &(mdev)->links, graph_obj.list)
> +
>  #else
>  static inline int media_device_register(struct media_device *mdev)
>  {
> @@ -604,5 +616,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev)
>  {
>  	return NULL;
>  }
> +static inline bool media_device_is_unregister_in_progress(
> +					struct media_device *mdev)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  #endif

[toc] | [next] | [standalone]

#1320883

On 01/28/2016 10:01 AM, Mauro Carvalho Chehab wrote:
> Em Wed,  6 Jan 2016 13:27:18 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Add support to track media device unregister in progress
>> state to prevent more than one driver entering unregister.
>> This enables fixing the general protection faults while
>> snd-usb-audio was cleaning up media resources for pcm
>> streams and mixers. In this patch a new interface is added
>> to return the unregister in progress state. Subsequent
>> patches to snd-usb-audio and au0828-core use this interface
>> to avoid entering unregister and attempting to unregister
>> entities and remove devnodes while unregister is in progress.
>> Media device unregister removes entities and interface nodes.
> 
> Hmm... isn't the spinlock enough to serialize it? It seems weird the
> need of an extra bool here to warrant that this is really serialized.
> 

The spinlock and check for media_devnode_is_registered(&mdev->devnode)
aren't enough to ensure only one driver enters the unregister. Please
note that the devnode isn't marked unregistered until the end in
media_device_unregister().


>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/media-device.c |  5 ++++-
>>  include/media/media-device.h | 17 +++++++++++++++++
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 20c85a9..1bb9a5f 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -749,10 +749,13 @@ void media_device_unregister(struct media_device *mdev)
>>  	spin_lock(&mdev->lock);
>>  
>>  	/* Check if mdev was ever registered at all */
>> -	if (!media_devnode_is_registered(&mdev->devnode)) {
>> +	/* check if unregister is in progress */
>> +	if (!media_devnode_is_registered(&mdev->devnode) ||
>> +	    mdev->unregister_in_progress) {
>>  		spin_unlock(&mdev->lock);
>>  		return;
>>  	}
>> +	mdev->unregister_in_progress = true;
>>  
>>  	/* Remove all entities from the media device */
>>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index 04b6c2e..0807292 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -332,6 +332,10 @@ struct media_device {
>>  	spinlock_t lock;
>>  	/* Serializes graph operations. */
>>  	struct mutex graph_mutex;
>> +	/* Tracks unregister in progress state to prevent
>> +	 * more than one driver entering unregister
>> +	*/
>> +	bool unregister_in_progress;
>>  
>>  	/* Handlers to find source entity for the sink entity and
>>  	 * check if it is available, and activate the link using
>> @@ -365,6 +369,7 @@ struct media_device {
>>  /* media_devnode to media_device */
>>  #define to_media_device(node) container_of(node, struct media_device, devnode)
>>  
>> +
>>  /**
>>   * media_entity_enum_init - Initialise an entity enumeration
>>   *
>> @@ -553,6 +558,12 @@ struct media_device *media_device_get_devres(struct device *dev);
>>   * @dev: pointer to struct &device.
>>   */
>>  struct media_device *media_device_find_devres(struct device *dev);
>> +/* return unregister in progress state */
>> +static inline bool media_device_is_unregister_in_progress(
>> +					struct media_device *mdev)
>> +{
>> +	return mdev->unregister_in_progress;
>> +}
>>  
>>  /* Iterate over all entities. */
>>  #define media_device_for_each_entity(entity, mdev)			\
>> @@ -569,6 +580,7 @@ struct media_device *media_device_find_devres(struct device *dev);
>>  /* Iterate over all links. */
>>  #define media_device_for_each_link(link, mdev)			\
>>  	list_for_each_entry(link, &(mdev)->links, graph_obj.list)
>> +
>>  #else
>>  static inline int media_device_register(struct media_device *mdev)
>>  {
>> @@ -604,5 +616,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev)
>>  {
>>  	return NULL;
>>  }
>> +static inline bool media_device_is_unregister_in_progress(
>> +					struct media_device *mdev)
>> +{
>> +	return false;
>> +}
>>  #endif /* CONFIG_MEDIA_CONTROLLER */
>>  #endif


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[toc] | [prev] | [next] | [standalone]

#1320901

Em Thu, 28 Jan 2016 10:04:24 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 01/28/2016 10:01 AM, Mauro Carvalho Chehab wrote:
> > Em Wed,  6 Jan 2016 13:27:18 -0700
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >   
> >> Add support to track media device unregister in progress
> >> state to prevent more than one driver entering unregister.
> >> This enables fixing the general protection faults while
> >> snd-usb-audio was cleaning up media resources for pcm
> >> streams and mixers. In this patch a new interface is added
> >> to return the unregister in progress state. Subsequent
> >> patches to snd-usb-audio and au0828-core use this interface
> >> to avoid entering unregister and attempting to unregister
> >> entities and remove devnodes while unregister is in progress.
> >> Media device unregister removes entities and interface nodes.  
> > 
> > Hmm... isn't the spinlock enough to serialize it? It seems weird the
> > need of an extra bool here to warrant that this is really serialized.
> >   
> 
> The spinlock and check for media_devnode_is_registered(&mdev->devnode)
> aren't enough to ensure only one driver enters the unregister. 
>
> Please
> note that the devnode isn't marked unregistered until the end in
> media_device_unregister().

I guess the call to:
	device_remove_file(&mdev->devnode.dev, &dev_attr_model);

IMO, This should be, instead, at media_devnode_unregister().

Then, we can change the logic at media_devnode_unregister() to:

void media_devnode_unregister(struct media_devnode *mdev)
{
	mutex_lock(&media_devnode_lock);

	/* Check if mdev was ever registered at all */
	if (!media_devnode_is_registered(mdev)) {
		mutex_unlock(&media_devnode_lock);
		return;
	}

	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
	mutex_unlock(&media_devnode_lock);
	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
	device_unregister(&mdev->dev);
}

This sounds enough to avoid device_unregister() or device_remove_file()
to be called twice.

> 
> 
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >>  drivers/media/media-device.c |  5 ++++-
> >>  include/media/media-device.h | 17 +++++++++++++++++
> >>  2 files changed, 21 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 20c85a9..1bb9a5f 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -749,10 +749,13 @@ void media_device_unregister(struct media_device *mdev)
> >>  	spin_lock(&mdev->lock);
> >>  
> >>  	/* Check if mdev was ever registered at all */
> >> -	if (!media_devnode_is_registered(&mdev->devnode)) {
> >> +	/* check if unregister is in progress */
> >> +	if (!media_devnode_is_registered(&mdev->devnode) ||
> >> +	    mdev->unregister_in_progress) {
> >>  		spin_unlock(&mdev->lock);
> >>  		return;
> >>  	}
> >> +	mdev->unregister_in_progress = true;
> >>  
> >>  	/* Remove all entities from the media device */
> >>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index 04b6c2e..0807292 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -332,6 +332,10 @@ struct media_device {
> >>  	spinlock_t lock;
> >>  	/* Serializes graph operations. */
> >>  	struct mutex graph_mutex;
> >> +	/* Tracks unregister in progress state to prevent
> >> +	 * more than one driver entering unregister
> >> +	*/
> >> +	bool unregister_in_progress;
> >>  
> >>  	/* Handlers to find source entity for the sink entity and
> >>  	 * check if it is available, and activate the link using
> >> @@ -365,6 +369,7 @@ struct media_device {
> >>  /* media_devnode to media_device */
> >>  #define to_media_device(node) container_of(node, struct media_device, devnode)
> >>  
> >> +
> >>  /**
> >>   * media_entity_enum_init - Initialise an entity enumeration
> >>   *
> >> @@ -553,6 +558,12 @@ struct media_device *media_device_get_devres(struct device *dev);
> >>   * @dev: pointer to struct &device.
> >>   */
> >>  struct media_device *media_device_find_devres(struct device *dev);
> >> +/* return unregister in progress state */
> >> +static inline bool media_device_is_unregister_in_progress(
> >> +					struct media_device *mdev)
> >> +{
> >> +	return mdev->unregister_in_progress;
> >> +}
> >>  
> >>  /* Iterate over all entities. */
> >>  #define media_device_for_each_entity(entity, mdev)			\
> >> @@ -569,6 +580,7 @@ struct media_device *media_device_find_devres(struct device *dev);
> >>  /* Iterate over all links. */
> >>  #define media_device_for_each_link(link, mdev)			\
> >>  	list_for_each_entry(link, &(mdev)->links, graph_obj.list)
> >> +
> >>  #else
> >>  static inline int media_device_register(struct media_device *mdev)
> >>  {
> >> @@ -604,5 +616,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev)
> >>  {
> >>  	return NULL;
> >>  }
> >> +static inline bool media_device_is_unregister_in_progress(
> >> +					struct media_device *mdev)
> >> +{
> >> +	return false;
> >> +}
> >>  #endif /* CONFIG_MEDIA_CONTROLLER */
> >>  #endif  
> 
> 

[toc] | [prev] | [next] | [standalone]

#1321044

On 01/28/2016 10:28 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jan 2016 10:04:24 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> On 01/28/2016 10:01 AM, Mauro Carvalho Chehab wrote:
>>> Em Wed,  6 Jan 2016 13:27:18 -0700
>>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>>   
>>>> Add support to track media device unregister in progress
>>>> state to prevent more than one driver entering unregister.
>>>> This enables fixing the general protection faults while
>>>> snd-usb-audio was cleaning up media resources for pcm
>>>> streams and mixers. In this patch a new interface is added
>>>> to return the unregister in progress state. Subsequent
>>>> patches to snd-usb-audio and au0828-core use this interface
>>>> to avoid entering unregister and attempting to unregister
>>>> entities and remove devnodes while unregister is in progress.
>>>> Media device unregister removes entities and interface nodes.  
>>>
>>> Hmm... isn't the spinlock enough to serialize it? It seems weird the
>>> need of an extra bool here to warrant that this is really serialized.
>>>   
>>
>> The spinlock and check for media_devnode_is_registered(&mdev->devnode)
>> aren't enough to ensure only one driver enters the unregister. 
>>
>> Please
>> note that the devnode isn't marked unregistered until the end in
>> media_device_unregister().
> 
> I guess the call to:
> 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> 
> IMO, This should be, instead, at media_devnode_unregister().
> 
> Then, we can change the logic at media_devnode_unregister() to:
> 
> void media_devnode_unregister(struct media_devnode *mdev)
> {
> 	mutex_lock(&media_devnode_lock);
> 
> 	/* Check if mdev was ever registered at all */
> 	if (!media_devnode_is_registered(mdev)) {
> 		mutex_unlock(&media_devnode_lock);
> 		return;
> 	}
> 
> 	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
> 	mutex_unlock(&media_devnode_lock);
> 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> 	device_unregister(&mdev->dev);
> }
> 
> This sounds enough to avoid device_unregister() or device_remove_file()
> to be called twice.
> 

I can give it a try. There might other problems that could
result from media device being a devres in this case. The
last put_device on the usbdev parent device (media device
is created as devres for this), all device resources get
released. That might have to be solved in a different way.

For now I will see if your solution works.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[toc] | [prev] | [next] | [standalone]

#1325893

On 01/28/2016 01:42 PM, Shuah Khan wrote:
> On 01/28/2016 10:28 AM, Mauro Carvalho Chehab wrote:
>> Em Thu, 28 Jan 2016 10:04:24 -0700
>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>
>>> On 01/28/2016 10:01 AM, Mauro Carvalho Chehab wrote:
>>>> Em Wed,  6 Jan 2016 13:27:18 -0700
>>>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>>>   
>>>>> Add support to track media device unregister in progress
>>>>> state to prevent more than one driver entering unregister.
>>>>> This enables fixing the general protection faults while
>>>>> snd-usb-audio was cleaning up media resources for pcm
>>>>> streams and mixers. In this patch a new interface is added
>>>>> to return the unregister in progress state. Subsequent
>>>>> patches to snd-usb-audio and au0828-core use this interface
>>>>> to avoid entering unregister and attempting to unregister
>>>>> entities and remove devnodes while unregister is in progress.
>>>>> Media device unregister removes entities and interface nodes.  
>>>>
>>>> Hmm... isn't the spinlock enough to serialize it? It seems weird the
>>>> need of an extra bool here to warrant that this is really serialized.
>>>>   
>>>
>>> The spinlock and check for media_devnode_is_registered(&mdev->devnode)
>>> aren't enough to ensure only one driver enters the unregister. 
>>>
>>> Please
>>> note that the devnode isn't marked unregistered until the end in
>>> media_device_unregister().
>>
>> I guess the call to:
>> 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>
>> IMO, This should be, instead, at media_devnode_unregister().
>>
>> Then, we can change the logic at media_devnode_unregister() to:
>>
>> void media_devnode_unregister(struct media_devnode *mdev)
>> {
>> 	mutex_lock(&media_devnode_lock);
>>
>> 	/* Check if mdev was ever registered at all */
>> 	if (!media_devnode_is_registered(mdev)) {
>> 		mutex_unlock(&media_devnode_lock);
>> 		return;
>> 	}
>>
>> 	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
>> 	mutex_unlock(&media_devnode_lock);
>> 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>> 	device_unregister(&mdev->dev);
>> }
>>
>> This sounds enough to avoid device_unregister() or device_remove_file()
>> to be called twice.
>>
> 
> I can give it a try. There might other problems that could
> result from media device being a devres in this case. The
> last put_device on the usbdev parent device (media device
> is created as devres for this), all device resources get
> released. That might have to be solved in a different way.
> 
> For now I will see if your solution works.

Hi Mauro,

Moving device_remove_file() won't be easy without
making more changes. The file is created in
media_device_regsiter() and all the attributes are
handled in media-device.c

One solution I can think of is clearing the
MEDIA_FLAG_REGISTERED bit very early in
media_device_unregister()

--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -759,6 +759,9 @@ void media_device_unregister(struct media_device *mdev)
                return;
        }
 
+       /* Protect unregister path - clear MEDIA_FLAG_REGISTERED */
+       clear_bit(MEDIA_FLAG_REGISTERED, &mdev->devnode.flags);
+
        /* Remove all entities from the media device */
        list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
                __media_device_unregister_entity(entity);

and changing media_devnode_unregister() to simply call
device_unregister(&mdev->dev);

Again clearing MEDIA_FLAG_REGISTERED bit in
media_device_unregister() some problems.
For one thing clearing this bit should be
done holding media_devnode_lock. It can be
done cleanly if we do the following:

How about if we split media_devnode_unregister()
into twp ohases:

media_devnode_start_unregister()
to clear this bit. It can do:

media_devnode_start_unregister()
{
	mutex_lock(&media_devnode_lock);
	if (!media_devnode_is_registered(mdev)) {
		mutex_unlock(&media_devnode_lock);
		return;
	}
	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
	mutex_unlock(&media_devnode_lock);
}

then:media_device_unregister(struct media_device *mdev)
will call this first thing and then hold mdev->lock
do the rest and the call media_devnode_unregister()
and which will be changed to as follows:

--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -274,13 +274,6 @@ error:
 
 void media_devnode_unregister(struct media_devnode *mdev)
 {
-       /* Check if mdev was ever registered at all */
-       if (!media_devnode_is_registered(mdev))
-               return;
-
-       mutex_lock(&media_devnode_lock);
-       clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
-       mutex_unlock(&media_devnode_lock);
        device_unregister(&mdev->dev);
 }

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web