| 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: <56B36787.4060304@osg.samsung.com>
Date: Thu, 4 Feb 2016 08:00:23 -0700
From: Shuah Khan <shuahkh@....samsung.com>
To: Mauro Carvalho Chehab <mchehab@....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,
alsa-devel@...a-project.org, Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH v2 06/22] media: Media Controller non-locking
__media_entity_pipeline_start/stop()
On 02/04/2016 02:18 AM, Mauro Carvalho Chehab wrote:
> 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.
>
Removed linux-api from cc list
ok - I updated several changelogs based on your earlier
comments and this one slipped by.
-- Shuah
> 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
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@....samsung.com | (970) 217-8978
Powered by blists - more mailing lists