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

[Shuah Khan]

On 10/16/2014 08:00 AM, Takashi Iwai wrote:
> At Wed, 15 Oct 2014 18:53:28 -0600,
> Shuah Khan wrote:
>>
>> On 10/15/2014 11:05 AM, Takashi Iwai wrote:
>>
>>>> +#if defined(CONFIG_MEDIA_SUPPORT)
>>>> +extern int media_tknres_create(struct device *dev);
>>>> +extern int media_tknres_destroy(struct device *dev);
>>>> +
>>>> +extern int media_get_tuner_tkn(struct device *dev);
>>>> +extern int media_put_tuner_tkn(struct device *dev);
>>>> +
>>>> +extern int media_get_audio_tkn(struct device *dev);
>>>> +extern int media_put_audio_tkn(struct device *dev);
>>>
>>> The words "tknres" and "tkn" (especially latter) look ugly and not
>>> clear to me.  IMO, it should be "token".
>>
>> No problem. I can change that to media_token_create/destroy()
>> and expand token in other functions.
>>
>>>> +struct media_tkn {
>>>> +	spinlock_t lock;
>>>> +	unsigned int owner;	/* owner task pid */
>>>> +	unsigned int tgid;	/* owner task gid */
>>>> +	struct task_struct *task;
>>>> +};
>>>> +
>>>> +struct media_tknres {
>>>> +	struct media_tkn tuner;
>>>> +	struct media_tkn audio;
>>>> +};
>>>
>>> Why do you need to have both tuner and audio tokens?  If I understand
>>> correctly, no matter whether it's tuner or audio, if it's being used,
>>> the open must fail, right?
>>
>> As it evolved during development, it turns out at the moment I don't
>> have any use-cases that require holding audio and tuner separately.
>> It probably could be collapsed into just a media token. I have to
>> think about this some.
>>
>>>> +
>>>> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
>>>> +{
>>>> +	int rc = 0;
>>>> +	unsigned tpid;
>>>> +	unsigned tgid;
>>>> +
>>>> +	spin_lock(&tkn->lock);
>>>
>>> You should use spin_lock_irqsave() here and in all other places.
>>> The trigger callback in usb-audio, for example, may be called in irq
>>> context.
>>
>> ok. Good point, will change that.
>>
>>>
>>>> +
>>>> +	tpid = task_pid_nr(current);
>>>> +	tgid = task_tgid_nr(current);
>>>> +
>>>> +	/* allow task in the same group id to release */
>>>
>>> IMO, it's not "release" but "steal"...  But what happens if the stolen
>>> owner calls put?  Then it'll be released although the stealing owner
>>> still thinks it's being held.
>>
>> Yeah it could be called a steal. :) Essentially tgid happens to be
>> the real owner. I am overwriting the pid with current pid when
>> tgid is same.
>>
>> media dvb and v4l apps start two or more threads that all share the
>> tgid and subsequent token gets should be allowed based on the tgid.
>>
>> Scenario 1:
>>
>> Please note that there are 3 device files in question and media
>> token resource is the lock for all. Hence the changes to v4l-core,
>> dvb-core, and snd-usb-audio to hold the token for exclusive access
>> when the task or tgid don't match.
>>
>> program starts:
>> - first thread opens device file in R/W mode - open gets the token
>>   or thread makes ioctls calls that clearly indicates intent to
>>   change tuner settings
>> - creates one thread for audio
>> - creates another for video or continues video function
>> - video thread does a close and re-opens the device file
>>
>>   In this case when thread tries to close, token put fails unless
>>   tasks with same tgid are allowed to release.
>>   ( I ran into this one of the media applications and it is a valid
>>     case to handle, thread can close the file and should be able to
>>     open again without running into token busy case )
>>
>> - or continue to just use the device file
>> - audio thread does snd_pcm_capture ops - trigger start
>>
>> program exits:
>> - video thread closes the device file
>> - audio thread does snd_pcm_capture ops - trigger stop
>>
>> This got me thinking about the scenario when an application
>> does a fork() as opposed to create a thread. I have to think
>> about this and see how I can address that.
>>
>>>
>>>> +	if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
>>>
>>> Shouldn't the second "&&" be "||" instead?
>>> And too many parentheses there.
>>
>> Right - this is my bad. The comment right above this conditional
>> is a give away that, at some point I did a copy and paste from
>> _put to _get and only changed the first task null check.
>> I am yelling at myself at the moment.
>>
>>>
>>>> +			rc = -EBUSY;
>>>
>>> Wrong indentation.
>>
>> Yes. Will fix that.
>>
>>>
>>> I have a feeling that the whole thing can be a bit more simplified in
>>> the end...
>>>
>>
>> Any ideas to simplify are welcome.
> 
> There are a few points to make things more consistent and simplified,
> IMO.  First of all, the whole concept can be more generalized.  It's
> not necessarily only for media and audio.  Rather we can make it a
> generic dev_token.  Then it'll become more convincing to land into
> lib/*.

Right. At the moment this is very media specific.

> 
> The media-specific handling is only about the pid and gid checks.
> This can be implemented as a callback that is passed at creating a
> token, for example.  This will reduce the core code in lib/*.
> 
> Also, get and put should handle a proper refcount.  The point I
> mentioned in my previous post is the possible unbalance, and you'll
> see unexpected unlock in the scenario.  In addition, with use of kref,
> the lock can be removed.

Yes. kref would eliminate the need for locks and potential for
unbalanced scenario.

> 
> So, essentially the lib code would be just a devres_alloc() for an
> object with kref, and dev_token_get() will take a kref with the
> exclusion check.
> 

I considered callback for media specific handling to make this token
construct generic. Talked myself out of it with the idea to get this
working for media use-case first and then extend it to be generic.

With this patch set covering media cases including non-media driver,
I think I am confident that the approach itself works to address the
issues and I don't mind pursuing a generic approach you are suggesting.
The current work isn't that far off and I can get the generic working
without many changes.

Thanks again for talking through the problem and ideas.

-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/