[PATCH 1/3] rtl28xxu: lower the rc poll time to mitigate i2c transfer errors

[PATCH 1/3] rtl28xxu: lower the rc poll time to mitigate i2c transfer errors

[Benjamin Larsson]

The Astrometa device has issues with i2c transfers. Lowering the
poll time somehow makes these errors disappear.

Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
---
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index 705c6c3..9ec4223 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -1567,7 +1567,7 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d,
 	rc->allowed_protos = RC_BIT_ALL;
 	rc->driver_type = RC_DRIVER_IR_RAW;
 	rc->query = rtl2832u_rc_query;
-	rc->interval = 400;
+	rc->interval = 200;
 
 	return 0;
 }
-- 
1.9.1

[PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Benjamin Larsson]

Using this driver with the attach dvb model might trigger a use
after free when unloading the driver. With this change the driver
will always fail on unload instead of randomly crash depending
on if the memory has been reused or not.

Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
---
 drivers/staging/media/mn88472/mn88472.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c
index 36ef39b..a9d5f0a 100644
--- a/drivers/staging/media/mn88472/mn88472.c
+++ b/drivers/staging/media/mn88472/mn88472.c
@@ -489,6 +489,7 @@ static int mn88472_remove(struct i2c_client *client)
 
 	regmap_exit(dev->regmap[0]);
 
+	memset(dev, 0, sizeof(*dev));
 	kfree(dev);
 
 	return 0;
-- 
1.9.1

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Antti Palosaari]

But that is not needed anymore ?

regards
Antti

On 12/06/2014 02:25 AM, Benjamin Larsson wrote:
> Using this driver with the attach dvb model might trigger a use
> after free when unloading the driver. With this change the driver
> will always fail on unload instead of randomly crash depending
> on if the memory has been reused or not.
>
> Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
> ---
>   drivers/staging/media/mn88472/mn88472.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c
> index 36ef39b..a9d5f0a 100644
> --- a/drivers/staging/media/mn88472/mn88472.c
> +++ b/drivers/staging/media/mn88472/mn88472.c
> @@ -489,6 +489,7 @@ static int mn88472_remove(struct i2c_client *client)
>
>   	regmap_exit(dev->regmap[0]);
>
> +	memset(dev, 0, sizeof(*dev));
>   	kfree(dev);
>
>   	return 0;
>

-- 
http://palosaari.fi/

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Benjamin Larsson]

On 12/06/2014 05:29 PM, Antti Palosaari wrote:
> But that is not needed anymore ?
>
> regards
> Antti

Chances are that more devices with the mn8847x chips appear. Someone 
somewhere might try to use this demod with the old dvb attach model 
during development. Adding this memset will make the unload issue appear 
all the time instead of randomly. But this patch doesn't really matter 
so feel free to NACK it. I just wanted to post it for completeness.

I do think it is good practice to set pointers to null generally as that 
would have saved me several days of work of whentracking down this bug. 
The current dvb framework contain several other cases where pointers are 
feed'd but not nulled.

MvH
Benjamin Larsson

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Antti Palosaari]

On 12/06/2014 08:08 PM, Benjamin Larsson wrote:
> On 12/06/2014 05:29 PM, Antti Palosaari wrote:
>> But that is not needed anymore ?
>>
>> regards
>> Antti
>
> Chances are that more devices with the mn8847x chips appear. Someone
> somewhere might try to use this demod with the old dvb attach model
> during development. Adding this memset will make the unload issue appear
> all the time instead of randomly. But this patch doesn't really matter
> so feel free to NACK it. I just wanted to post it for completeness.
>
> I do think it is good practice to set pointers to null generally as that
> would have saved me several days of work of whentracking down this bug.
> The current dvb framework contain several other cases where pointers are
> feed'd but not nulled.

There is kzfree() for that, but still I am very unsure should we start 
zeroing memory upon release driver has allocated, or just relase it 
using kfree.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Benjamin Larsson]

On 12/06/2014 07:37 PM, Antti Palosaari wrote:
>>
>> I do think it is good practice to set pointers to null generally as that
>> would have saved me several days of work of whentracking down this bug.
>> The current dvb framework contain several other cases where pointers are
>> feed'd but not nulled.
>
> There is kzfree() for that, but still I am very unsure should we start 
> zeroing memory upon release driver has allocated, or just relase it 
> using kfree.
>
> regards
> Antti 

Well I guess I am biased as I have spent lots of time finding a bug that 
probably wouldn't exist if the policy was that drivers always should set 
their memory to zero before it is free'd. Maybe we should have a compile 
time override so that all free calls zeroes the memory before the actual 
free? Maybe there already is this kind of feature?

MvH
Benjamin Larsson

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Hans Verkuil]

