All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Fix streaming on/off logic
@ 2021-09-26 20:51 Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 1/3] media: dib0700: fix undefined behavior in tuner shutdown Mauro Carvalho Chehab
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-26 20:51 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, pb, linux-kernel,
	linux-media

As discussed on:
	https://github.com/hselasky/webcamd/issues/16

the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
affect most devices, in practice, as it seems to happen only under
certain circunstances. 

Michael came up with a solution for the issue (already submitted to
the ML) but let's take the opportunity to do a cleanup, as the resulting
code was still touching both adapters when an stream off command
was issued to one adapter, turning on the other one.

After the change, each adapter is idependently controlled by
a separate bit, as can be shown when its debug message 
is turned on (tested on a dual-adapter device: Hauppauge
WinTV Nova TD):

[608855.124780] adapter 1, streaming ON: 0f 10 12
[608868.189827] adapter 0, streaming ON: 0f 10 13
[608879.584330] adapter 1, streaming OFF: 0f 00 11
[608887.014772] adapter 0, streaming OFF: 0f 00 10

Mauro Carvalho Chehab (2):
  media: dib0700: cleanup start/stop streaming logic
  media: dib0700: Only touch one bit when start/stop an adapter

Michael Kuron (1):
  media: dib0700: fix undefined behavior in tuner shutdown

 drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
 1 file changed, 13 insertions(+), 15 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] media: dib0700: fix undefined behavior in tuner shutdown
  2021-09-26 20:51 [PATCH 0/3] Fix streaming on/off logic Mauro Carvalho Chehab
@ 2021-09-26 20:51 ` Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 2/3] media: dib0700: cleanup start/stop streaming logic Mauro Carvalho Chehab
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-26 20:51 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Michael Kuron, Mauro Carvalho Chehab,
	Olivier Grenie, Patrick Boettcher, linux-kernel, linux-media, pb,
	stable, Mauro Carvalho Chehab

From: Michael Kuron <michael.kuron@gmail.com>

This fixes a problem where closing the tuner would leave it in a state
where it would not tune to any channel when reopened. This problem was
discovered as part of https://github.com/hselasky/webcamd/issues/16.

Since adap->id is 0 or 1, this bit-shift overflows, which is undefined
behavior. The driver still worked in practice as the overflow would in
most environments result in 0, which rendered the line a no-op. When
running the driver as part of webcamd however, the overflow could lead to
0xff due to optimizations by the compiler, which would, in the end,
improperly shut down the tuner.

The bug is a regression introduced in the commit referenced below. The
present patch causes identical behavior to before that commit for adap->id
equal to 0 or 1. The driver does not contain support for dib0700 devices
with more adapters, assuming such even exist.

Tests have been performed with the Xbox One Digital TV Tuner on amd64. Not
all dib0700 devices are expected to be affected by the regression; this
code path is only taken by those with incorrect endpoint numbers.

Cc: stable@vger.kernel.org
Fixes: 7757ddda6f4f ("[media] DiB0700: add function to change I2C-speed")
Signed-off-by: Michael Kuron <michael.kuron@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 70219b3e8566..7ea8f68b0f45 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -618,8 +618,6 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 		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);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] media: dib0700: cleanup start/stop streaming logic
  2021-09-26 20:51 [PATCH 0/3] Fix streaming on/off logic Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 1/3] media: dib0700: fix undefined behavior in tuner shutdown Mauro Carvalho Chehab
@ 2021-09-26 20:51 ` Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 3/3] media: dib0700: Only touch one bit when start/stop an adapter Mauro Carvalho Chehab
  2021-12-21  6:34 ` [PATCH 0/3] Fix streaming on/off logic Hans Petter Selasky
  3 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-26 20:51 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Mauro Carvalho Chehab, Michael Kuron, linux-kernel, linux-media,
	pb

Having two different pathes to start/stop streaming, depending
weather the USB endpoints are 0x82/0x83 or not makes it more
prune to errors.

Unify the logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 26 +++++++++++++-----------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 7ea8f68b0f45..d7c5836b9271 100644
--- 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,24 +610,26 @@ 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);
+	    && (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;
 	}
 
+	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]);
+	deb_info("adapter %d, streaming %s: %*ph\n",
+		adapt_nr, onoff ? "ON" : "OFF", 3, st->buf);
 
 	ret = dib0700_ctrl_wr(adap->dev, st->buf, 4);
 	mutex_unlock(&adap->dev->usb_mutex);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] media: dib0700: Only touch one bit when start/stop an adapter
  2021-09-26 20:51 [PATCH 0/3] Fix streaming on/off logic Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 1/3] media: dib0700: fix undefined behavior in tuner shutdown Mauro Carvalho Chehab
  2021-09-26 20:51 ` [PATCH 2/3] media: dib0700: cleanup start/stop streaming logic Mauro Carvalho Chehab
@ 2021-09-26 20:51 ` Mauro Carvalho Chehab
  2021-12-21  6:34 ` [PATCH 0/3] Fix streaming on/off logic Hans Petter Selasky
  3 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-26 20:51 UTC (permalink / raw)
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Mauro Carvalho Chehab, Michael Kuron, linux-kernel, linux-media,
	pb

Only touch the right bit to enable/disable an adapter channel,
without touching the other adapter's one.

