* [PATCH 2/2] em28xx: regression fix: use DRX-K sync firmware requests on em28xx
2012-10-02 19:05 [PATCH 1/2] drxk: allow loading firmware synchrousnously Mauro Carvalho Chehab
@ 2012-10-02 19:05 ` Mauro Carvalho Chehab
2012-10-02 19:26 ` Antti Palosaari
2012-10-02 19:25 ` [PATCH 1/2] drxk: allow loading firmware synchrousnously Antti Palosaari
2012-10-03 7:13 ` Oliver Endriss
2 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2012-10-02 19:05 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List
As em28xx-dvb will always be initialized asynchronously, there's
no need anymore for a separate thread to load the DRX-K firmware.
Fixes a known regression with kernel 3.6 with tda18271 driver
and asynchronous DRX-K firmware load.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/media/usb/em28xx/em28xx-dvb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 1662b70..913e522 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -318,6 +318,7 @@ static struct drxk_config terratec_h5_drxk = {
.no_i2c_bridge = 1,
.microcode_name = "dvb-usb-terratec-h5-drxk.fw",
.qam_demod_parameter_count = 2,
+ .load_firmware_sync = true,
};
static struct drxk_config hauppauge_930c_drxk = {
@@ -327,6 +328,7 @@ static struct drxk_config hauppauge_930c_drxk = {
.microcode_name = "dvb-usb-hauppauge-hvr930c-drxk.fw",
.chunk_size = 56,
.qam_demod_parameter_count = 2,
+ .load_firmware_sync = true,
};
struct drxk_config terratec_htc_stick_drxk = {
@@ -340,12 +342,14 @@ struct drxk_config terratec_htc_stick_drxk = {
.antenna_dvbt = true,
/* The windows driver uses the same. This will disable LNA. */
.antenna_gpio = 0x6,
+ .load_firmware_sync = true,
};
static struct drxk_config maxmedia_ub425_tc_drxk = {
.adr = 0x29,
.single_master = 1,
.no_i2c_bridge = 1,
+ .load_firmware_sync = true,
};
static struct drxk_config pctv_520e_drxk = {
@@ -356,6 +360,7 @@ static struct drxk_config pctv_520e_drxk = {
.chunk_size = 58,
.antenna_dvbt = true, /* disable LNA */
.antenna_gpio = (1 << 2), /* disable LNA */
+ .load_firmware_sync = true,
};
static int drxk_gate_ctrl(struct dvb_frontend *fe, int enable)
--
1.7.11.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] em28xx: regression fix: use DRX-K sync firmware requests on em28xx
2012-10-02 19:05 ` [PATCH 2/2] em28xx: regression fix: use DRX-K sync firmware requests on em28xx Mauro Carvalho Chehab
@ 2012-10-02 19:26 ` Antti Palosaari
0 siblings, 0 replies; 6+ messages in thread
From: Antti Palosaari @ 2012-10-02 19:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
On 10/02/2012 10:05 PM, Mauro Carvalho Chehab wrote:
> As em28xx-dvb will always be initialized asynchronously, there's
> no need anymore for a separate thread to load the DRX-K firmware.
>
> Fixes a known regression with kernel 3.6 with tda18271 driver
> and asynchronous DRX-K firmware load.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Tested-by: Antti Palosaari <crope@iki.fi>
Hauppauge WinTV HVR 930C
MaxMedia UB425-TC
PCTV QuatroStick nano (520e)
> ---
> drivers/media/usb/em28xx/em28xx-dvb.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> index 1662b70..913e522 100644
> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> @@ -318,6 +318,7 @@ static struct drxk_config terratec_h5_drxk = {
> .no_i2c_bridge = 1,
> .microcode_name = "dvb-usb-terratec-h5-drxk.fw",
> .qam_demod_parameter_count = 2,
> + .load_firmware_sync = true,
> };
>
> static struct drxk_config hauppauge_930c_drxk = {
> @@ -327,6 +328,7 @@ static struct drxk_config hauppauge_930c_drxk = {
> .microcode_name = "dvb-usb-hauppauge-hvr930c-drxk.fw",
> .chunk_size = 56,
> .qam_demod_parameter_count = 2,
> + .load_firmware_sync = true,
> };
>
> struct drxk_config terratec_htc_stick_drxk = {
> @@ -340,12 +342,14 @@ struct drxk_config terratec_htc_stick_drxk = {
> .antenna_dvbt = true,
> /* The windows driver uses the same. This will disable LNA. */
> .antenna_gpio = 0x6,
> + .load_firmware_sync = true,
> };
>
> static struct drxk_config maxmedia_ub425_tc_drxk = {
> .adr = 0x29,
> .single_master = 1,
> .no_i2c_bridge = 1,
> + .load_firmware_sync = true,
> };
>
> static struct drxk_config pctv_520e_drxk = {
> @@ -356,6 +360,7 @@ static struct drxk_config pctv_520e_drxk = {
> .chunk_size = 58,
> .antenna_dvbt = true, /* disable LNA */
> .antenna_gpio = (1 << 2), /* disable LNA */
> + .load_firmware_sync = true,
> };
>
> static int drxk_gate_ctrl(struct dvb_frontend *fe, int enable)
>
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drxk: allow loading firmware synchrousnously
2012-10-02 19:05 [PATCH 1/2] drxk: allow loading firmware synchrousnously Mauro Carvalho Chehab
2012-10-02 19:05 ` [PATCH 2/2] em28xx: regression fix: use DRX-K sync firmware requests on em28xx Mauro Carvalho Chehab
@ 2012-10-02 19:25 ` Antti Palosaari
2012-10-03 7:13 ` Oliver Endriss
2 siblings, 0 replies; 6+ messages in thread
From: Antti Palosaari @ 2012-10-02 19:25 UTC (permalink / raw)
To: Mauro Carvalho Chehab
On 10/02/2012 10:05 PM, Mauro Carvalho Chehab wrote:
> Due to udev-182, the firmware load was changed to be async, as
> otherwise udev would give up of loading a firmware.
>
> Add an option to return to the previous behaviour, async firmware
> loads cause failures with the tda18271 driver.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Tested-by: Antti Palosaari <crope@iki.fi>
Hauppauge WinTV HVR 930C
MaxMedia UB425-TC
PCTV QuatroStick nano (520e)
> ---
> drivers/media/dvb-frontends/drxk.h | 2 ++
> drivers/media/dvb-frontends/drxk_hard.c | 20 +++++++++++++++-----
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drxk.h b/drivers/media/dvb-frontends/drxk.h
> index d615d7d..94fecfb 100644
> --- a/drivers/media/dvb-frontends/drxk.h
> +++ b/drivers/media/dvb-frontends/drxk.h
> @@ -28,6 +28,7 @@
> * A value of 0 (default) or lower indicates that
> * the correct number of parameters will be
> * automatically detected.
> + * @load_firmware_sync: Force the firmware load to be synchronous.
> *
> * On the *_gpio vars, bit 0 is UIO-1, bit 1 is UIO-2 and bit 2 is
> * UIO-3.
> @@ -39,6 +40,7 @@ struct drxk_config {
> bool parallel_ts;
> bool dynamic_clk;
> bool enable_merr_cfg;
> + bool load_firmware_sync;
>
> bool antenna_dvbt;
> u16 antenna_gpio;
> diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
> index 1ab8154..8b4c6d5 100644
> --- a/drivers/media/dvb-frontends/drxk_hard.c
> +++ b/drivers/media/dvb-frontends/drxk_hard.c
> @@ -6609,15 +6609,25 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>
> /* Load firmware and initialize DRX-K */
> if (state->microcode_name) {
> - status = request_firmware_nowait(THIS_MODULE, 1,
> + if (config->load_firmware_sync) {
> + const struct firmware *fw = NULL;
> +
> + status = request_firmware(&fw, state->microcode_name,
> + state->i2c->dev.parent);
> + if (status < 0)
> + fw = NULL;
> + load_firmware_cb(fw, state);
> + } else {
> + status = request_firmware_nowait(THIS_MODULE, 1,
> state->microcode_name,
> state->i2c->dev.parent,
> GFP_KERNEL,
> state, load_firmware_cb);
> - if (status < 0) {
> - printk(KERN_ERR
> - "drxk: failed to request a firmware\n");
> - return NULL;
> + if (status < 0) {
> + printk(KERN_ERR
> + "drxk: failed to request a firmware\n");
> + return NULL;
> + }
> }
> } else if (init_drxk(state) < 0)
> goto error;
>
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] drxk: allow loading firmware synchrousnously
2012-10-02 19:05 [PATCH 1/2] drxk: allow loading firmware synchrousnously Mauro Carvalho Chehab
2012-10-02 19:05 ` [PATCH 2/2] em28xx: regression fix: use DRX-K sync firmware requests on em28xx Mauro Carvalho Chehab
2012-10-02 19:25 ` [PATCH 1/2] drxk: allow loading firmware synchrousnously Antti Palosaari
@ 2012-10-03 7:13 ` Oliver Endriss
2012-10-03 8:38 ` Antti Palosaari
2 siblings, 1 reply; 6+ messages in thread
From: Oliver Endriss @ 2012-10-03 7:13 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Mauro Carvalho Chehab
Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Due to udev-182, the firmware load was changed to be async, as
> otherwise udev would give up of loading a firmware.
>
> Add an option to return to the previous behaviour, async firmware
> loads cause failures with the tda18271 driver.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
> drivers/media/dvb-frontends/drxk.h | 2 ++
> drivers/media/dvb-frontends/drxk_hard.c | 20 +++++++++++++++-----
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drxk.h b/drivers/media/dvb-frontends/drxk.h
> index d615d7d..94fecfb 100644
> --- a/drivers/media/dvb-frontends/drxk.h
> +++ b/drivers/media/dvb-frontends/drxk.h
> @@ -28,6 +28,7 @@
> * A value of 0 (default) or lower indicates that
> * the correct number of parameters will be
> * automatically detected.
> + * @load_firmware_sync: Force the firmware load to be synchronous.
> *
> * On the *_gpio vars, bit 0 is UIO-1, bit 1 is UIO-2 and bit 2 is
> * UIO-3.
> @@ -39,6 +40,7 @@ struct drxk_config {
> bool parallel_ts;
> bool dynamic_clk;
> bool enable_merr_cfg;
> + bool load_firmware_sync;
>
> bool antenna_dvbt;
> u16 antenna_gpio;
> diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
> index 1ab8154..8b4c6d5 100644
> --- a/drivers/media/dvb-frontends/drxk_hard.c
> +++ b/drivers/media/dvb-frontends/drxk_hard.c
> @@ -6609,15 +6609,25 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>
> /* Load firmware and initialize DRX-K */
> if (state->microcode_name) {
> - status = request_firmware_nowait(THIS_MODULE, 1,
> + if (config->load_firmware_sync) {
> + const struct firmware *fw = NULL;
> +
> + status = request_firmware(&fw, state->microcode_name,
> + state->i2c->dev.parent);
> + if (status < 0)
> + fw = NULL;
> + load_firmware_cb(fw, state);
> + } else {
> + status = request_firmware_nowait(THIS_MODULE, 1,
> state->microcode_name,
> state->i2c->dev.parent,
> GFP_KERNEL,
> state, load_firmware_cb);
> - if (status < 0) {
> - printk(KERN_ERR
> - "drxk: failed to request a firmware\n");
> - return NULL;
> + if (status < 0) {
> + printk(KERN_ERR
> + "drxk: failed to request a firmware\n");
> + return NULL;
> + }
> }
> } else if (init_drxk(state) < 0)
> goto error;
>
Sorry. loading the firmware asynchronously is simply crap! Remove this!
If you intend to load a firmware, firmware loading must be the first
thing you do with the drxk. You must not access the device, before
firmware loading has completed, or correct operation will not be
guaranteed.
If you insist to keep this option, I request that you make synchronous
loading the default, and you enable asynchronous loading only for
devices _you_ have tested. I will never use asynchronous loading.
(In fact, I have already backed-out your firmware patches from my
drivers and forked off my own version of the drxk.)
Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] drxk: allow loading firmware synchrousnously
2012-10-03 7:13 ` Oliver Endriss
@ 2012-10-03 8:38 ` Antti Palosaari
0 siblings, 0 replies; 6+ messages in thread
From: Antti Palosaari @ 2012-10-03 8:38 UTC (permalink / raw)
To: Oliver Endriss; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab
On 10/03/2012 10:13 AM, Oliver Endriss wrote:
> Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> Due to udev-182, the firmware load was changed to be async, as
>> otherwise udev would give up of loading a firmware.
>>
>> Add an option to return to the previous behaviour, async firmware
>> loads cause failures with the tda18271 driver.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>> drivers/media/dvb-frontends/drxk.h | 2 ++
>> drivers/media/dvb-frontends/drxk_hard.c | 20 +++++++++++++++-----
>> 2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/drxk.h b/drivers/media/dvb-frontends/drxk.h
>> index d615d7d..94fecfb 100644
>> --- a/drivers/media/dvb-frontends/drxk.h
>> +++ b/drivers/media/dvb-frontends/drxk.h
>> @@ -28,6 +28,7 @@
>> * A value of 0 (default) or lower indicates that
>> * the correct number of parameters will be
>> * automatically detected.
>> + * @load_firmware_sync: Force the firmware load to be synchronous.
>> *
>> * On the *_gpio vars, bit 0 is UIO-1, bit 1 is UIO-2 and bit 2 is
>> * UIO-3.
>> @@ -39,6 +40,7 @@ struct drxk_config {
>> bool parallel_ts;
>> bool dynamic_clk;
>> bool enable_merr_cfg;
>> + bool load_firmware_sync;
>>
>> bool antenna_dvbt;
>> u16 antenna_gpio;
>> diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
>> index 1ab8154..8b4c6d5 100644
>> --- a/drivers/media/dvb-frontends/drxk_hard.c
>> +++ b/drivers/media/dvb-frontends/drxk_hard.c
>> @@ -6609,15 +6609,25 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>>
>> /* Load firmware and initialize DRX-K */
>> if (state->microcode_name) {
>> - status = request_firmware_nowait(THIS_MODULE, 1,
>> + if (config->load_firmware_sync) {
>> + const struct firmware *fw = NULL;
>> +
>> + status = request_firmware(&fw, state->microcode_name,
>> + state->i2c->dev.parent);
>> + if (status < 0)
>> + fw = NULL;
>> + load_firmware_cb(fw, state);
>> + } else {
>> + status = request_firmware_nowait(THIS_MODULE, 1,
>> state->microcode_name,
>> state->i2c->dev.parent,
>> GFP_KERNEL,
>> state, load_firmware_cb);
>> - if (status < 0) {
>> - printk(KERN_ERR
>> - "drxk: failed to request a firmware\n");
>> - return NULL;
>> + if (status < 0) {
>> + printk(KERN_ERR
>> + "drxk: failed to request a firmware\n");
>> + return NULL;
>> + }
>> }
>> } else if (init_drxk(state) < 0)
>> goto error;
>>
>
> Sorry. loading the firmware asynchronously is simply crap! Remove this!
>
> If you intend to load a firmware, firmware loading must be the first
> thing you do with the drxk. You must not access the device, before
> firmware loading has completed, or correct operation will not be
> guaranteed.
>
> If you insist to keep this option, I request that you make synchronous
> loading the default, and you enable asynchronous loading only for
> devices _you_ have tested. I will never use asynchronous loading.
> (In fact, I have already backed-out your firmware patches from my
> drivers and forked off my own version of the drxk.)
+1, indeed. Broken by design.
That was quite what I explained earlier too. You are not allowed to
continue attach path until previous attach is done and chip is ready to
offer interface(s) to the chips which are next in attach path. It is not
only that, but general rule.
I don't see any reason why this code should be left here.
regards
Antti
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 6+ messages in thread