[3/5,media] au0828: Unregister notifiers
diff mbox

Message ID cdba12a00adbecabb66a662982991178383917b2.1458129823.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab March 16, 2016, 12:04 p.m. UTC
If au0828 gets removed, we need to remove the notifiers.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Javier Martinez Canillas March 16, 2016, 1:11 p.m. UTC | #1
Hello Mauro,

Patch looks good to me, I just have a minor comment:

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> If au0828 gets removed, we need to remove the notifiers.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 2fcd17d9b1a6..06da73f1ff22 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
>         return status;
>  }
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static void au0828_media_graph_notify(struct media_entity *new,
> +                                     void *notify_data);
> +#endif
> +

I would rather move the au0828_media_graph_notify() function
definition before au0828_unregister_media_device() instead of adding
this function forward declaration. Specially since requires ifdef
guards which makes it even harder to read.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier
--
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
Shuah Khan March 22, 2016, 7:55 p.m. UTC | #2
On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> If au0828 gets removed, we need to remove the notifiers.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

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/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 2fcd17d9b1a6..06da73f1ff22 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
>  	return status;
>  }
>  
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static void au0828_media_graph_notify(struct media_entity *new,
> +				      void *notify_data);
> +#endif
> +
>  static void au0828_unregister_media_device(struct au0828_dev *dev)
>  {
> -
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	if (dev->media_dev &&
> -		media_devnode_is_registered(&dev->media_dev->devnode)) {
> -		/* clear enable_source, disable_source */
> -		dev->media_dev->source_priv = NULL;
> -		dev->media_dev->enable_source = NULL;
> -		dev->media_dev->disable_source = NULL;
> +	struct media_device *mdev = dev->media_dev;
> +	struct media_entity_notify *notify, *nextp;
>  
> -		media_device_unregister(dev->media_dev);
> -		media_device_cleanup(dev->media_dev);
> -		dev->media_dev = NULL;
> +	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> +		return;
> +
> +	/* Remove au0828 entity_notify callbacks */
> +	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
> +		if (notify->notify != au0828_media_graph_notify)
> +			continue;
> +		media_device_unregister_entity_notify(mdev, notify);
>  	}
> +
> +	/* clear enable_source, disable_source */
> +	dev->media_dev->source_priv = NULL;
> +	dev->media_dev->enable_source = NULL;
> +	dev->media_dev->disable_source = NULL;
> +
> +	media_device_unregister(dev->media_dev);
> +	media_device_cleanup(dev->media_dev);
> +	dev->media_dev = NULL;
>  #endif
>  }
>  
>

Patch
diff mbox

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 2fcd17d9b1a6..06da73f1ff22 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -131,21 +131,35 @@  static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
 	return status;
 }
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+static void au0828_media_graph_notify(struct media_entity *new,
+				      void *notify_data);
+#endif
+
 static void au0828_unregister_media_device(struct au0828_dev *dev)
 {
-
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (dev->media_dev &&
-		media_devnode_is_registered(&dev->media_dev->devnode)) {
-		/* clear enable_source, disable_source */
-		dev->media_dev->source_priv = NULL;
-		dev->media_dev->enable_source = NULL;
-		dev->media_dev->disable_source = NULL;
+	struct media_device *mdev = dev->media_dev;
+	struct media_entity_notify *notify, *nextp;
 
-		media_device_unregister(dev->media_dev);
-		media_device_cleanup(dev->media_dev);
-		dev->media_dev = NULL;
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
+		return;
+
+	/* Remove au0828 entity_notify callbacks */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
+		if (notify->notify != au0828_media_graph_notify)
+			continue;
+		media_device_unregister_entity_notify(mdev, notify);
 	}
+
+	/* clear enable_source, disable_source */
+	dev->media_dev->source_priv = NULL;
+	dev->media_dev->enable_source = NULL;
+	dev->media_dev->disable_source = NULL;
+
+	media_device_unregister(dev->media_dev);
+	media_device_cleanup(dev->media_dev);
+	dev->media_dev = NULL;
 #endif
 }