linux-kernel - Re: [PATCH v2 1/6] media: add media token device resource framework

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 21 Oct 2014 13:58:59 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Takashi Iwai <tiwai@...e.de>
CC:	Shuah Khan <shuahkh@....samsung.com>, m.chehab@...sung.com,
	akpm@...ux-foundation.org, gregkh@...uxfoundation.org,
	crope@....fi, olebowle@....com, dheitmueller@...nellabs.com,
	sakari.ailus@...ux.intel.com, laurent.pinchart@...asonboard.com,
	perex@...ex.cz, prabhakar.csengg@...il.com,
	tim.gardner@...onical.com, linux@...elenboom.it,
	linux-media@...r.kernel.org, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] media: add media token device resource framework



On 10/21/2014 01:51 PM, Takashi Iwai wrote:
> At Tue, 21 Oct 2014 12:46:03 +0200,
> Hans Verkuil wrote:
>>
>> Hi Shuah,
>>
>> As promised, here is my review for this patch series.
>>
>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>> Add media token device resource framework to allow sharing
>>> resources such as tuner, dma, audio etc. across media drivers
>>> and non-media sound drivers that control media hardware. The
>>> Media token resource is created at the main struct device that
>>> is common to all drivers that claim various pieces of the main
>>> media device, which allows them to find the resource using the
>>> main struct device. As an example, digital, analog, and
>>> snd-usb-audio drivers can use the media token resource API
>>> using the main struct device for the interface the media device
>>> is attached to.
>>>
>>> A shared media tokens resource is created using devres framework
>>> for drivers to find and lock/unlock. Creating a shared devres
>>> helps avoid creating data structure dependencies between drivers.
>>> This media token resource contains media token for tuner, and
>>> audio. When tuner token is requested, audio token is issued.
>>
>> Did you mean: 'tuner token is issued' instead of audio token?
>>
>> I also have the same question as Takashi: why do we have an audio
>> token in the first place? While you are streaming audio over alsa
>> the underlying tuner must be marked as being in use. It's all about
>> the tuner, since that's the resource that is being shared, not about
>> audio as such.
>>
>> For the remainder of my review I will ignore the audio-related code
>> and concentrate only on the tuner part.
>>
>>> Subsequent token (for tuner and audio) gets from the same task
>>> and task in the same tgid succeed. This allows applications that
>>> make multiple v4l2 ioctls to work with the first call acquiring
>>> the token and applications that create separate threads to handle
>>> video and audio functions.
>>>
>>> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
>>> ---
>>>    MAINTAINERS                  |    2 +
>>>    include/linux/media_tknres.h |   50 +++++++++
>>>    lib/Makefile                 |    2 +
>>>    lib/media_tknres.c           |  237 ++++++++++++++++++++++++++++++++++++++++++
>>
>> I am still not convinced myself that this should be a generic API.
>> The only reason we need it today is for sharing tuners. Which is almost
>> a purely media thing with USB audio as the single non-media driver that
>> will be affected. Today I see no use case outside of tuners. I would
>> probably want to keep this inside drivers/media.
>>
>> If this is going to be expanded it can always be moved to lib later.
>
> Well, my argument is that it should be more generic if it were
> intended to be put in lib.  It'd be fine to put into drivers/media,
> but this code snippet must be a separate module.  Otherwise usb-audio
> would grab the whole media stuff even if not needed at all.

Certainly.

>
> (snip)
>> I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION
>> and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this
>> module to the GPL devres_* functions. It took me some time to figure that
>> out.
>
> It was a code in lib, so it cannot be a module at all :)

Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it
compiles as a module :-)

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/