On 12/06/2014 09:26 PM, Benjamin Larsson wrote:
> On 12/06/2014 07:37 PM, Antti Palosaari wrote:
>>>
>>> I do think it is good practice to set pointers to null generally as that
>>> would have saved me several days of work of whentracking down this bug.
>>> The current dvb framework contain several other cases where pointers are
>>> feed'd but not nulled.
>>
>> There is kzfree() for that, but still I am very unsure should we start 
>> zeroing memory upon release driver has allocated, or just relase it 
>> using kfree.
>>
>> regards
>> Antti 
> 
> Well I guess I am biased as I have spent lots of time finding a bug that 
> probably wouldn't exist if the policy was that drivers always should set 
> their memory to zero before it is free'd.

Just because you zero memory before it is freed doesn't mean it stays zeroed.
As soon as it is freed some other process might take that memory and fill it
up again. So zeroing is pointless and in fact will only *hide* bugs.

The only reason I know of for zeroing memory before freeing is if that memory
contains sensitive information and you want to make sure it is gone from memory.

You can turn on the kmemcheck kernel option when compiling the kernel to test
for accesses to uninitialized memory if you suspect you have a bug in that
area.

Anyway:

Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> Maybe we should have a compile 
> time override so that all free calls zeroes the memory before the actual 
> free? Maybe there already is this kind of feature?
> 
> MvH
> Benjamin Larsson
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] mn88472: make sure the private data struct is nulled after free

[Benjamin Larsson]

On 01/19/2015 02:15 PM, Hans Verkuil wrote:
> On 12/06/2014 09:26 PM, Benjamin Larsson wrote:
>>
>> Well I guess I am biased as I have spent lots of time finding a bug that
>> probably wouldn't exist if the policy was that drivers always should set
>> their memory to zero before it is free'd.
>
> Just because you zero memory before it is freed doesn't mean it stays zeroed.
> As soon as it is freed some other process might take that memory and fill it
> up again. So zeroing is pointless and in fact will only *hide* bugs.
>

Well in this specific case NOT zeroing the memory it actually hid a use 
after free bug. So stating that it is pointless and that it will only 
hide bugs is not correct at least for this case.

MvH
Benjamin Larsson

[PATCH 3/3] mn88472: add ts mode and ts clock to driver

[Benjamin Larsson]

Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
---
 drivers/media/dvb-frontends/mn88472.h        | 12 +++++++++++
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c      |  2 ++
 drivers/staging/media/mn88472/mn88472.c      | 30 ++++++++++++++++++++++++++--
 drivers/staging/media/mn88472/mn88472_priv.h |  2 ++
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/mn88472.h b/drivers/media/dvb-frontends/mn88472.h
index e4e0b80..095294d 100644
--- a/drivers/media/dvb-frontends/mn88472.h
+++ b/drivers/media/dvb-frontends/mn88472.h
@@ -19,6 +19,16 @@
 
 #include <linux/dvb/frontend.h>
 
+enum ts_clock {
+	VARIABLE_TS_CLOCK,
+	FIXED_TS_CLOCK,
+};
+
+enum ts_mode {
+	SERIAL_TS_MODE,
+	PARALLEL_TS_MODE,
+};
+
 struct mn88472_config {
 	/*
 	 * Max num of bytes given I2C adapter could write at once.
@@ -39,6 +49,8 @@ struct mn88472_config {
 	 * Hz
 	 */
 	u32 xtal;
+	int ts_mode;
+	int ts_clock;
 };
 
 #endif
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index 9ec4223..663583b 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -869,6 +869,8 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter *adap)
 			mn88472_config.i2c_wr_max = 22,
 			strlcpy(info.type, "mn88472", I2C_NAME_SIZE);
 			mn88472_config.xtal = 20500000;
+			mn88472_config.ts_mode = SERIAL_TS_MODE;
+			mn88472_config.ts_clock = VARIABLE_TS_CLOCK;
 			info.addr = 0x18;
 			info.platform_data = &mn88472_config;
 			request_module(info.type);
diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c
index a9d5f0a..c6895ee 100644
--- a/drivers/staging/media/mn88472/mn88472.c
+++ b/drivers/staging/media/mn88472/mn88472.c
@@ -188,8 +188,32 @@ static int mn88472_set_frontend(struct dvb_frontend *fe)
 
 	ret = regmap_write(dev->regmap[0], 0x46, 0x00);
 	ret = regmap_write(dev->regmap[0], 0xae, 0x00);
