Cannot switch DVB-C channels on Xbox One Digital TV tuner without restarting webcamd

[mkuron]

I am trying to use an Xbox One Digital TV tuner (see #1 for details on that device) with webcamd 5.13.2.4 for receiving DVB-C on FreeBSD 12.2. Unfortunately, it only successfully tunes to a channel exactly once. If I then close the channel and re-open it, or tune to a different channel, nothing happens. I have to restart webcamd to get another shot at tuning to a channel.

The issue is independent of the client application; I have tried both tvheadend and minisatip. In the case of tvheadend, the issue manifests itself in being unable to find channels in any mux except for the one at the starting frequency of 114 MHz. To actually play back one of these channels, you have to then restart webcamd and tvheadend, which again gives you a single shot.

I do not know whether this issue is specific to the Xbox tuner; I do not currently have access to a different one. The tuner does work fine on Linux with any application.

To reproduce, run the following three commands in separate terminals:

1. webcamd -d ugen0.2
2. minisatip -f -Z *:1 -R /usr/local/share/minisatip/html
3. ffplay http://127.0.0.1:8080/?freq=314&msys=dvbc&sr=6900&mtype=256qam&pids=0,100,101,102,103,106,105,104 (replace the parameters with ones corresponding to a valid station on your cable TV network)

After a few seconds, video is played back. Abort the ffplay command and run it again (or run one for a different channel, in my case e.g. ffplay http://127.0.0.1:8080/?freq=338&msys=dvbc&sr=6900&mtype=256qam&pids=0,100,110,120,121,122,125,131,130). In this case, video never starts playing. Aborting and restarting the minisatip command does not help. I actually need to abort and restart the webcamd command to successfully get video playback again. This makes me relatively certain it is a webcamd issue and not the fault of minisatip.

Please let me know if there is any webcamd debug output I could provide to help track down the cause of this issue.

[mkuron]

I now managed to do a debug build:

git clone https://github.com/hselasky/webcamd.git
cd webcamd
git checkout v5.13.2.4
git submodule init && git submodule update
make patch && make configure HAVE_DEBUG=1 HAVE_CUSE=1 HAVE_DVB_DRV=1 && make -j 8 HAVE_DEBUG=1  HAVE_CUSE=1 HAVE_DVB_DRV=1

Comparing the debug output between the first and second attempt to play a channel, I see that the first difference is

- DBG: : dvb_frontend_clear_cache: Clearing cache for delivery system 3
+ DBG: : dvb_frontend_clear_cache: Clearing cache for delivery system 1

and the second one is

+ dvb-usb: error while enabling fifo.

Does it matter that it's using a different delivery system? Any idea while it would fail to enable the FIFO? When I add -m dib0700_core.debug=15 to the webcamd invocation, I see

modifying (1) streaming state for 0
the endpoint number (130) is not correct, use the adapter id instead
- data for streaming: 10 1
+ data for streaming: 10 fd
>>> 
0f 
10 
- 01 
+ fd
00 
<<< 
- 08 
+ 5b
+ ep 0 write error (status = -32, len: 4)
+ dvb-usb: error while enabling fifo.

The issue also happens with the current master branch and with v5.7.1.2.

[hselasky]

Hi, I cannot dig so much into this right now. But if you find a solution and patch, I will review it!

[mkuron]

Okay, so the -32 comes from usb_control_msg. And that returns -EPIPE only when libusb20_dev_request_sync fails. That function's error code is -99, corresponding to LIBUSB20_ERROR_OTHER. This is so unspecific that I can't tell where inside libusb it's coming from.

However, taking one step back and applying the following patch to ensure that the data sent matches what was sent the first time actually resolves my issue!

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 70219b3e8566..a5865bc137f4 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -627,7 +627,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
                        st->channel_state |=    1 << (3-adap->fe_adap[0].stream.props.endpoint);
        }

-       st->buf[2] |= st->channel_state;
+       st->buf[2] |= (onoff ? 1 : 255);

        deb_info("data for streaming: %x %x\n", st->buf[1], st->buf[2]);

The first time, streaming is enabled and disabled with channel_state values of 1 and 255, respectively. My patch makes it do that every time. Without my patch, it tries enabling with 253 the second time. Now clearly this patch bypasses a bunch of logic that surely is there for a reason, but at least it seems to get things working for me. But since I know nothing about the dib0700, I don't think I can come up with anything better.

[hselasky]

Good find. All patches for this piece of code should be upstreamed fist to Torvald's Linux, before I then import them back.

linux-media@vger.kernel.org

[mkuron]

The odd thing is that it works fine on Linux, i.e. this patch isn‘t needed there. And that code hasn‘t been touched in Torvalds‘ kernel in years, so I guess the code running in webcamd and the code running on my Ubuntu 20.04 machine are the same.

[hselasky]

Can you try to dig a bit more. Maybe your distribution overrides the driver, or maybe something else?

[mkuron]

I looked at Ubuntu's kernel patches and found that the only change it makes to the dib0700 driver is this one, and as far as I can tell that is in code for the remote control.

--- linux-5.4.0.orig/drivers/media/usb/dvb-usb/dib0700_core.c
+++ linux-5.4.0/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -818,7 +818,7 @@

        /* Starting in firmware 1.20, the RC info is provided on a bulk pipe */

-       if (intf->altsetting[0].desc.bNumEndpoints < rc_ep + 1)
+       if (intf->cur_altsetting->desc.bNumEndpoints < rc_ep + 1)
                return -ENODEV;

        purb = usb_alloc_urb(0, GFP_KERNEL);
@@ -838,7 +838,7 @@
         * Some devices like the Hauppauge NovaTD model 52009 use an interrupt
         * endpoint, while others use a bulk one.
         */
-       e = &intf->altsetting[0].endpoint[rc_ep].desc;
+       e = &intf->cur_altsetting->endpoint[rc_ep].desc;
        if (usb_endpoint_dir_in(e)) {
                if (usb_endpoint_xfer_bulk(e)) {
                        pipe = usb_rcvbulkpipe(d->udev, rc_ep);

[hselasky]

If you diff the Ubuntu kernel source code with my media_tree, what is the difference?

[hselasky]

It's also strange that bugfixes are not upstreamed.

[mkuron]

The diff I posted above actually turns out to be a patch from Linux 5.7 (https://github.com/torvalds/linux/commit/f52981019ad8d6718de79b425a574c6bddf81f7c), and Ubuntu seems to have backported that to its 5.4 kernel. So webcamd and Ubuntu behave identically here.

I now applied your patches to Linux 5.13-rc2 and Ubuntu's patches to Linux 5.4 and compared their dib* drivers:

git clone --recursive https://github.com/hselasky/webcamd.git
cd webcamd
make patch
cd media_tree
git add .
git checkout -b webcamd
git commit -m "webcamd"
git checkout v5.4
curl -Lq https://launchpad.net/ubuntu/+archive/primary/+sourcefiles/linux/5.4.0-84.94/linux_5.4.0-84.94.diff.gz | gzip -d | git apply -
git checkout -b ubuntu
git commit -m "ubuntu"
git diff ubuntu..webcamd drivers/media/usb/dvb-usb/dib*

The result is here. The differences are all trivial, with the exception of one from Linux 5.6 (https://github.com/torvalds/linux/commit/6e040e6f8f8b9429d861808244e1c451138a56e2) that Ubuntu has not backported. Judging from https://lore.kernel.org/all/20191106212120.27983-1-wsa+renesas@sang-engineering.com/, it does not change any functionality, but merely switches to a newer API and improves error handling. So I still have no idea where the difference between Ubuntu and webcamd are coming from. Are there any files beyond drivers/media/usb/dvb-usb/dib* that you would like to see diffs of?

[hselasky]

If you clone webcamd from github https://github.com/hselasky/webcamd/ and also its submodules.

Then you enter into media_tree and do a "git blame" on that file. Then find that exact line which is different and see who may have changed it!

As you can see Ubuntu is not exactly Linux :-)

I'm using stock Torvalds sources. So my guess is that Ubuntu is doing something on their own :-(

[mkuron]

Then find that exact line which is different and see who may have changed it!

That's basically what I did already. The set of differences between Ubuntu's kernel sources and your media_tree is gigantic, not only because one is based on 5.4 and the other one on 5.13, but also because both you and Ubuntu apply non-upstream patches. The differences in the dib0700 driver however are very small, see uw.diff. Since none of these differences are significant, I suspect the real cause of the issue with the Xbox tuner is in non-device-specific code (e.g. DVB, USB or I2C subsystems). Are there any specific files that you think might be worth looking at and comparing? I obviously can't look at the full diff, consisting of millions of lines spread out across the entire kernel.

I'm using stock Torvalds sources. So my guess is that Ubuntu is doing something on their own

Not in the dib0700 as discussed above. Of course, they are definitely doing lots of things on their own elsewhere, so I'll try a vanilla Linux kernel on the weekend and see whether that exhibits the problem or not. If it does, then it's either a regression in 5.13 over 5.4 or something that Ubuntu patched. If it does not, then I guess it's a bug in webcamd's user-space compatibility layer?

[hselasky]

I suspect this is a regression. I'll try to find the Linux upstream commit which changed this code-path or if you want to do that, no problem either.

[mkuron]

I don't know which changed code path might be causing this, but if you have an idea, please do try to find the Linux upstream commit responsible. I am happy to try out anything you find.

[mkuron]

I just tried a vanilla 5.13-rc2 kernel, the same version that webcamd's media_tree is. And it does not exhibit the problem -- it tunes to any channels in any order just fine. So it's clearly not a regression in the Linux kernel. Any chance it might be a bug in webcamd's user-space interface code?

[hselasky]

And you also diffed the code, that it is identical, before applying any BSD patches?

Might be a bug in webcamd .... but still strange.

--HPS

[mkuron]

Yes, Torvalds' v5.13-rc2 and Ubuntu's v5.13-rc2 are identical except for a bunch of packaging stuff. There is one obvious difference, however: Ubuntu uses its own config file, which differs from Torvalds' default config file. Unless you can identify one of the differing config options as a likely cause, this really seems like a webcamd bug.

[hselasky]

Do you think there may be two different drivers for your hardware?

[hselasky]

Under linux, can you do "lsmod" and if there is a "-v" option , add that too.

[mkuron]

It uses the same driver, but it loads quite a few extra modules:

tda18250               24576  1
mn88472                24576  1
dvb_usb_dib0700       163840  0
dib9000                40960  1 dvb_usb_dib0700
dib7000m               24576  1 dvb_usb_dib0700
dib0090                40960  1 dvb_usb_dib0700
dib0070                20480  1 dvb_usb_dib0700
dib3000mc              20480  1 dvb_usb_dib0700
dibx000_common         16384  4 dib9000,dib7000m,dib3000mc,dvb_usb_dib0700
dvb_usb                28672  1 dvb_usb_dib0700
dvb_core              139264  3 dib9000,dvb_usb,mn88472
rc_core                53248  2 dvb_usb,dvb_usb_dib0700
mc                     53248  2 dvb_usb,dvb_core

[hselasky]

Can you check if the CONFIG_XXX for all those is present in webcamd?

[mkuron]

Yes, they are all present. I also checked that the tda18250 and mn88472 drivers are identical between the Ubuntu kernel and webcamd‘s media_tree.

[hselasky]

Can you also check if linux specifies any module parameters we don't?

[hselasky]

Okay, so the -32 comes from usb_control_msg. And that returns -EPIPE only when libusb20_dev_request_sync fails. That function's error code is -99, corresponding to LIBUSB20_ERROR_OTHER. This is so unspecific that I can't tell where inside libusb it's coming from.

However, taking one step back and applying the following patch to ensure that the data sent matches what was sent the first time actually resolves my issue!

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 70219b3e8566..a5865bc137f4 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -627,7 +627,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
                        st->channel_state |=    1 << (3-adap->fe_adap[0].stream.props.endpoint);
        }

-       st->buf[2] |= st->channel_state;
+       st->buf[2] |= (onoff ? 1 : 255);

        deb_info("data for streaming: %x %x\n", st->buf[1], st->buf[2]);

The first time, streaming is enabled and disabled with channel_state values of 1 and 255, respectively. My patch makes it do that every time. Without my patch, it tries enabling with 253 the second time. Now clearly this patch bypasses a bunch of logic that surely is there for a reason, but at least it seems to get things working for me. But since I know nothing about the dib0700, I don't think I can come up with anything better.

The code in the latest version of webcamd looks like this:

        if (st->disable_streaming_master_mode == 1)
                st->buf[2] = 0x00;
        else
                st->buf[2] = 0x01 << 4; /* Master mode */

How did you apply the patch you referred to?

Won't it fail to apply?

[mkuron]

The code you quote is https://github.com/torvalds/linux/blob/d07f6ca923ea0927a1024dfccafc5b53b61cfecc/drivers/media/usb/dvb-usb/dib0700_core.c#L606-L609, but my patch actually applies to https://github.com/torvalds/linux/blob/d07f6ca923ea0927a1024dfccafc5b53b61cfecc/drivers/media/usb/dvb-usb/dib0700_core.c#L630.

Can you also check if linux specifies any module parameters we don't?

no, nothing in the modprobe config

[hselasky]

I did:

git log -p drivers/media/usb/dvb-usb/dib0700_core.c

But cannot find anywhere this patch you mention.

Should we maybe consult: Mauro Carvalho Chehab mchehab@kernel.org about this?

[mchehab]

It's also strange that bugfixes are not upstreamed.

Yes. The one who sent such fix to Ubuntu should also submit it to linux-media@vger.kernel.org.

[mchehab]

The first time, streaming is enabled and disabled with channel_state values of 1 and 255, respectively. My patch makes it do that every time. Without my patch, it tries enabling with 253 the second time. Now clearly this patch bypasses a bunch of logic that surely is there for a reason, but at least it seems to get things working for me. But since I know nothing about the dib0700, I don't think I can come up with anything better.

(Looking at Linux upstream tree) it seems weird that there's a bug there. I mean, the only place at dib0700 driver that ever touch channel_state is precisely this function:

        st->channel_state &= ~0x3;
        if ((adap->fe_adap[0].stream.props.endpoint != 2)
                        && (adap->fe_adap[0].stream.props.endpoint != 3)) {
                deb_info("the endpoint number (%i) is not correct, use the adapter id instead", adap->fe_adap[0].stream.props.endpoint);
                if (onoff)
                        st->channel_state |=    1 << (adap->id);
                else
                        st->channel_state |=    1 << ~(adap->id);
        } else {
                if (onoff)
                        st->channel_state |=    1 << (adap->fe_adap[0].stream.props.endpoint-2);
                else
                        st->channel_state |=    1 << (3-adap->fe_adap[0].stream.props.endpoint);
        }

    st->buf[2] |= st->channel_state;

The number of endpoints should not change with time. The adap->id also should not change. So, every time this function is called, it should contain the same values. If it doesn't then, maybe there's a bug at the USB core.

[mkuron]

But cannot find anywhere this patch you mention.

Which patch do you mean? The st->buf[2] |= (onoff ? 1 : 255) from https://github.com/hselasky/webcamd/issues/16#issuecomment-917411383 is my own. I did not find any nontrivial differences between Ubuntu and upstream Linux. All patches contained in Ubuntu‘s kernel turned out to be backports. All fixes in upstream that were not backported were basically just maintenance stuff.

It's also strange that bugfixes are not upstreamed.

I believe you made that comment about the fix to remote-control code in https://github.com/hselasky/webcamd/issues/16#issuecomment-917639677. I did find out later that that was actually a backport from upstream, see the first paragraph of https://github.com/hselasky/webcamd/issues/16#issuecomment-920223536. Sorry about any confusion I may have caused here.

The one who sent such fix to Ubuntu should also submit it

see previous point: I did not find anything in Ubuntu that is not also in upstream

The number of endpoints should not change with time. The adap->id also should not change. So, every time this function is called, it should contain the same values. If it doesn't then, maybe there's a bug at the USB core.

Correct, and I have confirmed that they don‘t change. However, in the first line of the code quoted in https://github.com/hselasky/webcamd/issues/16#issuecomment-925957343, there is an &=. I have not been able to find out where the old value of channel_state going into the bitwise-and operation comes from, but it seems like that gets changed from elsewhere.

[hselasky]

The "st" structure is allocated by dvb_usb_init(). From what I can see kzalloc() is used, so the memory should be all zero, and (channel_state & 3) should be zero initially.

[mkuron]

Hmm, I‘ll run again with printfs in all places where channel_state is accessed and report back.

[mchehab]

It's also strange that bugfixes are not upstreamed.

Yes. The one who sent such fix to Ubuntu should also submit it to linux-media@vger.kernel.org.

But cannot find anywhere this patch you mention.

Which patch do you mean?

I mean: https://github.com/hselasky/webcamd/issues/16#issuecomment-917639677

[mchehab]

The number of endpoints should not change with time. The adap->id also should not change. So, every time this function is called, it should contain the same values. If it doesn't then, maybe there's a bug at the USB core.

Correct, and I have confirmed that they don‘t change. However, in the first line of the code quoted in #16 (comment), there is an &=. I have not been able to find out where the old value of channel_state going into the bitwise-and operation comes from, but it seems like that gets changed from elsewhere.

On Linux, data structs like that are initialized with kzalloc(), which means that the initial value for channel_state is 0.

The first time, streaming is enabled and disabled with channel_state values of 1 and 255, respectively.

A value of 255 looks really weird here. I would expect it to be either (0, 1) or (0, 2), as it is masking the value to use just two bits, keeping the high bits with 0.

It sounds to me that some other part of the driver is overriding the memory allocated for channel_state.

[mkuron]

I mean: https://github.com/hselasky/webcamd/issues/16#issuecomment-917639677

Yes, that one is upstreamed as https://github.com/torvalds/linux/commit/f52981019ad8d6718de79b425a574c6bddf81f7c

A value of 255 looks really weird here. I would expect it to be either (0, 1) or (0, 2)

I have not checked which values Linux ends up using. It‘s possible that they are different from what my webcamd patch uses, but they both work.

It sounds to me that some other part of the driver is overriding the memory allocated for channel_state.

Okay, I‘ll see if I can identify that part on the weekend.

[mkuron]

Okay, so the problem occurs when the tuner is switched off for the first time. It is the line st->channel_state |= 1 << ~(adap->id). We enter dib0700_streaming_ctrl with st->channel_state=1 and onoff=0 because the tuner has previously been turned on and we want to switch it off. The st->channel_state &= ~0x3 line sets st->channel_state=0. Then we take the the endpoint number is not correct branch because the endpoint number is 130, but the adapter ID is 0. This branch sets st->channel_state=255. st->buf[2], which is later bitwise OR-ed with st->channel_state, always contains 0 for me.

As an alternative to my patch from https://github.com/hselasky/webcamd/issues/16#issuecomment-917411383, the following patch also works:

                if (onoff)
                        st->channel_state |=    1 << (adap->id);
                else
-                       st->channel_state |=    1 << ~(adap->id);
+                       st->channel_state &=    1 << ~(adap->id);

Since the adapter ID probably isn't going to differ between webcamd and Linux, and these OR operations are bound to produce 255, my current suspicion is that Linux never takes the the endpoint number is not correct branch and thus does not run into the problem. @mchehab, do you know where these endpoint numbers come from and why they might be incorrect?

I have tracked down https://github.com/torvalds/linux/commit/7757ddda6f4febbc52342d82440dd4f7a7d4f14f from Linux 2.6.39, which introduced the endpoint-based branching. Looking at that patch, it changed st->channel_state &= ~(1 << adap->id) to st->channel_state |= 1 << ~(adap->id), which is definitely a change in functionality. And it's also what my new one-line patch reverts. One possible way how that wouldn't have broken anything on Linux is if that branch is never taken on Linux, but is taken on webcamd due to other differences.

[mchehab]

As an alternative to my patch from #16 (comment), the following patch also works:

                if (onoff)
                        st->channel_state |=    1 << (adap->id);
                else
-                       st->channel_state |=    1 << ~(adap->id);
+                       st->channel_state &=    1 << ~(adap->id);

Such patch makes sense to me. Could you test it on Linux and see if everything works?

I have tracked down torvalds/linux@7757ddd from Linux 2.6.39, which introduced the endpoint-based branching. Looking at that patch, it changed st->channel_state &= ~(1 << adap->id) to st->channel_state |= 1 << ~(adap->id), which is definitely a change in functionality. And it's also what my new one-line patch reverts. One possible way how that wouldn't have broken anything on Linux is if that branch is never taken on Linux, but is taken on webcamd due to other differences.

If the patch works properly on Linux, please submit it to linux-media@vger.kernel.org. General instructions about how to contribute to the Linux Kernel can be found at: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

When submitting your patch, please add:

Cc: stable@vger.kernel.org
Fixes: 7757ddda6f4f ("[media] DiB0700: add function to change I2C-speed")
Signed-off-by: Your Name <your@email>

at the end of the patch description, in order for it to be backported to the older Kernel releases and to document what patch it fixes.

[mkuron]

Such patch makes sense to me. Could you test it on Linux and see if everything works?

Will do, once I actually understand what is going on.

I rebuilt the Linux kernel module per the instructions of https://www.linuxtv.org/wiki/index.php/How_to_Obtain,_Build_and_Install_V4L-DVB_Device_Drivers. This proved wrong my previous suspicion that Linux might not takes the the endpoint number is not correct branch branch -- it does, just as webcamd does, yet it ends at a different st->channel_state value:

We enter dib0700_streaming_ctrl with st->channel_state=1 and onoff=0 because the tuner has previously been turned on and we want to switch it off. The st->channel_state &= ~0x3 line sets st->channel_state=0. Then we take the the endpoint number is not correct branch because the endpoint number is 130, but the adapter ID is 0. This branch somehow sets st->channel_state=0, which is different from what webcamd does with the exact same code. Why?

Looking at 1 << ~(adap->id) again: what is that even supposed to mean? adap->id is 0, so the bitwise-NOT of that is 255. Left-shifting 1 by 255 simply overflows, so the expression is 0. And that would also happen for any reasonably small value of adap->id. So we would be doing st->channel_state |= 0 here, which is a no-op. As a result, we could apply the even simpler patch

-             else
-                       st->channel_state |=    1 << ~(adap->id);

to fix the problem and eliminate a seemingly useless piece of code from the Linux kernel.

To understand what is happening in webcamd, I now applied the following patch:

-               else
+               else {
+                       deb_info("0x%x | 0x%x = 0x%x\n", (u8) st->channel_state, (u8) (1 << ~(adap->id)),
+                                                        (u8) (st->channel_state | (1 << ~(adap->id))));
                        st->channel_state |=    1 << ~(adap->id);
+               }

and it prints

webcamd 22334 - - 0x0 | 0x0 = 0xff

That's a very unexpected result. But it is consistent with our observations. Somehow, bitwise-OR behaves very oddly on FreeBSD. @hselasky, any idea why? @mchehab, would the patch that removes the two lines still qualify for upstreaming into Linux, even if Linux doesn't need it because its bitwise-OR behaves properly? It wouldn't need certainly backporting to older kernels, but perhaps the same issue might pop up depending on compiler version or architecture?

[mkuron]

Oh neat, I found a minimal working example of this bug with Clang on Linux with optimizations turned on: https://godbolt.org/z/rKdbG7Kvq. In this case, Clang 9 exhibits the bug and Clang 10 does not. I use Clang 10 on FreeBSD, but since it's optimization-related, it is quite likely that it will depend on surrounding code whether we hit the incorrect optimization or not. Or is there any undefined behavior in this code example? Overflowing a bitshift operation is permitted, right?

[mkuron]

Seems like we are hitting undefined behavior here: any version of Clang with -fsanitize=undefined says runtime error: shift exponent -1 is negative for the example from https://godbolt.org/z/rKdbG7Kvq. Indeed, the C11 standard says

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

meaning that my previous assumption that bitwise shifts may safely overflow was wrong. So it's pure coincidence that this ever worked on Linux. If the explanation satisfies you, @mchehab, I would now be happy to submit

-             else
-                       st->channel_state |=    1 << ~(adap->id);

as a patch. Before https://github.com/torvalds/linux/commit/7757ddd, there was no undefined behavior, so this is actually a regression introduced in 2.6.39.

[mchehab]

Well, 1 << n, where n is greater than 32/64 (depending if arch is 32 or 64) is indeed an undefined behaviour. Even gcc handles it different, depending on arch. For instance, on arm32, 1<<<32 (or greater) is a no-op expression.

The right fix here seems to simply partially revert the regression, e. g. the off logic should be:

st->channel_state &= ~(adap->id);

I suspect that people didn't notice it before because the new way of using the endpoint is probably the path taken with almost all adapters. The specific adapter you have likely has a problem on its USB descriptors, causing the issue.

[mkuron]

For adap->id equal to 0 or 1, deleting those two lines is equivalent to reverting the regression (because &= ~0x3 is done a few lines earlier, which was not done before the regression was introduced). Looking at the other uses of adap->id in that driver, I have a feeling that those are the only values it can handle anyway. However, if you think reverting is safer, then I'm happy to do that.

Looking at https://github.com/torvalds/linux/blob/master/drivers/media/usb/dvb-usb/dib0700_devices.c, indeed only .num_adapters = 1 and .num_adapters = 2 show up in the list.

[mkuron]

Patch submitted: media: dib0700: fix undefined behavior in tuner shutdown. @hselasky, once it is accepted, could you please add it to webcamd or upgrade media_tree to the next kernel version?

[mchehab]

For adap->id equal to 0 or 1, deleting those two lines is equivalent to reverting the regression (because &= ~0x3 is done a few lines earlier, which was not done before the regression was introduced). Looking at the other uses of adap->id in that driver, I have a feeling that those are the only values it can handle anyway. However, if you think reverting is safer, then I'm happy to do that.

Looking at https://github.com/torvalds/linux/blob/master/drivers/media/usb/dvb-usb/dib0700_devices.c, indeed only .num_adapters = 1 and .num_adapters = 2 show up in the list.

Good point.

In order to avoid confusion here, I'll use the word adapt_nr for the adapter number inside the hardware, and adap->id for the Linux ID for each adapter.

The original logic before 7757ddda6f4f ("[media] DiB0700: add function to change I2C-speed") was relying on the DVB probing order to identify what adapter is adapt_nr == 0. It basically assumes that the first probed adapter channel is adap->id == 0.

The new logic relies on the USB descriptors, with is a way more reliable. So, a device with multiple adapters (WinTV Nova TD) is shown as:

Bus 001 Device 019: ID 2040:5200 Hauppauge NovaT 500Stick
      Endpoint Descriptor:
        bEndpointAddress     0x82  EP 2 IN
      Endpoint Descriptor:
        bEndpointAddress     0x83  EP 3 IN

So (when disable_streaming_master_mode == 1), the endpoint-based logic should be doing this logic:

• adapt_nr == 0 -> stream.props.endpoint == 2
  • ON sequence: 0F 00 01 00
  • OFF sequence: 0F 10 02 00
• adapt_nr == 1 -> stream.props.endpoint == 3
  • ON sequence: 0F 00 02 00
  • OFF sequence: 0F 10 01 00

The logic when using the fall-back logic for devices with broken USB descriptors should do the same.

The enclosed patch would make it a lot clearer:

--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -583,7 +583,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 {
    struct dib0700_state *st = adap->dev->priv;
-   int ret;
+   int ret, adapt_nr;

    if ((onoff != 0) && (st->fw_version >= 0x10201)) {
        /* for firmware later than 1.20.1,
@@ -610,23 +610,23 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)

    st->buf[3] = 0x00;

-   deb_info("modifying (%d) streaming state for %d\n", onoff, adap->id);
-
-   st->channel_state &= ~0x3;
    if ((adap->fe_adap[0].stream.props.endpoint != 2)
-           && (adap->fe_adap[0].stream.props.endpoint != 3)) {
-       deb_info("the endpoint number (%i) is not correct, use the adapter id instead", adap->fe_adap[0].stream.props.endpoint);
-       if (onoff)
-           st->channel_state |=    1 << (adap->id);
-       else
-           st->channel_state |=    1 << ~(adap->id);
+       && (adap->fe_adap[0].stream.props.endpoint != 3)) {
+       deb_info("the endpoint number (%i) is not correct, use the adapter id instead\n",
+            adap->fe_adap[0].stream.props.endpoint);
+       adapt_nr = adap->id;
    } else {
-       if (onoff)
-           st->channel_state |=    1 << (adap->fe_adap[0].stream.props.endpoint-2);
-       else
-           st->channel_state |=    1 << (3-adap->fe_adap[0].stream.props.endpoint);
+       adapt_nr = adap->fe_adap[0].stream.props.endpoint - 2;
    }

+   deb_info("modifying (%d) streaming state for %d\n", onoff, adapt_nr);
+
+   st->channel_state &= ~0x3;
+   if (onoff)
+       st->channel_state |= 1 << adapt_nr;
+   else
+       st->channel_state |= 1 << (1 - adapt_nr);
+
    st->buf[2] |= st->channel_state;

    deb_info("data for streaming: %x %x\n", st->buf[1], st->buf[2]);

This sounds OK when there's just one adapter, but I'm not so sure if it would be 100% when multiple adapters are there. On such case, I would expect that just one of the bits would be touched. So, the st->channel_state &= ~0x3; would be causing a bug too.

So, maybe the right logic would be, instead:

@@ -621,11 +621,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)

        deb_info("modifying (%d) streaming state for %d\n", onoff, adapt_nr);

-       st->channel_state &= ~0x3;
        if (onoff)
                st->channel_state |= 1 << adapt_nr;
        else
-               st->channel_state |= 1 << (2 - adapt_nr);
+               st->channel_state &= ~(1 << adapt_nr);

        st->buf[2] |= st->channel_state;

(diff against the previous one)

But the only way to know for sure is to test this on a multi-adapter device. I'll see if I can do some tests here.

Anyway, your patch seems to be correct. Yet, I may apply some patches after it, after running some tests.

[mkuron]

Thanks for looking into this, @mchehab. Let me know if you do end up rewriting the adapt_nr-determining logic, I will be glad to test it with my dib0700 device.

[hselasky]

+1 Great reading and investigation!

All credits to you @mkuron !

[mchehab]

Thanks for looking into this, @mchehab. Let me know if you do end up rewriting the adapt_nr-determining logic, I will be glad to test it with my dib0700 device.

Could you please test this series? It starts with your patch, and has two patches on the top of it:

https://lore.kernel.org/linux-media/cover.1632689033.git.mchehab+huawei@kernel.org/T/#me4c8006db4c09e1028ff79814591120c171c95ec

It is also available on this branch:

https://github.com/mchehab/linux/commits/dib0700_fixes

[mkuron]

Your patch series works fine with the Xbox tuner. And the logic is actually comprehensible now, so it's a big improvement over the previous state of the dib0700 driver. There is one minor thing that I would appreciate if you could fix before this patch series gets accepted:

+   deb_info("adapter %d, streaming %s: %*ph\n",

That prints a pointer, not the data in the array pointed to, which I don't find particularly useful. Indeed, my log messages look like adapter 0, streaming ON: 0x8009b7668h, and not like adapter 0, streaming ON: 0f 10 13 as in the first message of your patch series.

[mchehab]

Your patch series works fine with the Xbox tuner. And the logic is actually comprehensible now, so it's a big improvement over the previous state of the dib0700 driver. There is one minor thing that I would appreciate if you could fix before this patch series gets accepted:

+ deb_info("adapter %d, streaming %s: %*ph\n",

That prints a pointer, not the data in the array pointed to, which I don't find particularly useful. Indeed, my log messages look like adapter 0, streaming ON: 0x8009b7668h, and not like adapter 0, streaming ON: 0f 10 13 as in the first message of your patch series.

I guess that this is because you're running it on FreeBSD. Linux defines some special printk formats:

https://www.kernel.org/doc/html/latest/core-api/printk-formats.html?highlight=printk#raw-buffer-as-a-hex-string

So, either "%*ph" should be ported to webcamd or some webcamd-specific change is needed.

[mkuron]

Ah, I wasn‘t aware of that, I only knew standard printf syntax. In that case, I have no objections against your patch series. Thanks for all your work!

[mchehab]

Linux defines some special printk formats:

I actually meant to say "Linux Kernel". Those are there to help Kernel developers and avoid code duplication. Userspace printf should be identical ;-)

Ah, I wasn‘t aware of that, I only knew standard printf syntax. In that case, I have no objections against your patch series. Thanks for all your work!

Thank you for your help with that.