Is DVB ioctl FE_SET_FRONTEND broken?

Is DVB ioctl FE_SET_FRONTEND broken?

[Chris Rankin]

Hi,

As far as I understand it, the FE_SET_FRONTEND ioctl is supposed to tell a DVB device to tune itself, and will send a poll() event when it completes. The "frequency" parameter of this event will be the frequency of the newly tuned channel, or 0 if tuning failed.

http://www.linuxtv.org/docs/dvbapi/DVB_Frontend_API.html#SECTION00328000000000000000

I have now tested with 2 different DVB adapters, and I don't think the 3.0.x kernel still behaves like this. A study of the dvb-core/dvb_frontend.c file reveals the following code:

In the dvb_frontend_ioctl_legacy() function,

    switch(cmd) {

    ...

    case FE_SET_FRONTEND: {

        ...

        fepriv->state = FESTATE_RETUNE;

        /* Request the search algorithm to search */
        fepriv->algo_status |= DVBFE_ALGO_SEARCH_AGAIN;

        dvb_frontend_wakeup(fe);
        dvb_frontend_add_event(fe, 0);   // <--- HERE!!!!
        fepriv->status = 0;
        err = 0;
        break;
    }

So basically, the ioctl always sends an event immediately and does not wait for the tuning to happen first. Presumably, the device still tunes in the background and writes the frequency into the frontend's private structure so that a second FE_SET_FRONTEND ioctl succeeds. But this is not the documented behaviour.

The bug is visible when you try to use the device for the very first time. I tested by unloading / reloading the kernel modules, launching xine and then pressing its DVB button. This *always* fails the first time, for the reason described above, and works every time after that.

Cheers,
Chris

Re: Is DVB ioctl FE_SET_FRONTEND broken?

[Andreas Oberritter]

Hello Chris,

On 26.08.2011 01:27, Chris Rankin wrote:
> As far as I understand it, the FE_SET_FRONTEND ioctl is supposed to tell a DVB device to tune itself, and will send a poll() event when it completes. The "frequency" parameter of this event will be the frequency of the newly tuned channel, or 0 if tuning failed.
> 
> http://www.linuxtv.org/docs/dvbapi/DVB_Frontend_API.html#SECTION00328000000000000000
> 
> I have now tested with 2 different DVB adapters, and I don't think the 3.0.x kernel still behaves like this. A study of the dvb-core/dvb_frontend.c file reveals the following code:
> 
> In the dvb_frontend_ioctl_legacy() function,
> 
>     switch(cmd) {
> 
>     ...
> 
>     case FE_SET_FRONTEND: {
> 
>         ...
> 
>         fepriv->state = FESTATE_RETUNE;
> 
>         /* Request the search algorithm to search */
>         fepriv->algo_status |= DVBFE_ALGO_SEARCH_AGAIN;
> 
>         dvb_frontend_wakeup(fe);
>         dvb_frontend_add_event(fe, 0);   // <--- HERE!!!!
>         fepriv->status = 0;
>         err = 0;
>         break;
>     }
> 
> So basically, the ioctl always sends an event immediately and does not wait for the tuning to happen first. Presumably, the device still tunes in the background and writes the frequency into the frontend's private structure so that a second FE_SET_FRONTEND ioctl succeeds. But this is not the documented behaviour.
> 
> The bug is visible when you try to use the device for the very first time. I tested by unloading / reloading the kernel modules, launching xine and then pressing its DVB button. This *always* fails the first time, for the reason described above, and works every time after that.

can you please test whether https://patchwork.kernel.org/patch/1036132/
restores the old behaviour?

These three pending patches are also related to frontend events:
https://patchwork.kernel.org/patch/1036112/
https://patchwork.kernel.org/patch/1036142/
https://patchwork.kernel.org/patch/1036122/

Regards,
Andreas

Re: Is DVB ioctl FE_SET_FRONTEND broken?

[Chris Rankin]

--- On Fri, 26/8/11, Andreas Oberritter <obi@linuxtv.org> wrote:
> can you please test whether https://patchwork.kernel.org/patch/1036132/
> restores the old behaviour?
> 
> These three pending patches are also related to frontend events:
>
> https://patchwork.kernel.org/patch/1036112/
> https://patchwork.kernel.org/patch/1036142/
> https://patchwork.kernel.org/patch/1036122/

Andreas,

I've only reviewed these patches so far, but I am concerned that we both have a different understanding of what the FE_SET_FRONTEND ioctl is supposed to do. According to the documentation:

"The result of this call will be successful if the parameters were valid and the tuning could be initiated. The result of the tuning operation in itself, however, will arrive asynchronously as an event."

However, your comment in your first patch reads:

"FE_SET_FRONTEND triggers an initial frontend event with status = 0, which copies output parameters to userspace."

To my mind, these are conflicting statements because how can there be such thing as "an initial frontend event"? I am not expecting the kernel to send any event until the tuning has finished and it can give me real information. I am *definitely* not expecting the kernel to send my input parameters straight back to me.

Given the documented description of this ioctl, I would write the following (pseudo)code in userspace:

int rc;

rc = ioctl(fd, FE_SET_FRONTEND, &args);
if (rc != 0) {
    printf("Error: could not start tuning.\n");
} else {
    struct pollfd  pfd;
    pfd.fd = fd;
    pfd.events = POLLIN;
    pfd.revents = 0;

    // Wait 5 seconds for tuning to finish.
    rc = poll(&pfd, 1, 5000);
    if (rc < 0) {
        printf("Error!\n");
    } else if (rc == 0) {
        printf("Still not tuned after 5 seconds - give up!\n");
    } else {
        printf("YAY! WE ARE TUNED!\n");
    }
}

But your code adds an event to the queue as the ioctl() exits, which means that my pseudocode is never going to print the "Still not tuned after 5 seconds" message but jump straight to "YAY! WE ARE TUNED!" instead while tuning is actually still in progress.

So I'm going to say "No", your patches don't restore the old behaviour.

Cheers,
Chris

Re: Is DVB ioctl FE_SET_FRONTEND broken?

[Andreas Oberritter]

Hi Chris,

reading your first message again, I realize that I was misunderstanding
you. I first thought that you were talking about a regression in Linux
3.0.x.

On 26.08.2011 10:50, Chris Rankin wrote:
> --- On Fri, 26/8/11, Andreas Oberritter <obi@linuxtv.org> wrote:
>> can you please test whether https://patchwork.kernel.org/patch/1036132/
>> restores the old behaviour?
>>
>> These three pending patches are also related to frontend events:
>>
>> https://patchwork.kernel.org/patch/1036112/
>> https://patchwork.kernel.org/patch/1036142/
>> https://patchwork.kernel.org/patch/1036122/
> 
> Andreas,
> 
> I've only reviewed these patches so far, but I am concerned that we both have a different understanding of what the FE_SET_FRONTEND ioctl is supposed to do. According to the documentation:
> 
> "The result of this call will be successful if the parameters were valid and the tuning could be initiated. The result of the tuning operation in itself, however, will arrive asynchronously as an event."

The quoted text is right.

"Result of this call" and "result of the tuning operation" are distinct,
where "result of this call" means that ioctl() will return 0. The
"result of a tuning operation" is not a single value (success/failure),
but a series of changes. I.e., you'll get an event for every status
change reported by the tuner, e.g. also when a signal is lost.

> However, your comment in your first patch reads:
> 
> "FE_SET_FRONTEND triggers an initial frontend event with status = 0, which copies output parameters to userspace."
> 
> To my mind, these are conflicting statements because how can there be such thing as "an initial frontend event"? I am not expecting the kernel to send any event until the tuning has finished and it can give me real information. I am *definitely* not expecting the kernel to send my input parameters straight back to me.

This initial event with status=0 exists since 2002. It's used to notify
a new tuning operation to the event listener.

http://www.linuxtv.org/cgi-bin/viewvc.cgi/DVB/driver/dvb_frontend.c?revision=1.6.2.30&view=markup

> Given the documented description of this ioctl, I would write the following (pseudo)code in userspace:
> 
> int rc;
> 
> rc = ioctl(fd, FE_SET_FRONTEND, &args);
> if (rc != 0) {
>     printf("Error: could not start tuning.\n");
> } else {
>     struct pollfd  pfd;
>     pfd.fd = fd;
>     pfd.events = POLLIN;
>     pfd.revents = 0;
> 
>     // Wait 5 seconds for tuning to finish.
>     rc = poll(&pfd, 1, 5000);
>     if (rc < 0) {
>         printf("Error!\n");
>     } else if (rc == 0) {
>         printf("Still not tuned after 5 seconds - give up!\n");
>     } else {
>         printf("YAY! WE ARE TUNED!\n");
>     }
> }
> 
> But your code adds an event to the queue as the ioctl() exits, which means that my pseudocode is never going to print the "Still not tuned after 5 seconds" message but jump straight to "YAY! WE ARE TUNED!" instead while tuning is actually still in progress.

It's not my code and my patch doesn't create any new event.

Your example code can't work. You need to call FE_GET_EVENT or
FE_READ_STATUS.

> So I'm going to say "No", your patches don't restore the old behaviour.

Yes. The patch is restoring a different old behaviour. The behaviour
you're referring to has never been in the kernel. ;-)

Regards,
Andreas

Re: Is DVB ioctl FE_SET_FRONTEND broken?

[Chris Rankin]

--- On Fri, 26/8/11, Andreas Oberritter <obi@linuxtv.org> wrote:
> I first thought that you were talking about a
> regression in Linux 3.0.x.

Heh, yes and no. I am talking about a regression that I am definitely seeing in 3.0.x. However, I cannot say which kernel the problem first appeared in.

> This initial event with status=0 exists since 2002. It's
> used to notify a new tuning operation to the event listener.
> 
> http://www.linuxtv.org/cgi-bin/viewvc.cgi/DVB/driver/dvb_frontend.c?revision=1.6.2.30&view=markup

OK, that's different. I've only noticed this regression because xine has started having trouble using a brand new DVB adapter. Debugging the problem has shown that the first event received after a FE_SET_FRONTEND ioctl() has frequency == 0, which is considered an error.

Reading the documentation for FE_SET_FRONTEND lead me to believe that it would send only a single event once tuning had completed, which is not what the code does.
 
> It's not my code and my patch doesn't create any new event.

Those patches don't, no. I was assuming that you were patching code that you had patched earlier. My bad, it seems.

> Your example code can't work. You need to call FE_GET_EVENT
> or FE_READ_STATUS.

And that's why I only called it "pseudocode" :-).
 
> > So I'm going to say "No", your patches don't restore the old behaviour.
> 
> Yes. The patch is restoring a different old behaviour. The
> behaviour you're referring to has never been in the kernel. ;-)

Yikes! Documentation bug, anyone?

Cheers,
Chris

Re: Is DVB ioctl FE_SET_FRONTEND broken?

[Andreas Oberritter]

On 26.08.2011 13:31, Chris Rankin wrote:
> --- On Fri, 26/8/11, Andreas Oberritter <obi@linuxtv.org> wrote:
>> I first thought that you were talking about a
>> regression in Linux 3.0.x.
> 
> Heh, yes and no. I am talking about a regression that I am definitely seeing in 3.0.x. However, I cannot say which kernel the problem first appeared in.

[...]

> Debugging the problem has shown that the first event received after a FE_SET_FRONTEND ioctl() has frequency == 0, which is considered an error.

OK, this is actually the problem, which the proposed patch tries to
address: https://patchwork.kernel.org/patch/1036132/

What you're observing is the stale data mentioned in the patch
description. You should definitely try the patch.

>> Yes. The patch is restoring a different old behaviour. The
>> behaviour you're referring to has never been in the kernel. ;-)
> 
> Yikes! Documentation bug, anyone?

Large parts of the documentation haven't been updated since about 10
years, besides some renamed enums and functions here and there, when it
was merged into Linux 2.5. Contributions are welcome, of course.

Regards,
Andreas