-	ret = regmap_write(dev->regmap[2], 0x08, 0x1d);
-	ret = regmap_write(dev->regmap[0], 0xd9, 0xe3);
+
+	switch (dev->ts_mode) {
+	case SERIAL_TS_MODE:
+		ret = regmap_write(dev->regmap[2], 0x08, 0x1d);
+		break;
+	case PARALLEL_TS_MODE:
+		ret = regmap_write(dev->regmap[2], 0x08, 0x00);
+		break;
+	default:
+		dev_dbg(&client->dev, "ts_mode error: %d\n", dev->ts_mode);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	switch (dev->ts_clock) {
+	case VARIABLE_TS_CLOCK:
+		ret = regmap_write(dev->regmap[0], 0xd9, 0xe3);
+		break;
+	case FIXED_TS_CLOCK:
+		ret = regmap_write(dev->regmap[0], 0xd9, 0xe1);
+		break;
+	default:
+		dev_dbg(&client->dev, "ts_clock error: %d\n", dev->ts_clock);
+		ret = -EINVAL;
+		goto err;
+	}
 
 	/* Reset demod */
 	ret = regmap_write(dev->regmap[2], 0xf8, 0x9f);
@@ -406,6 +430,8 @@ static int mn88472_probe(struct i2c_client *client,
 
 	dev->i2c_wr_max = config->i2c_wr_max;
 	dev->xtal = config->xtal;
+	dev->ts_mode = config->ts_mode;
+	dev->ts_clock = config->ts_clock;
 	dev->client[0] = client;
 	dev->regmap[0] = regmap_init_i2c(dev->client[0], &regmap_config);
 	if (IS_ERR(dev->regmap[0])) {
diff --git a/drivers/staging/media/mn88472/mn88472_priv.h b/drivers/staging/media/mn88472/mn88472_priv.h
index b12b731..9ba8c8b 100644
--- a/drivers/staging/media/mn88472/mn88472_priv.h
+++ b/drivers/staging/media/mn88472/mn88472_priv.h
@@ -32,6 +32,8 @@ struct mn88472_dev {
 	fe_delivery_system_t delivery_system;
 	bool warm; /* FW running */
 	u32 xtal;
+	int ts_mode;
+	int ts_clock;
 };
 
 #endif
-- 
1.9.1

Re: [PATCH 3/3] mn88472: add ts mode and ts clock to driver

[Antti Palosaari]

On 12/06/2014 02:25 AM, Benjamin Larsson wrote:
> Signed-off-by: Benjamin Larsson <benjamin@southpole.se>

Reviewed-by: Antti Palosaari <crope@iki.fi>

Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/3] rtl28xxu: lower the rc poll time to mitigate i2c transfer errors

[Antti Palosaari]

Moikka!
I am very surprised about that patch, especially because it *increases* 
polling interval from 400ms to 200ms. For me it has been always worked 
rather well, but now I suspect it could be due to I disable always 
remote controller... I have to test that.

regards
Antti

On 12/06/2014 02:25 AM, Benjamin Larsson wrote:
> The Astrometa device has issues with i2c transfers. Lowering the
> poll time somehow makes these errors disappear.
>
> Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
> ---
>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> index 705c6c3..9ec4223 100644
> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> @@ -1567,7 +1567,7 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d,
>   	rc->allowed_protos = RC_BIT_ALL;
>   	rc->driver_type = RC_DRIVER_IR_RAW;
>   	rc->query = rtl2832u_rc_query;
> -	rc->interval = 400;
> +	rc->interval = 200;
>
>   	return 0;
>   }
>

-- 
http://palosaari.fi/

Re: [PATCH 1/3] rtl28xxu: lower the rc poll time to mitigate i2c transfer errors

[Benjamin Larsson]

On 12/06/2014 01:46 PM, Antti Palosaari wrote:
> Moikka!
> I am very surprised about that patch, especially because it 
> *increases* polling interval from 400ms to 200ms. For me it has been 
> always worked rather well, but now I suspect it could be due to I 
> disable always remote controller... I have to test that.
>
> regards
> Antti

I noticed that I got more retry errors when I removed the poll. So when 
I tried lowering the interval time the errors totally disappeared for 
me. Exactly how it works is unclear to me but I guess that the rc poll 
triggers something in the chip to mitigate some overflow in the i2c 
transmit buffer. This workaround also suggest that the i2c bus actually 
is ok and not the cause for the errors. Anyway please test and if this 
is an acceptable solution then there might also be some check that this 
poll is active and set to 200 at all times for this card. At least when 
the card is set in dvb mode, for sdr mode this might not be an issue.

MvH
Benjamin Larsson

Re: [PATCH 1/3] rtl28xxu: lower the rc poll time to mitigate i2c transfer errors

[Benjamin Larsson]

On 2014-12-06 01:25, Benjamin Larsson wrote:
> The Astrometa device has issues with i2c transfers. Lowering the
> poll time somehow makes these errors disappear.
>
> Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
> ---
>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> index 705c6c3..9ec4223 100644
> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> @@ -1567,7 +1567,7 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d,
>   	rc->allowed_protos = RC_BIT_ALL;
>   	rc->driver_type = RC_DRIVER_IR_RAW;
>   	rc->query = rtl2832u_rc_query;
> -	rc->interval = 400;
> +	rc->interval = 200;
>
>   	return 0;
>   }
>

Ping, can I get an ack or nack on this ?

MvH
Benjamin Larsson