af9015 Remove call to get config from probe.

PATCH In review Malcolm Priestley 2 participants 6 messages Nov 12, 2011 – Nov 13, 2011 Media (V4L/DVB)

lore

Share & tools

lore

Malcolm Priestley

author

2011-11-12 15:55 UTC #0 lore raw

PATCH 2/7 af9015 Remove call to get config from probe.

Remove get config from probe and move to identify_state.

intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.

Calling from probe seems to cause a race condition with some USB controllers.

The first call fails as the device appears to busy with USB sub system control calls.

Signed-off-by Malcolm Priestley <[email protected]>

---
 drivers/media/dvb/dvb-usb/af9015.c |   87 ++++++++++++++---------------------
 1 files changed, 35 insertions(+), 52 deletions(-)

drivers/media/dvb/dvb-usb/af9015.c +35 ~~-52~~

index dc6e4ec..eb464c8 100644
@@ -820,7 +820,8 @@ static void af9015_set_remote_config(struct usb_device *udev,
 	return;
 }
 
-static int af9015_read_config(struct usb_device *udev)
+static int af9015_read_config(struct usb_device *udev,
+		struct dvb_usb_device_properties *props)
 {
 	int ret;
 	u8 val, i, offset = 0;

@@ -842,12 +843,10 @@ static int af9015_read_config(struct usb_device *udev)
 		goto error;
 
 	deb_info("%s: IR mode:%d\n", __func__, val);
-	for (i = 0; i < af9015_properties_count; i++) {
-		if (val == AF9015_IR_MODE_DISABLED)
-			af9015_properties[i].rc.core.rc_codes = NULL;
-		else
-			af9015_set_remote_config(udev, &af9015_properties[i]);
-	}
+	if (val == AF9015_IR_MODE_DISABLED)
+		props->rc.core.rc_codes = NULL;
+	else
+		af9015_set_remote_config(udev, props);
 
 	/* TS mode - one or two receivers */
 	req.addr = AF9015_EEPROM_TS_MODE;

@@ -859,18 +858,16 @@ static int af9015_read_config(struct usb_device *udev)
 
 	/* Set adapter0 buffer size according to USB port speed, adapter1 buffer
 	   size can be static because it is enabled only USB2.0 */
-	for (i = 0; i < af9015_properties_count; i++) {
-		/* USB1.1 set smaller buffersize and disable 2nd adapter */
-		if (udev->speed == USB_SPEED_FULL) {
-			af9015_properties[i].adapter[0].fe[0].stream.u.bulk.buffersize
-				= TS_USB11_FRAME_SIZE;
-			/* disable 2nd adapter because we don't have
+	/* USB1.1 set smaller buffersize and disable 2nd adapter */
+	if (udev->speed == USB_SPEED_FULL) {
+		props->adapter[0].fe[0].stream.u.bulk.buffersize
+			= TS_USB11_FRAME_SIZE;
+		/* disable 2nd adapter because we don't have
 			   PID-filters */
-			af9015_config.dual_mode = 0;
-		} else {
-			af9015_properties[i].adapter[0].fe[0].stream.u.bulk.buffersize
-				= TS_USB20_FRAME_SIZE;
-		}
+		af9015_config.dual_mode = 0;
+	} else {
+		props->adapter[0].fe[0].stream.u.bulk.buffersize
+			= TS_USB20_FRAME_SIZE;
 	}
 
 	if (af9015_config.dual_mode) {

@@ -882,16 +879,11 @@ static int af9015_read_config(struct usb_device *udev)
 		af9015_af9013_config[1].demod_address = val;
 
 		/* enable 2nd adapter */
-		for (i = 0; i < af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 2;
+		props->num_adapters = 2;
+	} else /* disable 2nd adapter */
+		props->num_adapters = 1;
 
-	} else {
-		 /* disable 2nd adapter */
-		for (i = 0; i < af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 1;
-	}
-
-	for (i = 0; i < af9015_properties[0].num_adapters; i++) {
+	for (i = 0; i < props->num_adapters; i++) {
 		if (i == 1)
 			offset = AF9015_EEPROM_OFFSET;
 		/* xtal */

@@ -995,8 +987,7 @@ error:
 		/* disable dual mode */
 		af9015_config.dual_mode = 0;
 		 /* disable 2nd adapter */
-		for (i = 0; i < af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 1;
+		props->num_adapters = 1;
 
 		/* set correct IF */
 		af9015_af9013_config[0].tuner_if = 4570;

@@ -1014,6 +1005,10 @@ static int af9015_identify_state(struct usb_device *udev,
 	u8 reply;
 	struct req_t req = {GET_CONFIG, 0, 0, 0, 0, 1, &reply};
 
+	ret = af9015_read_config(udev, props);
+	if (ret)
+		return ret;
+
 	ret = af9015_rw_udev(udev, &req);
 	if (ret)
 		return ret;

@@ -1675,33 +1670,21 @@ static int af9015_usb_probe(struct usb_interface *intf,
 {
 	int ret = 0;
 	struct dvb_usb_device *d = NULL;
-	struct usb_device *udev = interface_to_usbdev(intf);
 	u8 i;
 
-	deb_info("%s: interface:%d\n", __func__,
-		intf->cur_altsetting->desc.bInterfaceNumber);
-
-	/* interface 0 is used by DVB-T receiver and
-	   interface 1 is for remote controller (HID) */
-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
-		ret = af9015_read_config(udev);
-		if (ret)
-			return ret;
-
-		for (i = 0; i < af9015_properties_count; i++) {
-			ret = dvb_usb_device_init(intf, &af9015_properties[i],
-				THIS_MODULE, &d, adapter_nr);
-			if (!ret)
-				break;
-			if (ret != -ENODEV)
-				return ret;
-		}
-		if (ret)
+	for (i = 0; i < af9015_properties_count; i++) {
+		ret = dvb_usb_device_init(intf, &af9015_properties[i],
+			THIS_MODULE, &d, adapter_nr);
+		if (!ret)
+			break;
+		if (ret != -ENODEV)
 			return ret;
-
-		if (d)
-			ret = af9015_init(d);
 	}
+	if (ret)
+		return ret;
+
+	if (d)
+		ret = af9015_init(d);
 
 	return ret;
 }

Signature · 1 line

1.7.5.4

Antti Palosaari

2011-11-12 16:18 UTC #1 lore raw

Quoted 4 lines · Malcolm Priestley

Remove get config from probe and move to identify_state.
intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.

Are you sure? IIRC there is HID remote on interface 1 or 2 or so (some other than 0). Please double check.

Quoted 1 line

Calling from probe seems to cause a race condition with some USB controllers.

Why?

Unless reasons are not clear it should not be moved.

Antti

Quoted 154 lines

The first call fails as the device appears to busy with USB sub system control calls.

Signed-off-by Malcolm Priestley<[email protected]>

---
  drivers/media/dvb/dvb-usb/af9015.c |   87 ++++++++++++++---------------------
  1 files changed, 35 insertions(+), 52 deletions(-)

drivers/media/dvb/dvb-usb/af9015.c +35 ~~-52~~

index dc6e4ec..eb464c8 100644
@@ -820,7 +820,8 @@ static void af9015_set_remote_config(struct usb_device *udev,
  	return;
  }
 
-static int af9015_read_config(struct usb_device *udev)
+static int af9015_read_config(struct usb_device *udev,
+		struct dvb_usb_device_properties *props)
  {
  	int ret;
  	u8 val, i, offset = 0;

@@ -842,12 +843,10 @@ static int af9015_read_config(struct usb_device *udev)
  		goto error;
 
  	deb_info("%s: IR mode:%d\n", __func__, val);
-	for (i = 0; i<  af9015_properties_count; i++) {
-		if (val == AF9015_IR_MODE_DISABLED)
-			af9015_properties[i].rc.core.rc_codes = NULL;
-		else
-			af9015_set_remote_config(udev,&af9015_properties[i]);
-	}
+	if (val == AF9015_IR_MODE_DISABLED)
+		props->rc.core.rc_codes = NULL;
+	else
+		af9015_set_remote_config(udev, props);
 
  	/* TS mode - one or two receivers */
  	req.addr = AF9015_EEPROM_TS_MODE;

@@ -859,18 +858,16 @@ static int af9015_read_config(struct usb_device *udev)
 
  	/* Set adapter0 buffer size according to USB port speed, adapter1 buffer
  	   size can be static because it is enabled only USB2.0 */
-	for (i = 0; i<  af9015_properties_count; i++) {
-		/* USB1.1 set smaller buffersize and disable 2nd adapter */
-		if (udev->speed == USB_SPEED_FULL) {
-			af9015_properties[i].adapter[0].fe[0].stream.u.bulk.buffersize
-				= TS_USB11_FRAME_SIZE;
-			/* disable 2nd adapter because we don't have
+	/* USB1.1 set smaller buffersize and disable 2nd adapter */
+	if (udev->speed == USB_SPEED_FULL) {
+		props->adapter[0].fe[0].stream.u.bulk.buffersize
+			= TS_USB11_FRAME_SIZE;
+		/* disable 2nd adapter because we don't have
  			   PID-filters */
-			af9015_config.dual_mode = 0;
-		} else {
-			af9015_properties[i].adapter[0].fe[0].stream.u.bulk.buffersize
-				= TS_USB20_FRAME_SIZE;
-		}
+		af9015_config.dual_mode = 0;
+	} else {
+		props->adapter[0].fe[0].stream.u.bulk.buffersize
+			= TS_USB20_FRAME_SIZE;
  	}
 
  	if (af9015_config.dual_mode) {

@@ -882,16 +879,11 @@ static int af9015_read_config(struct usb_device *udev)
  		af9015_af9013_config[1].demod_address = val;
 
  		/* enable 2nd adapter */
-		for (i = 0; i<  af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 2;
+		props->num_adapters = 2;
+	} else /* disable 2nd adapter */
+		props->num_adapters = 1;
 
-	} else {
-		 /* disable 2nd adapter */
-		for (i = 0; i<  af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 1;
-	}
-
-	for (i = 0; i<  af9015_properties[0].num_adapters; i++) {
+	for (i = 0; i<  props->num_adapters; i++) {
  		if (i == 1)
  			offset = AF9015_EEPROM_OFFSET;
  		/* xtal */

@@ -995,8 +987,7 @@ error:
  		/* disable dual mode */
  		af9015_config.dual_mode = 0;
  		 /* disable 2nd adapter */
-		for (i = 0; i<  af9015_properties_count; i++)
-			af9015_properties[i].num_adapters = 1;
+		props->num_adapters = 1;
 
  		/* set correct IF */
  		af9015_af9013_config[0].tuner_if = 4570;

@@ -1014,6 +1005,10 @@ static int af9015_identify_state(struct usb_device *udev,
  	u8 reply;
  	struct req_t req = {GET_CONFIG, 0, 0, 0, 0, 1,&reply};
 
+	ret = af9015_read_config(udev, props);
+	if (ret)
+		return ret;
+
  	ret = af9015_rw_udev(udev,&req);
  	if (ret)
  		return ret;

@@ -1675,33 +1670,21 @@ static int af9015_usb_probe(struct usb_interface *intf,
  {
  	int ret = 0;
  	struct dvb_usb_device *d = NULL;
-	struct usb_device *udev = interface_to_usbdev(intf);
  	u8 i;
 
-	deb_info("%s: interface:%d\n", __func__,
-		intf->cur_altsetting->desc.bInterfaceNumber);
-
-	/* interface 0 is used by DVB-T receiver and
-	   interface 1 is for remote controller (HID) */
-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
-		ret = af9015_read_config(udev);
-		if (ret)
-			return ret;
-
-		for (i = 0; i<  af9015_properties_count; i++) {
-			ret = dvb_usb_device_init(intf,&af9015_properties[i],
-				THIS_MODULE,&d, adapter_nr);
-			if (!ret)
-				break;
-			if (ret != -ENODEV)
-				return ret;
-		}
-		if (ret)
+	for (i = 0; i<  af9015_properties_count; i++) {
+		ret = dvb_usb_device_init(intf,&af9015_properties[i],
+			THIS_MODULE,&d, adapter_nr);
+		if (!ret)
+			break;
+		if (ret != -ENODEV)
  			return ret;
-
-		if (d)
-			ret = af9015_init(d);
  	}
+	if (ret)
+		return ret;
+
+	if (d)
+		ret = af9015_init(d);
 
  	return ret;
  }

Signature · 1 line

http://palosaari.fi/

Malcolm Priestley

author

2011-11-12 18:22 UTC #2 lore raw

Quoted 6 lines · Antti Palosaari

Quoted 4 lines · Malcolm Priestley
Remove get config from probe and move to identify_state.
intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.

Quoted 7 lines

Are you sure? IIRC there is HID remote on interface 1 or 2 or so (some other than 0). Please double check.
Quoted 1 line
Calling from probe seems to cause a race condition with some USB controllers.
Why?

Is some other module going to claim the device?

Would it not be better use usb_set_interface to set it back to 0?

Rather than failing it back to the user.

Regards

Malcolm

Antti Palosaari

2011-11-12 19:36 UTC #3 lore raw

Quoted 12 lines · Malcolm Priestley

Quoted 11 lines · Antti Palosaari
Quoted 4 lines · Malcolm Priestley
Remove get config from probe and move to identify_state.
intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.
Are you sure? IIRC there is HID remote on interface 1 or 2 or so (some other than 0). Please double check.
Quoted 1 line
Calling from probe seems to cause a race condition with some USB controllers.

Quoted 7 lines

Quoted 2 lines
Why?
Is some other module going to claim the device?
Would it not be better use usb_set_interface to set it back to 0?
Rather than failing it back to the user.

Maybe I don't understand what you mean.

But here is the copy&paste from code:
/* interface 0 is used by DVB-T receiver and
interface 1 is for remote controller (HID) */
if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {

As it says (comment in the code), interface 1 is for remote controller. I added that to prevent call all that attach stuff twice. I have done such check for some other drivers too where is multiple interfaces.

IIRC .probe() is called once per each interface device has. So it will call that two times (if there is HID remote activated), resulting same device appears two times in bad luck.

Just returning 0 for those probes we don't need sounds good approach for me.

Antti

Signature · 1 line

http://palosaari.fi/

Malcolm Priestley

author

2011-11-13 12:04 UTC #4 lore raw

Quoted 11 lines · Antti Palosaari

Quoted 10 lines · Malcolm Priestley
Quoted 9 lines · Antti Palosaari
Quoted 4 lines · Malcolm Priestley
Remove get config from probe and move to identify_state.
intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.
Are you sure? IIRC there is HID remote on interface 1 or 2 or so (some other than 0). Please double check.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Calling from probe seems to cause a race condition with some USB controllers.
Why?
Is some other module going to claim the device?
Would it not be better use usb_set_interface to set it back to 0?

I spoke too soon, there is someone else about.

On boot input claims interface 1

Nov 13 11:43:36 tvbox kernel: [    1.830276] input: Afatech DVB-T 2 as /devices/pci0000:00/0000:00:06.1/usb2/2-4/2-4:1.1/input/input2
Nov 13 11:43:36 tvbox kernel: [    1.830367] generic-usb 0003:1B80:E399.0001: input,hidraw0: USB HID v1.01 Keyboard [Afatech DVB-T 2] on usb-0000:00:06.1-4/input1
...
Nov 13 11:43:38 tvbox kernel: [   19.151700] dvb-usb: found a 'KWorld PlusTV Dual DVB-T Stick (DVB-T 399U)' in cold state, will try to load a firmware
...
Nov 13 11:43:39 tvbox kernel: [   20.313483] input: IR-receiver inside an USB DVB receiver as /devices/pci0000:00/0000:00:06.1/usb2/2-4/rc/rc0/input8
Nov 13 11:43:39 tvbox kernel: [   20.313528] rc0: IR-receiver inside an USB DVB receiver as /devices/pci0000:00/0000:00:06.1/usb2/2-4/rc/rc0
Nov 13 11:43:39 tvbox kernel: [   20.313530] dvb-usb: schedule remote query interval to 500 msecs.
Nov 13 11:43:39 tvbox kernel: [   20.313534] dvb-usb: KWorld PlusTV Dual DVB-T Stick (DVB-T 399U) successfully initialized and connected.

So, a usb_set_interface is needed to make sure we on interface 0 and remain there.

Regards

Malcolm

Antti Palosaari

2011-11-13 12:34 UTC #5 lore raw

Quoted 35 lines · Malcolm Priestley

Quoted 11 lines · Antti Palosaari
Quoted 10 lines · Malcolm Priestley
Quoted 9 lines · Antti Palosaari
Quoted 4 lines · Malcolm Priestley
Remove get config from probe and move to identify_state.
intf->cur_altsetting->desc.bInterfaceNumber is always expected to be zero, so there no point in checking for it.
Are you sure? IIRC there is HID remote on interface 1 or 2 or so (some other than 0). Please double check.
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Calling from probe seems to cause a race condition with some USB controllers.
Why?
Is some other module going to claim the device?
Would it not be better use usb_set_interface to set it back to 0?
I spoke too soon, there is someone else about.
On boot input claims interface 1
Nov 13 11:43:36 tvbox kernel: [    1.830276] input: Afatech DVB-T 2 as /devices/pci0000:00/0000:00:06.1/usb2/2-4/2-4:1.1/input/input2
Nov 13 11:43:36 tvbox kernel: [    1.830367] generic-usb 0003:1B80:E399.0001: input,hidraw0: USB HID v1.01 Keyboard [Afatech DVB-T 2] on usb-0000:00:06.1-4/input1
...
Nov 13 11:43:38 tvbox kernel: [   19.151700] dvb-usb: found a 'KWorld PlusTV Dual DVB-T Stick (DVB-T 399U)' in cold state, will try to load a firmware
...
Nov 13 11:43:39 tvbox kernel: [   20.313483] input: IR-receiver inside an USB DVB receiver as /devices/pci0000:00/0000:00:06.1/usb2/2-4/rc/rc0/input8
Nov 13 11:43:39 tvbox kernel: [   20.313528] rc0: IR-receiver inside an USB DVB receiver as /devices/pci0000:00/0000:00:06.1/usb2/2-4/rc/rc0
Nov 13 11:43:39 tvbox kernel: [   20.313530] dvb-usb: schedule remote query interval to 500 msecs.
Nov 13 11:43:39 tvbox kernel: [   20.313534] dvb-usb: KWorld PlusTV Dual DVB-T Stick (DVB-T 399U) successfully initialized and connected.

Quoted 1 line

So, a usb_set_interface is needed to make sure we on interface 0 and remain there.

Sorry, didn't still understand what you mean.

What I would like to say check for interface was set for HID remote and since we just want to leave HID without any driver help we will just skip it.

Do you try to say it should be always set usb_set_interface 0 just for sure?

Antti

Signature · 1 line

http://palosaari.fi/