Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

[Andreas Oberritter <obi@linuxtv.org>]

On 10.12.2011 02:59, Devin Heitmueller wrote:
> On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter <obi@linuxtv.org> wrote:
>> On 10.12.2011 00:43, Mauro Carvalho Chehab wrote:
>>> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote:
>>>> On 09-12-2011 20:33, Devin Heitmueller wrote:
>>>>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab
>>>>> <mchehab@redhat.com> wrote:
>>>>>>> Could someone explain reason for that?
>>>>>>
>>>>>>
>>>>>> I dunno, but I think this needs to be fixed, at least when the frontend
>>>>>> is opened with O_NONBLOCK.
>>>>>
>>>>> Are you doing the drx-k firmware load on dvb_init()? That could
>>>>> easily take 4 seconds.
>>>>
>>>> No. The firmware were opened previously.
>>>
>>> Maybe the delay is due to this part of dvb_frontend.c:
>>>
>>> static int dvb_mfe_wait_time = 5;
>>> ...
>>>                         int mferetry = (dvb_mfe_wait_time << 1);
>>>
>>>                         mutex_unlock (&adapter->mfe_lock);
>>>                         while (mferetry-- && (mfedev->users != -1 ||
>>>                                         mfepriv->thread != NULL)) {
>>>                                 if(msleep_interruptible(500)) {
>>>                                         if(signal_pending(current))
>>>                                                 return -EINTR;
>>>                                 }
>>>                         }
>>
>> I haven't looked at the mfe code, but in case it's waiting for the
>> frontend thread to exit, there's a problem that causes the thread
>> not to exit immediately. Here's a patch that's been sitting in my
>> queue for a while:
>>
>> ---
>>
>> Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
>>
>> diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>> index 7784d74..6823c2b 100644
>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-07 12:32:24.000000000 +0200
>> +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-13 15:55:48.865742791 +0200
>> @@ -514,7 +514,7 @@
>>                return 1;
>>
>>        if (fepriv->dvbdev->writers == 1)
>> -               if (time_after(jiffies, fepriv->release_jiffies +
>> +               if (time_after_eq(jiffies, fepriv->release_jiffies +
>>                                  dvb_shutdown_timeout * HZ))
>>                        return 1;
>>
>> @@ -2070,12 +2070,15 @@
>>
>>        dprintk ("%s\n", __func__);
>>
>> -       if ((file->f_flags & O_ACCMODE) != O_RDONLY)
>> +       if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
>>                fepriv->release_jiffies = jiffies;
>> +               mb();
>> +       }
>>
>>        ret = dvb_generic_release (inode, file);
>>
>>        if (dvbdev->users == -1) {
>> +               wake_up(&fepriv->wait_queue);
>>                if (fepriv->exit != DVB_FE_NO_EXIT) {
>>                        fops_put(file->f_op);
>>                        file->f_op = NULL;
> 
> This patch needs to have a much better explanation of exactly what it
> does and what problem it solves.  We have a history of race conditions
> in dvb_frontend.c, and it's patches like this with virtually no
> details just makes it worse.
> 
> I'm not arguing the actual merits of the code change - it *may* be
> correct.  But without the appropriate background there is no real way
> of knowing...
> 
> Mauro, this patch should be NACK'd and resubmitted with a detailed
> explanation of the current behavior, what the problem is, and how the
> code changes proposed solve that problem.

WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free
to analyze the code and resubmit it.