Tested on Nova-TD.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index d7c5836b9271..1caabb51ea47 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -610,8 +610,6 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 
 	st->buf[3] = 0x00;
 
-	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\n",
@@ -624,7 +622,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 	if (onoff)
 		st->channel_state |= 1 << adapt_nr;
 	else
-		st->channel_state |= 1 << (1 - adapt_nr);
+		st->channel_state &= ~(1 << adapt_nr);
 
 	st->buf[2] |= st->channel_state;
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Fix streaming on/off logic
  2021-09-26 20:51 [PATCH 0/3] Fix streaming on/off logic Mauro Carvalho Chehab
                   ` (2 preceding siblings ...)
  2021-09-26 20:51 ` [PATCH 3/3] media: dib0700: Only touch one bit when start/stop an adapter Mauro Carvalho Chehab
@ 2021-12-21  6:34 ` Hans Petter Selasky
  2021-12-22 11:50   ` Mauro Carvalho Chehab
  3 siblings, 1 reply; 6+ messages in thread
From: Hans Petter Selasky @ 2021-12-21  6:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linuxarm, mauro.chehab, pb, linux-kernel, linux-media

On 9/26/21 22:51, Mauro Carvalho Chehab wrote:
> As discussed on:
> 	https://github.com/hselasky/webcamd/issues/16
> 
> the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
> affect most devices, in practice, as it seems to happen only under
> certain circunstances.
> 
> Michael came up with a solution for the issue (already submitted to
> the ML) but let's take the opportunity to do a cleanup, as the resulting
> code was still touching both adapters when an stream off command
> was issued to one adapter, turning on the other one.
> 
> After the change, each adapter is idependently controlled by
> a separate bit, as can be shown when its debug message
> is turned on (tested on a dual-adapter device: Hauppauge
> WinTV Nova TD):
> 
> [608855.124780] adapter 1, streaming ON: 0f 10 12
> [608868.189827] adapter 0, streaming ON: 0f 10 13
> [608879.584330] adapter 1, streaming OFF: 0f 00 11
> [608887.014772] adapter 0, streaming OFF: 0f 00 10
> 
> Mauro Carvalho Chehab (2):
>    media: dib0700: cleanup start/stop streaming logic
>    media: dib0700: Only touch one bit when start/stop an adapter
> 
> Michael Kuron (1):
>    media: dib0700: fix undefined behavior in tuner shutdown
> 
>   drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 

Were these patches upstreamed yet?

--HPS

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Fix streaming on/off logic
  2021-12-21  6:34 ` [PATCH 0/3] Fix streaming on/off logic Hans Petter Selasky
@ 2021-12-22 11:50   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-12-22 11:50 UTC (permalink / raw)
  To: Hans Petter Selasky; +Cc: linuxarm, mauro.chehab, pb, linux-kernel, linux-media

Em Tue, 21 Dec 2021 07:34:27 +0100
Hans Petter Selasky <hps@selasky.org> escreveu:

> On 9/26/21 22:51, Mauro Carvalho Chehab wrote:
> > As discussed on:
> > 	https://github.com/hselasky/webcamd/issues/16
> > 
> > the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
> > affect most devices, in practice, as it seems to happen only under
> > certain circunstances.
> > 
> > Michael came up with a solution for the issue (already submitted to
> > the ML) but let's take the opportunity to do a cleanup, as the resulting
> > code was still touching both adapters when an stream off command
> > was issued to one adapter, turning on the other one.
> > 
> > After the change, each adapter is idependently controlled by
> > a separate bit, as can be shown when its debug message
> > is turned on (tested on a dual-adapter device: Hauppauge
> > WinTV Nova TD):
> > 
> > [608855.124780] adapter 1, streaming ON: 0f 10 12
> > [608868.189827] adapter 0, streaming ON: 0f 10 13
> > [608879.584330] adapter 1, streaming OFF: 0f 00 11
> > [608887.014772] adapter 0, streaming OFF: 0f 00 10
> > 
> > Mauro Carvalho Chehab (2):
> >    media: dib0700: cleanup start/stop streaming logic
> >    media: dib0700: Only touch one bit when start/stop an adapter
> > 
> > Michael Kuron (1):
> >    media: dib0700: fix undefined behavior in tuner shutdown
> > 
> >   drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
> >   1 file changed, 13 insertions(+), 15 deletions(-)
> >   
> 
> Were these patches upstreamed yet?

Those were merged on media upstream tree. They should likely be
merged upstream before v5.17-rc1.

> 
> --HPS



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-22 11:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26 20:51 [PATCH 0/3] Fix streaming on/off logic Mauro Carvalho Chehab
2021-09-26 20:51 ` [PATCH 1/3] media: dib0700: fix undefined behavior in tuner shutdown Mauro Carvalho Chehab
2021-09-26 20:51 ` [PATCH 2/3] media: dib0700: cleanup start/stop streaming logic Mauro Carvalho Chehab
2021-09-26 20:51 ` [PATCH 3/3] media: dib0700: Only touch one bit when start/stop an adapter Mauro Carvalho Chehab
2021-12-21  6:34 ` [PATCH 0/3] Fix streaming on/off logic Hans Petter Selasky
2021-12-22 11:50   ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.