| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160204071803.29d3682d@recife.lan>
Date: Thu, 4 Feb 2016 07:18:03 -0200
From: Mauro Carvalho Chehab <mchehab@....samsung.com>
To: Shuah Khan <shuahkh@....samsung.com>
Cc: tiwai@...e.com, clemens@...isch.de, hans.verkuil@...co.com,
laurent.pinchart@...asonboard.com, sakari.ailus@...ux.intel.com,
javier@....samsung.com, pawel@...iak.com, m.szyprowski@...sung.com,
kyungmin.park@...sung.com, perex@...ex.cz, arnd@...db.de,
dan.carpenter@...cle.com, tvboxspy@...il.com, crope@....fi,
ruchandani.tina@...il.com, corbet@....net, chehabrafael@...il.com,
k.kozlowski@...sung.com, stefanr@...6.in-berlin.de,
inki.dae@...sung.com, jh1009.sung@...sung.com,
elfring@...rs.sourceforge.net, prabhakar.csengg@...il.com,
sw0312.kim@...sung.com, p.zabel@...gutronix.de,
ricardo.ribalda@...il.com, labbott@...oraproject.org,
pierre-louis.bossart@...ux.intel.com, ricard.wanderlof@...s.com,
julian@...st.de, takamichiho@...il.com, dominic.sacre@....de,
misterpib@...il.com, daniel@...que.org, gtmkramer@...all.nl,
normalperson@...t.net, joe@...po.co.uk, linuxbugs@...tgam.net,
johan@...ud.se, klock.android@...il.com, nenggun.kim@...sung.com,
j.anaszewski@...sung.com, geliangtang@....com,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-api@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH v2 06/22] media: Media Controller non-locking
__media_entity_pipeline_start/stop()
Em Wed, 03 Feb 2016 21:03:38 -0700
Shuah Khan <shuahkh@....samsung.com> escreveu:
> Add non-locking __media_entity_pipeline_start/stop() interfaces
> to be called from code paths that hold the graph_mutex. For this
> change, the media_entity_pipeline_start() routine is renamed to
> __media_entity_pipeline_start() minus the graph_mutex lock and
> unlock. media_entity_pipeline_start() now calls the non-locking
> __media_entity_pipeline_start() holding the graph_lock. The stop
> interface, media_entity_pipeline_stop() routine is renamed to
> __media_entity_pipeline_stop() minus the graph_mutex lock and
> unlock. media_entity_pipeline_stop() now calls the non-locking
> __media_entity_pipeline_stop() holding the graph_lock.
Please write a better patch descriptions. The above could easily
be split into 3 or 4 paragraphs, with would make it easier to
read.
Also, too much details, written on a spaghetti style;)
It is obvious by anyone that looked at the patch that
"media_entity_pipeline_start() routine is renamed to
__media_entity_pipeline_start() minus the graph_mutex lock and
unlock."
I read exactly the same thing at the C code, *and* at the
subject of the e-mail. Patch subject/description should tell
what, why and how (but you should not repeat what can easily
be inferred from looking at the diff).
I would describe it as:
Currently, media pipeline graph traversal routines take
the graph_mutex lock.
While this works for the current code, we'll need
unlocked versions of them because
(some explanation *why* you need it, with is the main
thing that should be at the description).
So, add non-locking media_entity_pipeline_start/stop()
variations.
>
> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
> ---
> drivers/media/media-entity.c | 34 ++++++++++++++++++++++++----------
> include/media/media-entity.h | 12 ++++++++++++
> 2 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 9b4d712..b78e4c2 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -365,8 +365,8 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
> * Pipeline management
> */
>
> -__must_check int media_entity_pipeline_start(struct media_entity *entity,
> - struct media_pipeline *pipe)
> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
> + struct media_pipeline *pipe)
> {
> struct media_device *mdev = entity->graph_obj.mdev;
> struct media_entity_graph *graph = &pipe->graph;
> @@ -374,8 +374,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> struct media_link *link;
> int ret;
>
> - mutex_lock(&mdev->graph_mutex);
> -
> if (!pipe->streaming_count++) {
> ret = media_entity_graph_walk_init(&pipe->graph, mdev);
> if (ret)
> @@ -456,8 +454,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> }
> }
>
> - mutex_unlock(&mdev->graph_mutex);
> -
> return 0;
>
> error:
> @@ -487,19 +483,28 @@ error_graph_walk_start:
> if (!--pipe->streaming_count)
> media_entity_graph_walk_cleanup(graph);
>
> - mutex_unlock(&mdev->graph_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_start);
>
> +__must_check int media_entity_pipeline_start(struct media_entity *entity,
> + struct media_pipeline *pipe)
> +{
> + struct media_device *mdev = entity->graph_obj.mdev;
> + int ret;
> +
> + mutex_lock(&mdev->graph_mutex);
> + ret = __media_entity_pipeline_start(entity, pipe);
> + mutex_unlock(&mdev->graph_mutex);
> return ret;
> }
> EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
>
> -void media_entity_pipeline_stop(struct media_entity *entity)
> +void __media_entity_pipeline_stop(struct media_entity *entity)
> {
> - struct media_device *mdev = entity->graph_obj.mdev;
> struct media_entity_graph *graph = &entity->pipe->graph;
> struct media_pipeline *pipe = entity->pipe;
>
> - mutex_lock(&mdev->graph_mutex);
>
> WARN_ON(!pipe->streaming_count);
> media_entity_graph_walk_start(graph, entity);
> @@ -516,6 +521,15 @@ void media_entity_pipeline_stop(struct media_entity *entity)
> if (!--pipe->streaming_count)
> media_entity_graph_walk_cleanup(graph);
>
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_stop);
> +
> +void media_entity_pipeline_stop(struct media_entity *entity)
> +{
> + struct media_device *mdev = entity->graph_obj.mdev;
> +
> + mutex_lock(&mdev->graph_mutex);
> + __media_entity_pipeline_stop(entity);
> mutex_unlock(&mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fe485d3..c7583db 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -832,6 +832,12 @@ media_entity_graph_walk_next(struct media_entity_graph *graph);
> */
> __must_check int media_entity_pipeline_start(struct media_entity *entity,
> struct media_pipeline *pipe);
> +/**
> + * non-locking __media_entity_pipeline_start() can be called from
> + * code paths that hold the graph_mutex
> + */
> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
> + struct media_pipeline *pipe);
>
> /**
> * media_entity_pipeline_stop - Mark a pipeline as not streaming
> @@ -848,6 +854,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> void media_entity_pipeline_stop(struct media_entity *entity);
>
> /**
> + * non-locking __media_entity_pipeline_stop() can be called from
> + * code paths that hold the graph_mutex
> + */
> +void __media_entity_pipeline_stop(struct media_entity *entity);
> +
> +/**
> * media_devnode_create() - creates and initializes a device node interface
> *
> * @mdev: pointer to struct &media_device
Powered by blists - more mailing lists