[PATCH] [media] drxk: change it to use request_firmware_nowait()

[PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

The firmware blob may not be available when the driver probes.

Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.

This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.

Cc: Antti Palosaari <crope@iki.fi>
Cc: Kay Sievers <kay@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/dvb/frontends/drxk_hard.c |  109 +++++++++++++++++++------------
 drivers/media/dvb/frontends/drxk_hard.h |    3 +
 2 files changed, 72 insertions(+), 40 deletions(-)

diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
 	return status;
 }
 
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
-	const struct firmware *fw = NULL;
-	int err = 0;
-
-	dprintk(1, "\n");
-
-	err = request_firmware(&fw, mc_name, state->i2c->dev.parent);
-	if (err < 0) {
-		printk(KERN_ERR
-		       "drxk: Could not load firmware file %s.\n", mc_name);
-		printk(KERN_INFO
-		       "drxk: Copy %s to your hotplug directory!\n", mc_name);
-		return err;
-	}
-	err = DownloadMicrocode(state, fw->data, fw->size);
-	release_firmware(fw);
-	return err;
-}
-
 static int init_drxk(struct drxk_state *state)
 {
-	int status = 0;
+	int status = 0, n = 0;
 	enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
 	u16 driverVersion;
 
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
 		if (status < 0)
 			goto error;
 
-		if (state->microcode_name)
-			load_microcode(state, state->microcode_name);
+		if (state->fw) {
+			status = DownloadMicrocode(state, state->fw->data,
+						   state->fw->size);
+			if (status < 0)
+				goto error;
+		}
 
 		/* disable token-ring bus through OFDM block for possible ucode upload */
 		status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A, SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
 			state->m_DrxkState = DRXK_POWERED_DOWN;
 		} else
 			state->m_DrxkState = DRXK_STOPPED;
+
+		/* Initialize the supported delivery systems */
+		n = 0;
+		if (state->m_hasDVBC) {
+			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+			strlcat(state->frontend.ops.info.name, " DVB-C",
+				sizeof(state->frontend.ops.info.name));
+		}
+		if (state->m_hasDVBT) {
+			state->frontend.ops.delsys[n++] = SYS_DVBT;
+			strlcat(state->frontend.ops.info.name, " DVB-T",
+				sizeof(state->frontend.ops.info.name));
+		}
 	}
 error:
 	if (status < 0)
@@ -6175,11 +6173,44 @@ error:
 	return status;
 }
 
+static void load_firmware_cb(const struct firmware *fw,
+			     void *context)
+{
+	struct drxk_state *state = context;
+
+	if (!fw) {
+		printk(KERN_ERR
+		       "drxk: Could not load firmware file %s.\n",
+			state->microcode_name);
+		printk(KERN_INFO
+		       "drxk: Copy %s to your hotplug directory!\n",
+			state->microcode_name);
+		state->microcode_name = NULL;
+
+		/*
+		 * As firmware is now load asynchronous, it is not possible
+		 * anymore to fail at frontend attach. We might silently
+		 * return here, and hope that the driver won't crash.
+		 * We might also change all DVB callbacks to return -ENODEV
+		 * if the device is not initialized.
+		 * As the DRX-K devices have their own internal firmware,
+		 * let's just hope that it will match a firmware revision
+		 * compatible with this driver and proceed.
+		 */
+	}
+	state->fw = fw;
+
+	init_drxk(state);
+}
+
 static void drxk_release(struct dvb_frontend *fe)
 {
 	struct drxk_state *state = fe->demodulator_priv;
 
 	dprintk(1, "\n");
+	if (state->fw)
+		release_firmware(state->fw);
+
 	kfree(state);
 }
 
@@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
 struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 				 struct i2c_adapter *i2c)
 {
-	int n;
-
 	struct drxk_state *state = NULL;
 	u8 adr = config->adr;
+	int status;
 
 	dprintk(1, "\n");
 	state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
@@ -6425,22 +6455,21 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 	state->frontend.demodulator_priv = state;
 
 	init_state(state);
-	if (init_drxk(state) < 0)
-		goto error;
 
-	/* Initialize the supported delivery systems */
-	n = 0;
-	if (state->m_hasDVBC) {
-		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
-		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
-		strlcat(state->frontend.ops.info.name, " DVB-C",
-			sizeof(state->frontend.ops.info.name));
-	}
-	if (state->m_hasDVBT) {
-		state->frontend.ops.delsys[n++] = SYS_DVBT;
-		strlcat(state->frontend.ops.info.name, " DVB-T",
-			sizeof(state->frontend.ops.info.name));
-	}
+	/* Load firmware and initialize DRX-K */
+	if (state->microcode_name) {
+		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;
+		}
+	} else if (init_drxk(state) < 0)
+		goto error;
 
 	printk(KERN_INFO "drxk: frontend initialized.\n");
 	return &state->frontend;
diff --git a/drivers/media/dvb/frontends/drxk_hard.h b/drivers/media/dvb/frontends/drxk_hard.h
index 4bbf841..36677cd 100644
--- a/drivers/media/dvb/frontends/drxk_hard.h
+++ b/drivers/media/dvb/frontends/drxk_hard.h
@@ -338,7 +338,10 @@ struct drxk_state {
 	bool	antenna_dvbt;
 	u16	antenna_gpio;
 
+	/* Firmware */
 	const char *microcode_name;
+	struct completion fw_wait_load;
+	const struct firmware *fw;
 };
 
 #define NEVER_LOCK 0
-- 
1.7.10.2

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Devin Heitmueller]

On Thu, Jun 21, 2012 at 9:36 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> The firmware blob may not be available when the driver probes.
>
> Instead of blocking the whole kernel use request_firmware_nowait() and
> continue without firmware.
>
> This shouldn't be that bad on drx-k devices, as they all seem to have an
> internal firmware. So, only the firmware update will take a little longer
> to happen.

The patch itself looks fine, however the comment at the end probably
isn't valid. Many of the drx-k devices don't have onboard flash, and
even for the ones that do have flash, uploading the firmware doesn't
actually rewrite the flash.  It patches the in-RAM copy (or replaces
the *running* image).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 21-06-2012 11:21, Devin Heitmueller escreveu:
> On Thu, Jun 21, 2012 at 9:36 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> The firmware blob may not be available when the driver probes.
>>
>> Instead of blocking the whole kernel use request_firmware_nowait() and
>> continue without firmware.
>>
>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>> internal firmware. So, only the firmware update will take a little longer
>> to happen.
> 
> The patch itself looks fine, however the comment at the end probably
> isn't valid. Many of the drx-k devices don't have onboard flash, and
> even for the ones that do have flash, uploading the firmware doesn't
> actually rewrite the flash.  It patches the in-RAM copy (or replaces
> the *running* image).

Yeah, that's what I meant to say. Just rewrote the patch description.
See below.

Regards,
Mauro

-

[media] drxk: change it to use request_firmware_nowait()

The firmware blob may not be available when the driver probes.

Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.

This shouldn't be that bad on some drx-k devices that have firmware
stored on their ROM's. On those devices, only the RAM version of the firmware
is changed. If that fails, there's still a chance that the device will work.
So, a fail to load the firmware may not be a fatal error.

Cc: Antti Palosaari <crope@iki.fi>
Cc: Kay Sievers <kay@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

---
drivers/media/dvb/frontends/drxk_hard.c |  109 +++++++++++++++++++------------
 drivers/media/dvb/frontends/drxk_hard.h |    3 +
 2 files changed, 72 insertions(+), 40 deletions(-)

diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
 	return status;
 }
 
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
-	const struct firmware *fw = NULL;
-	int err = 0;
-
-	dprintk(1, "\n");
-
-	err = request_firmware(&fw, mc_name, state->i2c->dev.parent);
-	if (err < 0) {
-		printk(KERN_ERR
-		       "drxk: Could not load firmware file %s.\n", mc_name);
-		printk(KERN_INFO
-		       "drxk: Copy %s to your hotplug directory!\n", mc_name);
-		return err;
-	}
-	err = DownloadMicrocode(state, fw->data, fw->size);
-	release_firmware(fw);
-	return err;
-}
-
 static int init_drxk(struct drxk_state *state)
 {
-	int status = 0;
+	int status = 0, n = 0;
 	enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
 	u16 driverVersion;
 
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
 		if (status < 0)
 			goto error;
 
-		if (state->microcode_name)
-			load_microcode(state, state->microcode_name);
+		if (state->fw) {
+			status = DownloadMicrocode(state, state->fw->data,
+						   state->fw->size);
+			if (status < 0)
+				goto error;
+		}
 
 		/* disable token-ring bus through OFDM block for possible ucode upload */
 		status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A, SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
 			state->m_DrxkState = DRXK_POWERED_DOWN;
 		} else
 			state->m_DrxkState = DRXK_STOPPED;
+
+		/* Initialize the supported delivery systems */
+		n = 0;
+		if (state->m_hasDVBC) {
+			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+			strlcat(state->frontend.ops.info.name, " DVB-C",
+				sizeof(state->frontend.ops.info.name));
+		}
+		if (state->m_hasDVBT) {
+			state->frontend.ops.delsys[n++] = SYS_DVBT;
+			strlcat(state->frontend.ops.info.name, " DVB-T",
+				sizeof(state->frontend.ops.info.name));
+		}
 	}
 error:
 	if (status < 0)
@@ -6175,11 +6173,44 @@ error:
 	return status;
 }
 
+static void load_firmware_cb(const struct firmware *fw,
+			     void *context)
+{
+	struct drxk_state *state = context;
+
+	if (!fw) {
+		printk(KERN_ERR
+		       "drxk: Could not load firmware file %s.\n",
+			state->microcode_name);
+		printk(KERN_INFO
+		       "drxk: Copy %s to your hotplug directory!\n",
+			state->microcode_name);
+		state->microcode_name = NULL;
+
+		/*
+		 * As firmware is now load asynchronous, it is not possible
+		 * anymore to fail at frontend attach. We might silently
+		 * return here, and hope that the driver won't crash.
+		 * We might also change all DVB callbacks to return -ENODEV
+		 * if the device is not initialized.
+		 * As the DRX-K devices have their own internal firmware,
+		 * let's just hope that it will match a firmware revision
+		 * compatible with this driver and proceed.
+		 */
+	}
+	state->fw = fw;
+
+	init_drxk(state);
+}
+
 static void drxk_release(struct dvb_frontend *fe)
 {
 	struct drxk_state *state = fe->demodulator_priv;
 
 	dprintk(1, "\n");
+	if (state->fw)
+		release_firmware(state->fw);
+
 	kfree(state);
 }
 
@@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
 struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 				 struct i2c_adapter *i2c)
 {
-	int n;
-
 	struct drxk_state *state = NULL;
 	u8 adr = config->adr;
+	int status;
 
 	dprintk(1, "\n");
 	state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
@@ -6425,22 +6455,21 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
 	state->frontend.demodulator_priv = state;
 
 	init_state(state);
-	if (init_drxk(state) < 0)
-		goto error;
 
-	/* Initialize the supported delivery systems */
-	n = 0;
-	if (state->m_hasDVBC) {
-		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
-		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
-		strlcat(state->frontend.ops.info.name, " DVB-C",
-			sizeof(state->frontend.ops.info.name));
-	}
-	if (state->m_hasDVBT) {
-		state->frontend.ops.delsys[n++] = SYS_DVBT;
-		strlcat(state->frontend.ops.info.name, " DVB-T",
-			sizeof(state->frontend.ops.info.name));
-	}
+	/* Load firmware and initialize DRX-K */
+	if (state->microcode_name) {
+		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;
+		}
+	} else if (init_drxk(state) < 0)
+		goto error;
 
 	printk(KERN_INFO "drxk: frontend initialized.\n");
 	return &state->frontend;
diff --git a/drivers/media/dvb/frontends/drxk_hard.h b/drivers/media/dvb/frontends/drxk_hard.h
index 4bbf841..36677cd 100644
--- a/drivers/media/dvb/frontends/drxk_hard.h
+++ b/drivers/media/dvb/frontends/drxk_hard.h
@@ -338,7 +338,10 @@ struct drxk_state {
 	bool	antenna_dvbt;
 	u16	antenna_gpio;
 
+	/* Firmware */
 	const char *microcode_name;
+	struct completion fw_wait_load;
+	const struct firmware *fw;
 };
 
 #define NEVER_LOCK 0

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
> The firmware blob may not be available when the driver probes.
> 
> Instead of blocking the whole kernel use request_firmware_nowait() and
> continue without firmware.
> 
> This shouldn't be that bad on drx-k devices, as they all seem to have an
> internal firmware. So, only the firmware update will take a little longer
> to happen.

While thinking on converting another DVB frontend driver, I just realized
that a patch like that won't work fine.

As most of you know, there are _several_ I2C chips that don't tolerate any
usage of the I2C bus while a firmware is being loaded (I dunno if this is the
case of drx-k, but I won't doubt).

The current approach makes the device probe() logic is serialized. So, there's
no chance that two different I2C drivers to try to access the bus at the same
time, if the bridge driver is properly implemented.

However, now that firmware is loaded asynchronously, several other I2C drivers
may be trying to use the bus at the same time. So, events like IR (and CI) polling,
tuner get_status, etc can happen during a firmware transfer, causing the firmware
to not load properly.

A fix for that will require to lock the firmware load I2C traffic into a single
transaction.

So, the code that sends the firmware to the board need to be changed to something like:

static int DownloadMicrocode(struct drxk_state *state,
			     const u8 pMCImage[], u32 Length)
{

...
	i2c_lock_adapter(state->i2c);
	...
	for (i = 0; i < nBlocks; i += 1) {
		...

		status =  __i2c_transfer(state->i2c, ...);
		...
	}
	i2c_unlock_adapter(state->i2c);
	return status;
}

where __i2c_transfer() would be a variant of i2c_transfer() that won't take the
I2C lock.

Other drivers that load firmware at I2C and use request_firmware_nowait() will also
need a similar approach.

Jean,

What do you think?

Regards,
Mauro

> 
> Cc: Antti Palosaari <crope@iki.fi>
> Cc: Kay Sievers <kay@redhat.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>   drivers/media/dvb/frontends/drxk_hard.c |  109 +++++++++++++++++++------------
>   drivers/media/dvb/frontends/drxk_hard.h |    3 +
>   2 files changed, 72 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
> index 60b868f..4cb8d1e 100644
> --- a/drivers/media/dvb/frontends/drxk_hard.c
> +++ b/drivers/media/dvb/frontends/drxk_hard.c
> @@ -5968,29 +5968,9 @@ error:
>   	return status;
>   }
>   
> -static int load_microcode(struct drxk_state *state, const char *mc_name)
> -{
> -	const struct firmware *fw = NULL;
> -	int err = 0;
> -
> -	dprintk(1, "\n");
> -
> -	err = request_firmware(&fw, mc_name, state->i2c->dev.parent);
> -	if (err < 0) {
> -		printk(KERN_ERR
> -		       "drxk: Could not load firmware file %s.\n", mc_name);
> -		printk(KERN_INFO
> -		       "drxk: Copy %s to your hotplug directory!\n", mc_name);
> -		return err;
> -	}
> -	err = DownloadMicrocode(state, fw->data, fw->size);
> -	release_firmware(fw);
> -	return err;
> -}
> -
>   static int init_drxk(struct drxk_state *state)
>   {
> -	int status = 0;
> +	int status = 0, n = 0;
>   	enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
>   	u16 driverVersion;
>   
> @@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
>   		if (status < 0)
>   			goto error;
>   
> -		if (state->microcode_name)
> -			load_microcode(state, state->microcode_name);
> +		if (state->fw) {
> +			status = DownloadMicrocode(state, state->fw->data,
> +						   state->fw->size);
> +			if (status < 0)
> +				goto error;
> +		}
>   
>   		/* disable token-ring bus through OFDM block for possible ucode upload */
>   		status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A, SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
> @@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
>   			state->m_DrxkState = DRXK_POWERED_DOWN;
>   		} else
>   			state->m_DrxkState = DRXK_STOPPED;
> +
> +		/* Initialize the supported delivery systems */
> +		n = 0;
> +		if (state->m_hasDVBC) {
> +			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
> +			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
> +			strlcat(state->frontend.ops.info.name, " DVB-C",
> +				sizeof(state->frontend.ops.info.name));
> +		}
> +		if (state->m_hasDVBT) {
> +			state->frontend.ops.delsys[n++] = SYS_DVBT;
> +			strlcat(state->frontend.ops.info.name, " DVB-T",
> +				sizeof(state->frontend.ops.info.name));
> +		}
>   	}
>   error:
>   	if (status < 0)
> @@ -6175,11 +6173,44 @@ error:
>   	return status;
>   }
>   
> +static void load_firmware_cb(const struct firmware *fw,
> +			     void *context)
> +{
> +	struct drxk_state *state = context;
> +
> +	if (!fw) {
> +		printk(KERN_ERR
> +		       "drxk: Could not load firmware file %s.\n",
> +			state->microcode_name);
> +		printk(KERN_INFO
> +		       "drxk: Copy %s to your hotplug directory!\n",
> +			state->microcode_name);
> +		state->microcode_name = NULL;
> +
> +		/*
> +		 * As firmware is now load asynchronous, it is not possible
> +		 * anymore to fail at frontend attach. We might silently
> +		 * return here, and hope that the driver won't crash.
> +		 * We might also change all DVB callbacks to return -ENODEV
> +		 * if the device is not initialized.
> +		 * As the DRX-K devices have their own internal firmware,
> +		 * let's just hope that it will match a firmware revision
> +		 * compatible with this driver and proceed.
> +		 */
> +	}
> +	state->fw = fw;
> +
> +	init_drxk(state);
> +}
> +
>   static void drxk_release(struct dvb_frontend *fe)
>   {
>   	struct drxk_state *state = fe->demodulator_priv;
>   
>   	dprintk(1, "\n");
> +	if (state->fw)
> +		release_firmware(state->fw);
> +
>   	kfree(state);
>   }
>   
> @@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
>   struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>   				 struct i2c_adapter *i2c)
>   {
> -	int n;
> -
>   	struct drxk_state *state = NULL;
>   	u8 adr = config->adr;
> +	int status;
>   
>   	dprintk(1, "\n");
>   	state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
> @@ -6425,22 +6455,21 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>   	state->frontend.demodulator_priv = state;
>   
>   	init_state(state);
> -	if (init_drxk(state) < 0)
> -		goto error;
>   
> -	/* Initialize the supported delivery systems */
> -	n = 0;
> -	if (state->m_hasDVBC) {
> -		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
> -		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
> -		strlcat(state->frontend.ops.info.name, " DVB-C",
> -			sizeof(state->frontend.ops.info.name));
> -	}
> -	if (state->m_hasDVBT) {
> -		state->frontend.ops.delsys[n++] = SYS_DVBT;
> -		strlcat(state->frontend.ops.info.name, " DVB-T",
> -			sizeof(state->frontend.ops.info.name));
> -	}
> +	/* Load firmware and initialize DRX-K */
> +	if (state->microcode_name) {
> +		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;
> +		}
> +	} else if (init_drxk(state) < 0)
> +		goto error;
>   
>   	printk(KERN_INFO "drxk: frontend initialized.\n");
>   	return &state->frontend;
> diff --git a/drivers/media/dvb/frontends/drxk_hard.h b/drivers/media/dvb/frontends/drxk_hard.h
> index 4bbf841..36677cd 100644
> --- a/drivers/media/dvb/frontends/drxk_hard.h
> +++ b/drivers/media/dvb/frontends/drxk_hard.h
> @@ -338,7 +338,10 @@ struct drxk_state {
>   	bool	antenna_dvbt;
>   	u16	antenna_gpio;
>   
> +	/* Firmware */
>   	const char *microcode_name;
> +	struct completion fw_wait_load;
> +	const struct firmware *fw;
>   };
>   
>   #define NEVER_LOCK 0
> 

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Marko Ristola]

Hi Mauro

I think that your __i2c_transfer() solution is elegant for DVB-C frontend tuner reprogramming sleep cases:
TDA6650 programming manual, page 9 states:

"Main divider data are valid only if no new I2C-bus transmission
is started (START condition) during the computation
period of 50 μs."
http://www.datasheetcatalog.org/datasheet/philips/TDA6650.pdf

It states that after TDA6650 frontend programming, the whole I2C bus
(for any I2C transfer into any I2C device) must be quiet at least 50μs.

My main point for __i2c_transfer() elegance can be seen easilly
from the code examples below.

-----------------

An elegant way to solve only the 50μs TDA6650 sleep problem (not gate control issue),
is to solve it in tda665x.c's tda665x_set_state():

static int tda665x_set_state(struct dvb_frontend *fe,
                              enum tuner_param param,
                              struct tuner_state *tstate)
{
	...
	i2c_lock_adapter(state->i2c);
	/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
	err = __tda665x_write(state, buf, 5);
	usleep(50);
	i2c_unlock_adapter(state->i2c);
	...
}


Ugly robust way to solveonly the 50μs TDA6650 sleep problem (not gate control issue,
is to teach I2C transfer code TDA6650's requirements:
-- warning: ugly --
saa7146_i2c_transfer(..) {
   ...
   mutex_lock_interruptible(&dev->i2c_lock);
   ...
   send_i2c_messages();
   if (num == 5 && is_tda665())
     usleep(50);
   mutex_unlock(&dev->i2c_lock);
}


Here is a robust version with __i2c_transfer(). Main idea is to
protect the whole communicating with I2C Frontend device with I2C lock:
- Open I2C Gate so that you can talk to DVB Frontend Tuner.
- Send 5 bytes into DVB-C Frontend tuner.
- Quiesce I2C bus by sleeping 50μs to protect the Frontend Tuner  message.
- Close I2C Gate so that DVB Frontend Tuner isn't reachable by I2C bus.

static int tda665x_set_state(struct dvb_frontend *fe,
                              enum tuner_param param,
                              struct tuner_state *tstate)
{
	...
	i2c_lock_adapter(state->i2c);
         __open_i2c_gate(state);
	/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
	err = __tda665x_write(state, buf, 5);
	usleep(50);
         __close_i2c_gate(state);
	i2c_unlock_adapter(state->i2c);
	...
}


-- Ugly solution for the same full problem in SAA7146 I2C transfer function itself (code with some unsolved problems) --
saa7146_i2c_transfer(..) {
   ...
   mutex_lock_interruptible(&dev->i2c_lock);
   if (is_tda665(addr))
     send_msg_tda665_gate_open(dev);
   ...
   send_i2c_messages();
   if (num == 5 && is_tda665())
     usleep(50);
   if (is_tda665(addr))
     send_msg_tda665_gate_close(dev);
   mutex_unlock(&dev->i2c_lock);
}

Regards,
Marko Ristola

On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>> The firmware blob may not be available when the driver probes.
>>
>> Instead of blocking the whole kernel use request_firmware_nowait() and
>> continue without firmware.
>>
>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>> internal firmware. So, only the firmware update will take a little longer
>> to happen.
>
> While thinking on converting another DVB frontend driver, I just realized
> that a patch like that won't work fine.
>
> As most of you know, there are _several_ I2C chips that don't tolerate any
> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
> case of drx-k, but I won't doubt).
>
> The current approach makes the device probe() logic is serialized. So, there's
> no chance that two different I2C drivers to try to access the bus at the same
> time, if the bridge driver is properly implemented.
>
> However, now that firmware is loaded asynchronously, several other I2C drivers
> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
> tuner get_status, etc can happen during a firmware transfer, causing the firmware
> to not load properly.
>
> A fix for that will require to lock the firmware load I2C traffic into a single
> transaction.
>
> So, the code that sends the firmware to the board need to be changed to something like:
>
> static int DownloadMicrocode(struct drxk_state *state,
> 			     const u8 pMCImage[], u32 Length)
> {
>
> ...
> 	i2c_lock_adapter(state->i2c);
> 	...
> 	for (i = 0; i < nBlocks; i += 1) {
> 		...
>
> 		status =  __i2c_transfer(state->i2c, ...);
> 		...
> 	}
> 	i2c_unlock_adapter(state->i2c);
> 	return status;
> }
>
> where __i2c_transfer() would be a variant of i2c_transfer() that won't take the
> I2C lock.
>
> Other drivers that load firmware at I2C and use request_firmware_nowait() will also
> need a similar approach.
>
> Jean,
>
> What do you think?
>
> Regards,
> Mauro
>
>>
>> Cc: Antti Palosaari <crope@iki.fi>
>> Cc: Kay Sievers <kay@redhat.com>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>    drivers/media/dvb/frontends/drxk_hard.c |  109 +++++++++++++++++++------------
>>    drivers/media/dvb/frontends/drxk_hard.h |    3 +
>>    2 files changed, 72 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
>> index 60b868f..4cb8d1e 100644
>> --- a/drivers/media/dvb/frontends/drxk_hard.c
>> +++ b/drivers/media/dvb/frontends/drxk_hard.c
>> @@ -5968,29 +5968,9 @@ error:
>>    	return status;
>>    }
>>
>> -static int load_microcode(struct drxk_state *state, const char *mc_name)
>> -{
>> -	const struct firmware *fw = NULL;
>> -	int err = 0;
>> -
>> -	dprintk(1, "\n");
>> -
>> -	err = request_firmware(&fw, mc_name, state->i2c->dev.parent);
>> -	if (err < 0) {
>> -		printk(KERN_ERR
>> -		       "drxk: Could not load firmware file %s.\n", mc_name);
>> -		printk(KERN_INFO
>> -		       "drxk: Copy %s to your hotplug directory!\n", mc_name);
>> -		return err;
>> -	}
>> -	err = DownloadMicrocode(state, fw->data, fw->size);
>> -	release_firmware(fw);
>> -	return err;
>> -}
>> -
>>    static int init_drxk(struct drxk_state *state)
>>    {
>> -	int status = 0;
>> +	int status = 0, n = 0;
>>    	enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
>>    	u16 driverVersion;
>>
>> @@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
>>    		if (status < 0)
>>    			goto error;
>>
>> -		if (state->microcode_name)
>> -			load_microcode(state, state->microcode_name);
>> +		if (state->fw) {
>> +			status = DownloadMicrocode(state, state->fw->data,
>> +						   state->fw->size);
>> +			if (status < 0)
>> +				goto error;
>> +		}
>>
>>    		/* disable token-ring bus through OFDM block for possible ucode upload */
>>    		status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A, SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
>> @@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
>>    			state->m_DrxkState = DRXK_POWERED_DOWN;
>>    		} else
>>    			state->m_DrxkState = DRXK_STOPPED;
>> +
>> +		/* Initialize the supported delivery systems */
>> +		n = 0;
>> +		if (state->m_hasDVBC) {
>> +			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
>> +			state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
>> +			strlcat(state->frontend.ops.info.name, " DVB-C",
>> +				sizeof(state->frontend.ops.info.name));
>> +		}
>> +		if (state->m_hasDVBT) {
>> +			state->frontend.ops.delsys[n++] = SYS_DVBT;
>> +			strlcat(state->frontend.ops.info.name, " DVB-T",
>> +				sizeof(state->frontend.ops.info.name));
>> +		}
>>    	}
>>    error:
>>    	if (status < 0)
>> @@ -6175,11 +6173,44 @@ error:
>>    	return status;
>>    }
>>
>> +static void load_firmware_cb(const struct firmware *fw,
>> +			     void *context)
>> +{
>> +	struct drxk_state *state = context;
>> +
>> +	if (!fw) {
>> +		printk(KERN_ERR
>> +		       "drxk: Could not load firmware file %s.\n",
>> +			state->microcode_name);
>> +		printk(KERN_INFO
>> +		       "drxk: Copy %s to your hotplug directory!\n",
>> +			state->microcode_name);
>> +		state->microcode_name = NULL;
>> +
>> +		/*
>> +		 * As firmware is now load asynchronous, it is not possible
>> +		 * anymore to fail at frontend attach. We might silently
>> +		 * return here, and hope that the driver won't crash.
>> +		 * We might also change all DVB callbacks to return -ENODEV
>> +		 * if the device is not initialized.
>> +		 * As the DRX-K devices have their own internal firmware,
>> +		 * let's just hope that it will match a firmware revision
>> +		 * compatible with this driver and proceed.
>> +		 */
>> +	}
>> +	state->fw = fw;
>> +
>> +	init_drxk(state);
>> +}
>> +
>>    static void drxk_release(struct dvb_frontend *fe)
>>    {
>>    	struct drxk_state *state = fe->demodulator_priv;
>>
>>    	dprintk(1, "\n");
>> +	if (state->fw)
>> +		release_firmware(state->fw);
>> +
>>    	kfree(state);
>>    }
>>
>> @@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
>>    struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>>    				 struct i2c_adapter *i2c)
>>    {
>> -	int n;
>> -
>>    	struct drxk_state *state = NULL;
>>    	u8 adr = config->adr;
>> +	int status;
>>
>>    	dprintk(1, "\n");
>>    	state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
>> @@ -6425,22 +6455,21 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
>>    	state->frontend.demodulator_priv = state;
>>
>>    	init_state(state);
>> -	if (init_drxk(state) < 0)
>> -		goto error;
>>
>> -	/* Initialize the supported delivery systems */
>> -	n = 0;
>> -	if (state->m_hasDVBC) {
>> -		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
>> -		state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
>> -		strlcat(state->frontend.ops.info.name, " DVB-C",
>> -			sizeof(state->frontend.ops.info.name));
>> -	}
>> -	if (state->m_hasDVBT) {
>> -		state->frontend.ops.delsys[n++] = SYS_DVBT;
>> -		strlcat(state->frontend.ops.info.name, " DVB-T",
>> -			sizeof(state->frontend.ops.info.name));
>> -	}
>> +	/* Load firmware and initialize DRX-K */
>> +	if (state->microcode_name) {
>> +		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;
>> +		}
>> +	} else if (init_drxk(state) < 0)
>> +		goto error;
>>
>>    	printk(KERN_INFO "drxk: frontend initialized.\n");
>>    	return &state->frontend;
>> diff --git a/drivers/media/dvb/frontends/drxk_hard.h b/drivers/media/dvb/frontends/drxk_hard.h
>> index 4bbf841..36677cd 100644
>> --- a/drivers/media/dvb/frontends/drxk_hard.h
>> +++ b/drivers/media/dvb/frontends/drxk_hard.h
>> @@ -338,7 +338,10 @@ struct drxk_state {
>>    	bool	antenna_dvbt;
>>    	u16	antenna_gpio;
>>
>> +	/* Firmware */
>>    	const char *microcode_name;
>> +	struct completion fw_wait_load;
>> +	const struct firmware *fw;
>>    };
>>
>>    #define NEVER_LOCK 0
>>
>
>
> --
> 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] [media] drxk: change it to use request_firmware_nowait()

[Antti Palosaari]

On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>> The firmware blob may not be available when the driver probes.
>>
>> Instead of blocking the whole kernel use request_firmware_nowait() and
>> continue without firmware.
>>
>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>> internal firmware. So, only the firmware update will take a little longer
>> to happen.
>
> While thinking on converting another DVB frontend driver, I just realized
> that a patch like that won't work fine.
>
> As most of you know, there are _several_ I2C chips that don't tolerate any
> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
> case of drx-k, but I won't doubt).
>
> The current approach makes the device probe() logic is serialized. So, there's
> no chance that two different I2C drivers to try to access the bus at the same
> time, if the bridge driver is properly implemented.
>
> However, now that firmware is loaded asynchronously, several other I2C drivers
> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
> tuner get_status, etc can happen during a firmware transfer, causing the firmware
> to not load properly.
>
> A fix for that will require to lock the firmware load I2C traffic into a single
> transaction.

How about deferring registration or probe of every bus-interface (usb, 
pci, firewire) drivers we have. If we defer interface driver using work 
or some other trick we don't need to touch any other chip-drivers that 
are chained behind interface driver. Demodulator, tuner, decoder, remote 
and all the other peripheral drivers can be left as those are currently 
because those are deferred by bus interface driver.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 25-06-2012 16:15, Antti Palosaari escreveu:
> On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
>> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>>> The firmware blob may not be available when the driver probes.
>>>
>>> Instead of blocking the whole kernel use request_firmware_nowait() and
>>> continue without firmware.
>>>
>>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>>> internal firmware. So, only the firmware update will take a little longer
>>> to happen.
>>
>> While thinking on converting another DVB frontend driver, I just realized
>> that a patch like that won't work fine.
>>
>> As most of you know, there are _several_ I2C chips that don't tolerate any
>> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
>> case of drx-k, but I won't doubt).
>>
>> The current approach makes the device probe() logic is serialized. So, there's
>> no chance that two different I2C drivers to try to access the bus at the same
>> time, if the bridge driver is properly implemented.
>>
>> However, now that firmware is loaded asynchronously, several other I2C drivers
>> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
>> tuner get_status, etc can happen during a firmware transfer, causing the firmware
>> to not load properly.
>>
>> A fix for that will require to lock the firmware load I2C traffic into a single
>> transaction.
> 
> How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have.
> If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers
> that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral
> drivers can be left as those are currently because those are deferred by bus interface driver.

There are some issues with regards to it:

1) Currently, driver core doesn't allow a deferred probe. Drivers might implement
that, but they'll lie to the core that the driver were properly supported even when
probe fails. So, driver's core need an .async_probe() method;

2) The firmware load issue will still happen at resume. So, a lock like that is
still needed;

3) It can make some sense to async load the firmware for some drivers, especially
when the device detection may not be dependent on a firmware load.

I'm not fully convinced about (3), as the amount of changes are significant for
not much gain.

There's also another related issue: On devices where both bridge and sub-devices (like frontend)
needs firmware to be loaded, the load order is important at resume(), as the bridge
requires to get the firmware before the sub-devices.

That's said, IMO, the best approach is to do:

1) add support for asynchronous probe at device core, for devices that requires firmware
at probe(). The async_probe() will only be active if !usermodehelper_disabled.

2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface.

We can postpone or get rid of changing the I2C drivers to use request_firmware_async(),
if the request_firmware() core is not pedantic enough to complain and it is not gonna
to be deprecated.

Anyway, I'll open a thead c/c Greg KH (driver's core maintainer) with regards to the need
of an async_probe() callback.

Regards,
Mauro

> 
> regards
> Antti
> 

Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Greg,

Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).

Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should "bind the driver to a given device.  That includes 
verifying that the device is present, that it's a version the driver can handle, 
that driver data structures can be allocated and initialized, and that any
hardware can be initialized".

All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.

Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.

The media handling function is then implemented via a series of I2C devices[1]:
	- analog video decoders;
	- TV tuners;
	- radio tuners;
	- I2C remote controller decoders;
	- DVB frontends;
	- mpeg decoders;
	- mpeg encoders;
	- video enhancement devices;
	...

[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.

In order to properly refer to each component, we call the "main" kernel module
that talks with the media device via USB/PCI bus is called "bridge driver", 
and the I2C components are called as "sub-devices".

Different vendors use the same bridge driver to work with different sub-devices.

It is a .probe()'s task to detect what sub-devices are there inside the board.

There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.

So, drivers do things like the code below, inside the .probe() callback:

static int check_if_dvb_frontend_is_supported_and_bind()
{
	switch (device_model) {
	case VENDOR_A_MODEL_1:
		if (test_and_bind_frontend_1())	/* Doesn't require firmware */
			return 0;
		if (test_and_bind_frontend_2())	/* requires firmware "foo" */
			return 0;
		if (test_and_bind_frontend_3())	/* requires firmware "bar" */
			return 0;
		if (test_and_bind_frontend_4()) /* doesn't require firmware */
			return 0;
		break;
	case VENDOR_A_MODEL_2:
		/* Analog device - no DVB frontend on it */
		return 0;
	...
	}
	return -ENODEV;
}

On several devices, before being able to register the bus and do the actual
probe, the kernel needs to load a firmware.

Also, during the I2C device probing time, firmware may be required, in order
to properly expose the device's internal models and their capabilities. 

For example, drx-k sub-device can have support for either DVB-C or DVB-T or both,
depending on the device model. That affects the frontend properties exposed 
to the user and might affect the bridge driver's initialization task.

In practice, a driver like em28xx have a few devices like HVR-930C that require
the drx-k sub-device. For those devices, a firmware is required; for other
devices, a firmware is not required.

What's happening is that newer versions of request_firmware and udev are being
more pedantic (for a reason) about not requesting firmwares during module_init
or PCI/USB register's probe callback.

Worse than that, the same device driver may require a firmware or not, depending on
the I2C devices inside it. One such example is em28xx: for the great majority of
the supported devices, no firmware is needed, but devices like HVR-930C require
a firmware, because it uses a frontend that needs firmware.

After some discussions, it seems that the best model would be to add an async_probe()
callback to be used by devices similar to media ones. The async_probe() should be
not probed during the module_init; the probe() will be deferred to happen later,
when firmware's usermodehelper_disabled is false, allowing those drivers to load their
firmwares if needed.

What do you think?

Regards,
Mauro

Em 25-06-2012 17:06, Mauro Carvalho Chehab escreveu:
> Em 25-06-2012 16:15, Antti Palosaari escreveu:
>> On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
>>> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>>>> The firmware blob may not be available when the driver probes.
>>>>
>>>> Instead of blocking the whole kernel use request_firmware_nowait() and
>>>> continue without firmware.
>>>>
>>>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>>>> internal firmware. So, only the firmware update will take a little longer
>>>> to happen.
>>>
>>> While thinking on converting another DVB frontend driver, I just realized
>>> that a patch like that won't work fine.
>>>
>>> As most of you know, there are _several_ I2C chips that don't tolerate any
>>> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
>>> case of drx-k, but I won't doubt).
>>>
>>> The current approach makes the device probe() logic is serialized. So, there's
>>> no chance that two different I2C drivers to try to access the bus at the same
>>> time, if the bridge driver is properly implemented.
>>>
>>> However, now that firmware is loaded asynchronously, several other I2C drivers
>>> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
>>> tuner get_status, etc can happen during a firmware transfer, causing the firmware
>>> to not load properly.
>>>
>>> A fix for that will require to lock the firmware load I2C traffic into a single
>>> transaction.
>>
>> How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have.
>> If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers
>> that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral
>> drivers can be left as those are currently because those are deferred by bus interface driver.
> 
> There are some issues with regards to it:
> 
> 1) Currently, driver core doesn't allow a deferred probe. Drivers might implement
> that, but they'll lie to the core that the driver were properly supported even when
> probe fails. So, driver's core need an .async_probe() method;
> 
> 2) The firmware load issue will still happen at resume. So, a lock like that is
> still needed;
> 
> 3) It can make some sense to async load the firmware for some drivers, especially
> when the device detection may not be dependent on a firmware load.
> 
> I'm not fully convinced about (3), as the amount of changes are significant for
> not much gain.
> 
> There's also another related issue: On devices where both bridge and sub-devices (like frontend)
> needs firmware to be loaded, the load order is important at resume(), as the bridge
> requires to get the firmware before the sub-devices.
> 
> That's said, IMO, the best approach is to do:
> 
> 1) add support for asynchronous probe at device core, for devices that requires firmware
> at probe(). The async_probe() will only be active if !usermodehelper_disabled.
> 
> 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface.
> 
> We can postpone or get rid of changing the I2C drivers to use request_firmware_async(),
> if the request_firmware() core is not pedantic enough to complain and it is not gonna
> to be deprecated.
> 
> Anyway, I'll open a thead c/c Greg KH (driver's core maintainer) with regards to the need
> of an async_probe() callback.

Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Greg,

Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).

Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should "bind the driver to a given device.  That includes 
verifying that the device is present, that it's a version the driver can handle, 
that driver data structures can be allocated and initialized, and that any
hardware can be initialized".

All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.

Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.

The media handling function is then implemented via a series of I2C devices[1]:
	- analog video decoders;
	- TV tuners;
	- radio tuners;
	- I2C remote controller decoders;
	- DVB frontends;
	- mpeg decoders;
	- mpeg encoders;
	- video enhancement devices;
	...

[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.

In order to properly refer to each component, we call the "main" kernel module
that talks with the media device via USB/PCI bus is called "bridge driver", 
and the I2C components are called as "sub-devices".

Different vendors use the same bridge driver to work with different sub-devices.

It is a .probe()'s task to detect what sub-devices are there inside the board.

There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.

So, drivers do things like the code below, inside the .probe() callback:

static int check_if_dvb_frontend_is_supported_and_bind()
{
	switch (device_model) {
	case VENDOR_A_MODEL_1:
		if (test_and_bind_frontend_1())	/* Doesn't require firmware */
			return 0;
		if (test_and_bind_frontend_2())	/* requires firmware "foo" */
			return 0;
		if (test_and_bind_frontend_3())	/* requires firmware "bar" */
			return 0;
		if (test_and_bind_frontend_4()) /* doesn't require firmware */
			return 0;
		break;
	case VENDOR_A_MODEL_2:
		/* Analog device - no DVB frontend on it */
		return 0;
	...
	}
	return -ENODEV;
}

On several devices, before being able to register the bus and do the actual
probe, the kernel needs to load a firmware.

Also, during the I2C device probing time, firmware may be required, in order
to properly expose the device's internal models and their capabilities. 

For example, drx-k sub-device can have support for either DVB-C or DVB-T or both,
depending on the device model. That affects the frontend properties exposed 
to the user and might affect the bridge driver's initialization task.

In practice, a driver like em28xx have a few devices like HVR-930C that require
the drx-k sub-device. For those devices, a firmware is required; for other
devices, a firmware is not required.

What's happening is that newer versions of request_firmware and udev are being
more pedantic (for a reason) about not requesting firmwares during module_init
or PCI/USB register's probe callback.

Worse than that, the same device driver may require a firmware or not, depending on
the I2C devices inside it. One such example is em28xx: for the great majority of
the supported devices, no firmware is needed, but devices like HVR-930C require
a firmware, because it uses a frontend that needs firmware.

After some discussions, it seems that the best model would be to add an async_probe()
callback to be used by devices similar to media ones. The async_probe() should be
not probed during the module_init; the probe() will be deferred to happen later,
when firmware's usermodehelper_disabled is false, allowing those drivers to load their
firmwares if needed.

What do you think?

Regards,
Mauro

Em 25-06-2012 17:06, Mauro Carvalho Chehab escreveu:
> Em 25-06-2012 16:15, Antti Palosaari escreveu:
>> On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
>>> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>>>> The firmware blob may not be available when the driver probes.
>>>>
>>>> Instead of blocking the whole kernel use request_firmware_nowait() and
>>>> continue without firmware.
>>>>
>>>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>>>> internal firmware. So, only the firmware update will take a little longer
>>>> to happen.
>>>
>>> While thinking on converting another DVB frontend driver, I just realized
>>> that a patch like that won't work fine.
>>>
>>> As most of you know, there are _several_ I2C chips that don't tolerate any
>>> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
>>> case of drx-k, but I won't doubt).
>>>
>>> The current approach makes the device probe() logic is serialized. So, there's
>>> no chance that two different I2C drivers to try to access the bus at the same
>>> time, if the bridge driver is properly implemented.
>>>
>>> However, now that firmware is loaded asynchronously, several other I2C drivers
>>> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
>>> tuner get_status, etc can happen during a firmware transfer, causing the firmware
>>> to not load properly.
>>>
>>> A fix for that will require to lock the firmware load I2C traffic into a single
>>> transaction.
>>
>> How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have.
>> If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers
>> that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral
>> drivers can be left as those are currently because those are deferred by bus interface driver.
> 
> There are some issues with regards to it:
> 
> 1) Currently, driver core doesn't allow a deferred probe. Drivers might implement
> that, but they'll lie to the core that the driver were properly supported even when
> probe fails. So, driver's core need an .async_probe() method;
> 
> 2) The firmware load issue will still happen at resume. So, a lock like that is
> still needed;
> 
> 3) It can make some sense to async load the firmware for some drivers, especially
> when the device detection may not be dependent on a firmware load.
> 
> I'm not fully convinced about (3), as the amount of changes are significant for
> not much gain.
> 
> There's also another related issue: On devices where both bridge and sub-devices (like frontend)
> needs firmware to be loaded, the load order is important at resume(), as the bridge
> requires to get the firmware before the sub-devices.
> 
> That's said, IMO, the best approach is to do:
> 
> 1) add support for asynchronous probe at device core, for devices that requires firmware
> at probe(). The async_probe() will only be active if !usermodehelper_disabled.
> 
> 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface.
> 
> We can postpone or get rid of changing the I2C drivers to use request_firmware_async(),
> if the request_firmware() core is not pedantic enough to complain and it is not gonna
> to be deprecated.
> 
> Anyway, I'll open a thead c/c Greg KH (driver's core maintainer) with regards to the need
> of an async_probe() callback.
--
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: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote:
> Greg,
> 
> Basically, the recent changes at request_firmware() exposed an issue that
> affects all media drivers that use firmware (64 drivers).

What change was that?  How did it break anything?

> Driver's documentation at Documentation/driver-model/driver.txt says that the
> .probe() callback should "bind the driver to a given device.  That includes 
> verifying that the device is present, that it's a version the driver can handle, 
> that driver data structures can be allocated and initialized, and that any
> hardware can be initialized".

Yes.

> All media device drivers are complaint with that, returning 0 on success
> (meaning that the device was successfully probed) or an error otherwise.

Great, no probems then :)

> Almost all media devices are made by a SoC or a RISC CPU that works
> as a DMA engine and exposes a set of registers to allow I2C access to the
> device's internal and/or external I2C buses. Part of them have an internal
> EEPROM/ROM that stores firmware internally at the board, but others require
> a firmware to be loaded before being able to init/control the device and to
> export the I2C bus interface.
> 
> The media handling function is then implemented via a series of I2C devices[1]:
> 	- analog video decoders;
> 	- TV tuners;
> 	- radio tuners;
> 	- I2C remote controller decoders;
> 	- DVB frontends;
> 	- mpeg decoders;
> 	- mpeg encoders;
> 	- video enhancement devices;
> 	...
> 
> [1] several media chips have part of those function implemented internally,
> but almost all require external I2C components to be probed.
> 
> In order to properly refer to each component, we call the "main" kernel module
> that talks with the media device via USB/PCI bus is called "bridge driver", 
> and the I2C components are called as "sub-devices".
> 
> Different vendors use the same bridge driver to work with different sub-devices.
> 
> It is a .probe()'s task to detect what sub-devices are there inside the board.
> 
> There are several cases where the vendor switched the sub-devices without
> changing the PCI ID/USB ID.

Hardware vendors suck, we all know that :(

> So, drivers do things like the code below, inside the .probe() callback:
> 
> static int check_if_dvb_frontend_is_supported_and_bind()
> {
> 	switch (device_model) {
> 	case VENDOR_A_MODEL_1:
> 		if (test_and_bind_frontend_1())	/* Doesn't require firmware */
> 			return 0;
> 		if (test_and_bind_frontend_2())	/* requires firmware "foo" */
> 			return 0;
> 		if (test_and_bind_frontend_3())	/* requires firmware "bar" */
> 			return 0;

Wait, why would these, if they require firmware, not load the firmware
at this point in time?  Why are they saying "all is fine" here?

> 		if (test_and_bind_frontend_4()) /* doesn't require firmware */
> 			return 0;
> 		break;
> 	case VENDOR_A_MODEL_2:
> 		/* Analog device - no DVB frontend on it */
> 		return 0;
> 	...
> 	}
> 	return -ENODEV;
> }
> 
> On several devices, before being able to register the bus and do the actual
> probe, the kernel needs to load a firmware.

That's common.

> Also, during the I2C device probing time, firmware may be required, in order
> to properly expose the device's internal models and their capabilities. 
> 
> For example, drx-k sub-device can have support for either DVB-C or DVB-T or both,
> depending on the device model. That affects the frontend properties exposed 
> to the user and might affect the bridge driver's initialization task.
> 
> In practice, a driver like em28xx have a few devices like HVR-930C that require
> the drx-k sub-device. For those devices, a firmware is required; for other
> devices, a firmware is not required.
> 
> What's happening is that newer versions of request_firmware and udev are being
> more pedantic (for a reason) about not requesting firmwares during module_init
> or PCI/USB register's probe callback.

It is?  What changed to require this?  What commit id?

> Worse than that, the same device driver may require a firmware or not, depending on
> the I2C devices inside it. One such example is em28xx: for the great majority of
> the supported devices, no firmware is needed, but devices like HVR-930C require
> a firmware, because it uses a frontend that needs firmware.
> 
> After some discussions, it seems that the best model would be to add an async_probe()
> callback to be used by devices similar to media ones. The async_probe() should be
> not probed during the module_init; the probe() will be deferred to happen later,
> when firmware's usermodehelper_disabled is false, allowing those drivers to load their
> firmwares if needed.
> 
> What do you think?

We already have deferred probe() calls, you just need to return a
different value (can't remember it at the moment), and your probe
function will be called again at a later time after the rest of the
devices in the system have been initialized.  Can that work for you?

Or, if you "know" you are going to accept this device, just return 0
from probe() after spawning a workqueue or thread to load the firmware
properly and do everything else you need to do to initialize.  If
something bad happens, unbind your device with a call to unbind things.

thanks,

greg k-h

Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 25-06-2012 19:33, Greg KH escreveu:
> On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote:
>> Greg,
>>
>> Basically, the recent changes at request_firmware() exposed an issue that
>> affects all media drivers that use firmware (64 drivers).
> 
> What change was that?  How did it break anything?

https://bugzilla.redhat.com/show_bug.cgi?id=827538

Basically, userspace changes and some pm-related patches, mainly this one [1]:

@@ -613,7 +606,14 @@ request_firmware(const struct firmware **firmware_p, const char *name,
	if (ret <= 0)
		return ret;
 
-	ret = _request_firmware(*firmware_p, name, device, true, false);
+	ret = usermodehelper_read_trylock();
+	if (WARN_ON(ret)) {
+		dev_err(device, "firmware: %s will not be loaded\n", name);
+	} else {
+		ret = _request_firmware(*firmware_p, name, device, true, false,
+				        firmware_loading_timeout());
+		usermodehelper_read_unlock();
+	}
	if (ret)
		_request_firmware_cleanup(firmware_p);
 
[1] at git commit 9b78c1da60b3c62ccdd1509f0902ad19ceaf776b

>> Driver's documentation at Documentation/driver-model/driver.txt says that the
>> .probe() callback should "bind the driver to a given device.  That includes
>> verifying that the device is present, that it's a version the driver can handle,
>> that driver data structures can be allocated and initialized, and that any
>> hardware can be initialized".
> 
> Yes.
> 
>> All media device drivers are complaint with that, returning 0 on success
>> (meaning that the device was successfully probed) or an error otherwise.
> 
> Great, no probems then :)
> 
>> Almost all media devices are made by a SoC or a RISC CPU that works
>> as a DMA engine and exposes a set of registers to allow I2C access to the
>> device's internal and/or external I2C buses. Part of them have an internal
>> EEPROM/ROM that stores firmware internally at the board, but others require
>> a firmware to be loaded before being able to init/control the device and to
>> export the I2C bus interface.
>>
>> The media handling function is then implemented via a series of I2C devices[1]:
>> 	- analog video decoders;
>> 	- TV tuners;
>> 	- radio tuners;
>> 	- I2C remote controller decoders;
>> 	- DVB frontends;
>> 	- mpeg decoders;
>> 	- mpeg encoders;
>> 	- video enhancement devices;
>> 	...
>>
>> [1] several media chips have part of those function implemented internally,
>> but almost all require external I2C components to be probed.
>>
>> In order to properly refer to each component, we call the "main" kernel module
>> that talks with the media device via USB/PCI bus is called "bridge driver",
>> and the I2C components are called as "sub-devices".
>>
>> Different vendors use the same bridge driver to work with different sub-devices.
>>
>> It is a .probe()'s task to detect what sub-devices are there inside the board.
>>
>> There are several cases where the vendor switched the sub-devices without
>> changing the PCI ID/USB ID.
> 
> Hardware vendors suck, we all know that :(
> 
>> So, drivers do things like the code below, inside the .probe() callback:
>>
>> static int check_if_dvb_frontend_is_supported_and_bind()
>> {
>> 	switch (device_model) {
>> 	case VENDOR_A_MODEL_1:
>> 		if (test_and_bind_frontend_1())	/* Doesn't require firmware */
>> 			return 0;
>> 		if (test_and_bind_frontend_2())	/* requires firmware "foo" */
>> 			return 0;
>> 		if (test_and_bind_frontend_3())	/* requires firmware "bar" */
>> 			return 0;
> 
> Wait, why would these, if they require firmware, not load the firmware
> at this point in time?  Why are they saying "all is fine" here?

The above is a prototype of what happens. The "test_and_bind_frontend_n()" 
logic are, in fact, more complex than that: it tries to load the firmware
inside each frontend's code when needed.

This is a real example (taken from ttpci budget driver):

	switch(budget->dev->pci->subsystem_device) {
	case 0x1003: // Hauppauge/TT Nova budget (stv0299/ALPS BSRU6(tsa5059) OR ves1893/ALPS BSRV2(sp5659))
	case 0x1013:
		// try the ALPS BSRV2 first of all
		budget->dvb_frontend = dvb_attach(ves1x93_attach, &alps_bsrv2_config, &budget->i2c_adap);
		if (budget->dvb_frontend) {
			budget->dvb_frontend->ops.tuner_ops.set_params = alps_bsrv2_tuner_set_params;
			budget->dvb_frontend->ops.diseqc_send_master_cmd = budget_diseqc_send_master_cmd;
			budget->dvb_frontend->ops.diseqc_send_burst = budget_diseqc_send_burst;
			budget->dvb_frontend->ops.set_tone = budget_set_tone;
			break;
		}

		// try the ALPS BSRU6 now
		budget->dvb_frontend = dvb_attach(stv0299_attach, &alps_bsru6_config, &budget->i2c_adap);
		if (budget->dvb_frontend) {
			budget->dvb_frontend->ops.tuner_ops.set_params = alps_bsru6_tuner_set_params;
			budget->dvb_frontend->tuner_priv = &budget->i2c_adap;
			if (budget->dev->pci->subsystem_device == 0x1003 && diseqc_method == 0) {
				budget->dvb_frontend->ops.diseqc_send_master_cmd = budget_diseqc_send_master_cmd;
				budget->dvb_frontend->ops.diseqc_send_burst = budget_diseqc_send_burst;
				budget->dvb_frontend->ops.set_tone = budget_set_tone;
			}
			break;
		}
		break;

>> 		if (test_and_bind_frontend_4()) /* doesn't require firmware */
>> 			return 0;
>> 		break;
>> 	case VENDOR_A_MODEL_2:
>> 		/* Analog device - no DVB frontend on it */
>> 		return 0;
>> 	...
>> 	}
>> 	return -ENODEV;
>> }
>>
>> On several devices, before being able to register the bus and do the actual
>> probe, the kernel needs to load a firmware.
> 
> That's common.
> 
>> Also, during the I2C device probing time, firmware may be required, in order
>> to properly expose the device's internal models and their capabilities.
>>
>> For example, drx-k sub-device can have support for either DVB-C or DVB-T or both,
>> depending on the device model. That affects the frontend properties exposed
>> to the user and might affect the bridge driver's initialization task.
>>
>> In practice, a driver like em28xx have a few devices like HVR-930C that require
>> the drx-k sub-device. For those devices, a firmware is required; for other
>> devices, a firmware is not required.
>>
>> What's happening is that newer versions of request_firmware and udev are being
>> more pedantic (for a reason) about not requesting firmwares during module_init
>> or PCI/USB register's probe callback.
> 
> It is?  What changed to require this?  What commit id?

See above.

>> Worse than that, the same device driver may require a firmware or not, depending on
>> the I2C devices inside it. One such example is em28xx: for the great majority of
>> the supported devices, no firmware is needed, but devices like HVR-930C require
>> a firmware, because it uses a frontend that needs firmware.
>>
>> After some discussions, it seems that the best model would be to add an async_probe()
>> callback to be used by devices similar to media ones. The async_probe() should be
>> not probed during the module_init; the probe() will be deferred to happen later,
>> when firmware's usermodehelper_disabled is false, allowing those drivers to load their
>> firmwares if needed.
>>
>> What do you think?
> 
> We already have deferred probe() calls, you just need to return a
> different value (can't remember it at the moment), and your probe
> function will be called again at a later time after the rest of the
> devices in the system have been initialized.  Can that work for you?

Yeah, I think so. From what I saw, devices that require firmware could do, inside
their probe routine:

	if (usermodehelper_disabled)
		return -EPROBE_DEFER;

As usermodehelper_disabled is an static symbol at kernel/kmod.c, we would need to
either export it globally or to create a small routine that would return it to
the drivers.

For bridge drivers, a change like that would be trivial. For I2C devices, such change
can be more complex, as the probe() routine will need to be broken into some stages,
one for the bridge driver's core and another one for each I2C device that can be deferred,
keeping a record on what it was deferred, but it seems feasible to me.

Thank you for the suggestion!
 
> Or, if you "know" you are going to accept this device, just return 0
> from probe() after spawning a workqueue or thread to load the firmware
> properly and do everything else you need to do to initialize.  If
> something bad happens, unbind your device with a call to unbind things.

Not sure if this would work, as there are some USB devices with multiple
interfaces and some are handled by other drivers (like mceusb and snd-usb-audio).

IMO, the deferred probe() is the right answer for this issue.

Thanks!
Mauro

[PATCH RFC 0/4] Defer probe() on em28xx when firmware load is required

[Mauro Carvalho Chehab]

This patch series is an attempt to solve the recent issues with udev-182
and media drivers.

The .probe() callback should "bind the driver to a given device.  That
includes verifying that the device is present, that it's a version the
driver can handle, that driver data structures can be allocated and 
initialized, and that any hardware can be initialized".

In order to comply witht he above, sometimes firmware is needed. However,
PM and udev have problems with that.

Newer versions of request_firmware block firmware load on some situations,
due to PM. Also, udev simply refuses to load firmware when module_init
doesn't finish, and .probe() can be called during USB/PCI bus register,
if the device is plugged.

This patch series consists of 4 patches:
	1) a change at kmod, in order to export the information that
	   userspace mode is disabled;

	2) a patch at em28xx that defers probe() if firmware is needed
	   and userspace mode is disabled;

	3) a workaround due to udev-182 limitation of not loading firmware
	   while a driver is modprobed;

	4) a patch for tuner-xc2028, in order to indicate what firmware
	   files are used there.

I was hoping that dracut-018-40.git20120522.fc17.noarch would get the
MODULE_FIRMWARE info while creating the initfs filesystem, copying there
the right firmwares, but, unfortunately, this didn't work (at least while
copiling tuners as builtin). Maybe will would honour it if I re-compile
everything as module and force dracut to load em28xx at init time.

Comments?

Regards,
Mauro

Mauro Carvalho Chehab (4):
  kmod: add a routine to return if usermode is disabled
  em28xx: defer probe() if udev is not ready to request_firmware
  em28xx: Workaround for new udev versions
  tuner-xc2028: tag the usual firmwares to help dracut

 drivers/media/common/tuners/tuner-xc2028.c |    2 +
 drivers/media/video/em28xx/em28xx-cards.c  |   73 +++++++++++++++++++++++++++-
 drivers/media/video/em28xx/em28xx.h        |    1 +
 include/linux/firmware.h                   |    6 +++
 kernel/kmod.c                              |    6 +++
 5 files changed, 87 insertions(+), 1 deletion(-)

-- 
1.7.10.2

[PATCH RFC 1/4] kmod: add a routine to return if usermode is disabled

[Mauro Carvalho Chehab]

Several media devices are only capable of probing the device if
the firmware load is enabled, e. g. when the usermode var is not
disabled.

Add a routine to allow those drivers to test if probe can continue
or need to be deferred.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 include/linux/firmware.h |    6 ++++++
 kernel/kmod.c            |    6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 1e7c011..cacf27e 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -35,6 +35,12 @@ struct builtin_fw {
 	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
 	__used __section(.builtin_fw) = { name, blob, size }
 
+/**
+ * is_usermodehelp_disabled - returns true if firmware usermode is disabled
+ *			      false otherwise.
+ */
+bool is_usermodehelp_disabled(void);
+
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 05698a7..68901308 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -339,6 +339,12 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
  */
 static DECLARE_WAIT_QUEUE_HEAD(usermodehelper_disabled_waitq);
 
+bool is_usermodehelp_disabled(void)
+{
+	return usermodehelper_disabled ? true : false;
+}
+EXPORT_SYMBOL_GPL(is_usermodehelp_disabled);
+
 /*
  * Time to wait for running_helpers to become zero before the setting of
  * usermodehelper_disabled in usermodehelper_disable() fails
-- 
1.7.10.2

Re: [PATCH RFC 1/4] kmod: add a routine to return if usermode is disabled

[Greg KH]

On Tue, Jun 26, 2012 at 04:34:19PM -0300, Mauro Carvalho Chehab wrote:
> Several media devices are only capable of probing the device if
> the firmware load is enabled, e. g. when the usermode var is not
> disabled.
> 
> Add a routine to allow those drivers to test if probe can continue
> or need to be deferred.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  include/linux/firmware.h |    6 ++++++
>  kernel/kmod.c            |    6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 1e7c011..cacf27e 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -35,6 +35,12 @@ struct builtin_fw {
>  	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
>  	__used __section(.builtin_fw) = { name, blob, size }
>  
> +/**
> + * is_usermodehelp_disabled - returns true if firmware usermode is disabled
> + *			      false otherwise.
> + */
> +bool is_usermodehelp_disabled(void);
> +
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>  int request_firmware(const struct firmware **fw, const char *name,
>  		     struct device *device);
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 05698a7..68901308 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -339,6 +339,12 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
>   */
>  static DECLARE_WAIT_QUEUE_HEAD(usermodehelper_disabled_waitq);
>  
> +bool is_usermodehelp_disabled(void)
> +{
> +	return usermodehelper_disabled ? true : false;
> +}
> +EXPORT_SYMBOL_GPL(is_usermodehelp_disabled);

usermodehelper_disabled is an enum, and the 0 value is UMH_ENABLED, so I
think you need to explicitly test for the correct value here, you don't
want to return something incorrectly for UMH_FREEZING, right?

greg k-h

Re: [PATCH RFC 1/4] kmod: add a routine to return if usermode is disabled

[Mauro Carvalho Chehab]

Em 26-06-2012 17:38, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 04:34:19PM -0300, Mauro Carvalho Chehab wrote:
>> Several media devices are only capable of probing the device if
>> the firmware load is enabled, e. g. when the usermode var is not
>> disabled.
>>
>> Add a routine to allow those drivers to test if probe can continue
>> or need to be deferred.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>   include/linux/firmware.h |    6 ++++++
>>   kernel/kmod.c            |    6 ++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>> index 1e7c011..cacf27e 100644
>> --- a/include/linux/firmware.h
>> +++ b/include/linux/firmware.h
>> @@ -35,6 +35,12 @@ struct builtin_fw {
>>   	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
>>   	__used __section(.builtin_fw) = { name, blob, size }
>>   
>> +/**
>> + * is_usermodehelp_disabled - returns true if firmware usermode is disabled
>> + *			      false otherwise.
>> + */
>> +bool is_usermodehelp_disabled(void);
>> +
>>   #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>>   int request_firmware(const struct firmware **fw, const char *name,
>>   		     struct device *device);
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 05698a7..68901308 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -339,6 +339,12 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
>>    */
>>   static DECLARE_WAIT_QUEUE_HEAD(usermodehelper_disabled_waitq);
>>   
>> +bool is_usermodehelp_disabled(void)
>> +{
>> +	return usermodehelper_disabled ? true : false;
>> +}
>> +EXPORT_SYMBOL_GPL(is_usermodehelp_disabled);
> 
> usermodehelper_disabled is an enum, and the 0 value is UMH_ENABLED, so I
> think you need to explicitly test for the correct value here, you don't
> want to return something incorrectly for UMH_FREEZING, right?

Yes. I'll change it to explicitly test for UMH_ENABLED.

Thanks!
Mauro

[PATCH RFC 2/4] em28xx: defer probe() if userspace mode is disabled

[Mauro Carvalho Chehab]

Some em28xx devices with tuner-xc2028, drx-k and drx-d need firmware, in
order to be probed. Defer device probe on those devices, if userspace mode is
disabled.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/video/em28xx/em28xx-cards.c |   42 +++++++++++++++++++++++++++++
 drivers/media/video/em28xx/em28xx.h       |    1 +
 2 files changed, 43 insertions(+)

diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 12bc54a..9229cd2 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -776,6 +776,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "Terratec Cinergy A Hybrid XS",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 
@@ -800,6 +801,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name	      = "KWorld PVRTV 300U",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -880,6 +882,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "Terratec Cinergy T XS",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 	},
 	[EM2870_BOARD_TERRATEC_XS_MT2060] = {
@@ -891,6 +894,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "Kworld 350 U DVB-T",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 	},
 	[EM2870_BOARD_KWORLD_355U] = {
@@ -919,6 +923,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "Terratec Hybrid XS Secam",
 		.has_msp34xx  = 1,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.has_dvb      = 1,
@@ -970,6 +975,7 @@ struct em28xx_board em28xx_boards[] = {
 		.i2c_speed    = EM2874_I2C_SECONDARY_BUS_SELECT |
 				EM28XX_I2C_CLK_WAIT_ENABLE |
 				EM28XX_I2C_FREQ_400_KHZ,
+		.needs_firmware = true,
 	},
 	[EM2884_BOARD_CINERGY_HTC_STICK] = {
 		.name         = "Terratec Cinergy HTC Stick",
@@ -979,11 +985,13 @@ struct em28xx_board em28xx_boards[] = {
 		.i2c_speed    = EM2874_I2C_SECONDARY_BUS_SELECT |
 				EM28XX_I2C_CLK_WAIT_ENABLE |
 				EM28XX_I2C_FREQ_400_KHZ,
+		.needs_firmware = true,
 	},
 	[EM2880_BOARD_HAUPPAUGE_WINTV_HVR_900] = {
 		.name         = "Hauppauge WinTV HVR 900",
 		.tda9887_conf = TDA9887_PRESENT,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.mts_firmware = 1,
 		.has_dvb      = 1,
@@ -1011,6 +1019,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "Hauppauge WinTV HVR 900 (R2)",
 		.tda9887_conf = TDA9887_PRESENT,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.mts_firmware = 1,
 		.has_dvb      = 1,
@@ -1037,6 +1046,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2883_BOARD_HAUPPAUGE_WINTV_HVR_850] = {
 		.name           = "Hauppauge WinTV HVR 850",
 		.tuner_type     = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio     = default_tuner_gpio,
 		.mts_firmware   = 1,
 		.has_dvb        = 1,
@@ -1063,6 +1073,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2883_BOARD_HAUPPAUGE_WINTV_HVR_950] = {
 		.name           = "Hauppauge WinTV HVR 950",
 		.tuner_type     = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio     = default_tuner_gpio,
 		.mts_firmware   = 1,
 		.has_dvb        = 1,
@@ -1089,6 +1100,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_PINNACLE_PCTV_HD_PRO] = {
 		.name           = "Pinnacle PCTV HD Pro Stick",
 		.tuner_type     = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.mts_firmware   = 1,
 		.has_dvb        = 1,
@@ -1115,6 +1127,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_AMD_ATI_TV_WONDER_HD_600] = {
 		.name           = "AMD ATI TV Wonder HD 600",
 		.tuner_type     = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio     = default_tuner_gpio,
 		.mts_firmware   = 1,
 		.has_dvb        = 1,
@@ -1141,6 +1154,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_TERRATEC_HYBRID_XS] = {
 		.name           = "Terratec Hybrid XS",
 		.tuner_type     = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio     = default_tuner_gpio,
 		.decoder        = EM28XX_TVP5150,
 		.has_dvb        = 1,
@@ -1170,6 +1184,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_TERRATEC_PRODIGY_XS] = {
 		.name         = "Terratec Prodigy XS",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -1412,6 +1427,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "MSI DigiVox A/D",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -1435,6 +1451,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "MSI DigiVox A/D II",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -1457,6 +1474,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_KWORLD_DVB_305U] = {
 		.name	      = "KWorld DVB-T 305U",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -1476,6 +1494,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_KWORLD_DVB_310U] = {
 		.name	      = "KWorld DVB-T 310U",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.has_dvb      = 1,
 		.dvb_gpio     = default_digital,
@@ -1534,6 +1553,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2880_BOARD_EMPIRE_DUAL_TV] = {
 		.name = "Empire dual TV",
 		.tuner_type = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio = default_tuner_gpio,
 		.has_dvb = 1,
 		.dvb_gpio = default_digital,
@@ -1560,6 +1580,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name         = "DNT DA2 Hybrid",
 		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.input        = { {
@@ -1582,6 +1603,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2881_BOARD_PINNACLE_HYBRID_PRO] = {
 		.name         = "Pinnacle Hybrid Pro",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.has_dvb      = 1,
@@ -1606,6 +1628,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2882_BOARD_PINNACLE_HYBRID_PRO_330E] = {
 		.name         = "Pinnacle Hybrid Pro (330e)",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.mts_firmware = 1,
 		.has_dvb      = 1,
@@ -1632,6 +1655,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2882_BOARD_KWORLD_VS_DVBT] = {
 		.name         = "Kworld VS-DVB-T 323UR",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.mts_firmware = 1,
@@ -1656,6 +1680,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2882_BOARD_TERRATEC_HYBRID_XS] = {
 		.name         = "Terratec Cinnergy Hybrid T USB XS (em2882)",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.mts_firmware = 1,
 		.decoder      = EM28XX_TVP5150,
@@ -1683,6 +1708,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2882_BOARD_DIKOM_DK300] = {
 		.name         = "Dikom DK300",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.mts_firmware = 1,
@@ -1698,6 +1724,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2883_BOARD_KWORLD_HYBRID_330U] = {
 		.name         = "Kworld PlusTV HD Hybrid 330",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.mts_firmware = 1,
@@ -1750,6 +1777,7 @@ struct em28xx_board em28xx_boards[] = {
 		.name	      = "Kaiomy TVnPC U2",
 		.vchannels    = 3,
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_addr   = 0x61,
 		.mts_firmware = 1,
 		.decoder      = EM28XX_TVP5150,
@@ -1864,6 +1892,7 @@ struct em28xx_board em28xx_boards[] = {
 	[EM2882_BOARD_EVGA_INDTUBE] = {
 		.name         = "Evga inDtube",
 		.tuner_type   = TUNER_XC2028,
+		.needs_firmware = true,
 		.tuner_gpio   = default_tuner_gpio,
 		.decoder      = EM28XX_TVP5150,
 		.xclk         = EM28XX_XCLK_FREQUENCY_12MHZ, /* NEC IR */
@@ -3133,6 +3162,19 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 	char *speed;
 
+	/*
+	 * If the device requires firmware, probe() may need to be
+	 * postponed, as udev may not be ready yet to honour firmware
+	 * load requests.
+	 */
+	if (em28xx_boards[id->driver_info].needs_firmware &&
+	    is_usermodehelp_disabled()) {
+		printk_once(KERN_DEBUG DRIVER_NAME
+		            ": probe deferred for board %d.\n",
+		            (unsigned)id->driver_info);
+		return -EPROBE_DEFER;
+	}
+
 	udev = usb_get_dev(interface_to_usbdev(interface));
 
 	/* Check to see next free device and mark as used */
diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index 8757523..ed8dc65 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -402,6 +402,7 @@ struct em28xx_board {
 	unsigned int is_webcam:1;
 	unsigned int valid:1;
 	unsigned int has_ir_i2c:1;
+	unsigned int needs_firmware:1;
 
 	unsigned char xclk, i2c_speed;
 	unsigned char radio_addr;
-- 
1.7.10.2

Re: [PATCH RFC 2/4] em28xx: defer probe() if userspace mode is disabled

[Greg KH]

On Tue, Jun 26, 2012 at 04:34:20PM -0300, Mauro Carvalho Chehab wrote:
> +	/*
> +	 * If the device requires firmware, probe() may need to be
> +	 * postponed, as udev may not be ready yet to honour firmware
> +	 * load requests.
> +	 */
> +	if (em28xx_boards[id->driver_info].needs_firmware &&
> +	    is_usermodehelp_disabled()) {
> +		printk_once(KERN_DEBUG DRIVER_NAME
> +		            ": probe deferred for board %d.\n",
> +		            (unsigned)id->driver_info);
> +		return -EPROBE_DEFER;

You should printk once per device, right?  Not just for one time per
module load.

Also, what about using dev_dbg()?  Is there a _once version that works
for that?

thanks,

greg k-h

Re: [PATCH RFC 2/4] em28xx: defer probe() if userspace mode is disabled

[Mauro Carvalho Chehab]

Em 26-06-2012 17:43, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 04:34:20PM -0300, Mauro Carvalho Chehab wrote:
>> +	/*
>> +	 * If the device requires firmware, probe() may need to be
>> +	 * postponed, as udev may not be ready yet to honour firmware
>> +	 * load requests.
>> +	 */
>> +	if (em28xx_boards[id->driver_info].needs_firmware &&
>> +	    is_usermodehelp_disabled()) {
>> +		printk_once(KERN_DEBUG DRIVER_NAME
>> +		            ": probe deferred for board %d.\n",
>> +		            (unsigned)id->driver_info);
>> +		return -EPROBE_DEFER;
> 
> You should printk once per device, right?  Not just for one time per
> module load.

Yes, a per-device printk would be better. In a matter of fact, the first
logs when the kernel boots are:

[    2.884645] em28xx: init = 0, userspace_is_disabled = 0, needs firmware = 1
[    2.884647] em28xx: probe deferred for board 16.
[    2.884650] usb 1-6:1.0: Driver em28xx requests probe deferral

as usb core is already telling that probe was referred, we can simply remove
it here.

> 
> Also, what about using dev_dbg()?  Is there a _once version that works
> for that?

There is a dev_WARN_ONCE(). Not sure if that would be a better replacement.
Probably not.

The entire em28xx driver needs to be replaced to use dev_dbg() instead of
implementing their own printk logic. This is one of the things that it is
on my todo list (with very low priority, when compared with other things).

Regards,
Mauro

[PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Mauro Carvalho Chehab]

New udev-182 seems to be buggy: even when usermode is enabled, it
insists on needing that probe would defer any firmware requests.
So, drivers with firmware need to defer probe for the first
driver's core request, otherwise an useless penalty of 30 seconds
happens, as udev will refuse to load any firmware.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---

Note: this patch adds an ugly printk there, in order to allow testing it better.
This will be removed at the final version.

 drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 9229cd2..9a1c16c 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
 module_param_array(card,  int, NULL, 0444);
 MODULE_PARM_DESC(card,     "card type");
 
+static bool is_em28xx_initialized;
+
 /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
 
@@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	 * postponed, as udev may not be ready yet to honour firmware
 	 * load requests.
 	 */
+printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
+	is_em28xx_initialized,
+	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);
 	if (em28xx_boards[id->driver_info].needs_firmware &&
-	    is_usermodehelp_disabled()) {
-		printk_once(KERN_DEBUG DRIVER_NAME
-		            ": probe deferred for board %d.\n",
-		            (unsigned)id->driver_info);
+	    (!is_em28xx_initialized || is_usermodehelp_disabled())) {
+		printk(KERN_DEBUG DRIVER_NAME
+		       ": probe deferred for board %d.\n",
+		       (unsigned)id->driver_info);
 		return -EPROBE_DEFER;
 	}
 
@@ -3456,4 +3461,28 @@ static struct usb_driver em28xx_usb_driver = {
 	.id_table = em28xx_id_table,
 };
 
-module_usb_driver(em28xx_usb_driver);
+static int __init em28xx_module_init(void)
+{
+	int result;
+
+	/* register this driver with the USB subsystem */
+	result = usb_register(&em28xx_usb_driver);
+	if (result)
+		em28xx_err(DRIVER_NAME
+			   " usb_register failed. Error number %d.\n", result);
+
+	printk(KERN_INFO DRIVER_NAME " driver loaded\n");
+
+	is_em28xx_initialized = true;
+
+	return result;
+}
+
+static void __exit em28xx_module_exit(void)
+{
+	/* deregister this driver with the USB subsystem */
+	usb_deregister(&em28xx_usb_driver);
+}
+
+module_init(em28xx_module_init);
+module_exit(em28xx_module_exit);
-- 
1.7.10.2

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Greg KH]

On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
> New udev-182 seems to be buggy: even when usermode is enabled, it
> insists on needing that probe would defer any firmware requests.
> So, drivers with firmware need to defer probe for the first
> driver's core request, otherwise an useless penalty of 30 seconds
> happens, as udev will refuse to load any firmware.

Shouldn't you fix udev, if it really is a problem here?  Papering over
userspace bugs in the kernel isn't usually a good thing to do, as odds
are, it will hit some other driver sometime, right?

greg k-h

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Mauro Carvalho Chehab]

Em 26-06-2012 17:40, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
>> New udev-182 seems to be buggy: even when usermode is enabled, it
>> insists on needing that probe would defer any firmware requests.
>> So, drivers with firmware need to defer probe for the first
>> driver's core request, otherwise an useless penalty of 30 seconds
>> happens, as udev will refuse to load any firmware.
> 
> Shouldn't you fix udev, if it really is a problem here?  Papering over
> userspace bugs in the kernel isn't usually a good thing to do, as odds
> are, it will hit some other driver sometime, right?

That's my opinion too, but Kay seems to think otherwise. On his opinion,
waiting for firmware during module_init() is something that were never
allowed at the device model.

Regards,
Mauro

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Kay Sievers]

On Tue, 2012-06-26 at 18:07 -0300, Mauro Carvalho Chehab wrote:
> Em 26-06-2012 17:40, Greg KH escreveu:
> > On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
> >> New udev-182 seems to be buggy: even when usermode is enabled, it
> >> insists on needing that probe would defer any firmware requests.
> >> So, drivers with firmware need to defer probe for the first
> >> driver's core request, otherwise an useless penalty of 30 seconds
> >> happens, as udev will refuse to load any firmware.
> > 
> > Shouldn't you fix udev, if it really is a problem here?  Papering over
> > userspace bugs in the kernel isn't usually a good thing to do, as odds
> > are, it will hit some other driver sometime, right?
> 
> That's my opinion too, but Kay seems to think otherwise. On his opinion,
> waiting for firmware during module_init() is something that were never
> allowed at the device model.

No, that's not at all an udev *bug*, the changelog in this patch is just
plain wrong. It's just udev making noise about a broken driver behavior.
And it's the messenger, not the problem.

Kernel modules must not block in module_init() in a userspace
transaction (fw load) that can take an unpredictable amount of time. It
results in broken suspend/resume paths, or broken compiled-in module
behaviour, and a modprobe processes which hangs uninterruptible until
the firmware timeout happens.

Uevents have dependencies, if a parent device event calls modprobe, the
child device it creates will waits for the parent event to finish, but
if the parent blocks in modprobe, it will not finish and we run into the
deadlock udev complains about.

Udev used to work around that, that workaround we turned into the logged
error we see now. Again, uninterruptible blocking of module_init() in a
in-kernel callout-to-userspace is not proper driver behavior, and needs
to be changed. 

Thanks,
Kay

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Greg KH]

On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
> New udev-182 seems to be buggy: even when usermode is enabled, it
> insists on needing that probe would defer any firmware requests.
> So, drivers with firmware need to defer probe for the first
> driver's core request, otherwise an useless penalty of 30 seconds
> happens, as udev will refuse to load any firmware.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
> 
> Note: this patch adds an ugly printk there, in order to allow testing it better.
> This will be removed at the final version.
> 
>  drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
> index 9229cd2..9a1c16c 100644
> --- a/drivers/media/video/em28xx/em28xx-cards.c
> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> @@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>  module_param_array(card,  int, NULL, 0444);
>  MODULE_PARM_DESC(card,     "card type");
>  
> +static bool is_em28xx_initialized;
> +
>  /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
>  static unsigned long em28xx_devused;
>  
> @@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>  	 * postponed, as udev may not be ready yet to honour firmware
>  	 * load requests.
>  	 */
> +printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
> +	is_em28xx_initialized,
> +	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);

debug code?

Also, this doesn't seem wise.  probe() will be called and
is_em28xx_initialized will be 0 before it can be set if the device is
present when the module is loaded.  But, if a new device is added to the
system after probe() already runs, is_em28xx_initialized will be 1, yet
it isn't true for this new device.

So this doesn't seem like a valid solution, even if you were wanting to
paper over a udev bug.


>  	if (em28xx_boards[id->driver_info].needs_firmware &&
> -	    is_usermodehelp_disabled()) {
> -		printk_once(KERN_DEBUG DRIVER_NAME
> -		            ": probe deferred for board %d.\n",
> -		            (unsigned)id->driver_info);
> +	    (!is_em28xx_initialized || is_usermodehelp_disabled())) {
> +		printk(KERN_DEBUG DRIVER_NAME
> +		       ": probe deferred for board %d.\n",
> +		       (unsigned)id->driver_info);
>  		return -EPROBE_DEFER;
>  	}
>  
> @@ -3456,4 +3461,28 @@ static struct usb_driver em28xx_usb_driver = {
>  	.id_table = em28xx_id_table,
>  };
>  
> -module_usb_driver(em28xx_usb_driver);

Hint, if you are removing this macro, you can almost be assured that you
are doing something wrong :)

thanks,

greg k-h

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Mauro Carvalho Chehab]

Em 26-06-2012 17:42, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
>> New udev-182 seems to be buggy: even when usermode is enabled, it
>> insists on needing that probe would defer any firmware requests.
>> So, drivers with firmware need to defer probe for the first
>> driver's core request, otherwise an useless penalty of 30 seconds
>> happens, as udev will refuse to load any firmware.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>
>> Note: this patch adds an ugly printk there, in order to allow testing it better.
>> This will be removed at the final version.
>>
>>   drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
>>   1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
>> index 9229cd2..9a1c16c 100644
>> --- a/drivers/media/video/em28xx/em28xx-cards.c
>> +++ b/drivers/media/video/em28xx/em28xx-cards.c
>> @@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>>   module_param_array(card,  int, NULL, 0444);
>>   MODULE_PARM_DESC(card,     "card type");
>>   
>> +static bool is_em28xx_initialized;
>> +
>>   /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
>>   static unsigned long em28xx_devused;
>>   
>> @@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>   	 * postponed, as udev may not be ready yet to honour firmware
>>   	 * load requests.
>>   	 */
>> +printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
>> +	is_em28xx_initialized,
>> +	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);
> 
> debug code?

Yes, temporary debug code, for people @linux-media that might be interested
on testing the patch. It will be removed at the final version, of course.

> Also, this doesn't seem wise.  probe() will be called and
> is_em28xx_initialized will be 0 before it can be set if the device is
> present when the module is loaded.  But, if a new device is added to the
> system after probe() already runs, is_em28xx_initialized will be 1, yet
> it isn't true for this new device.

Yes.
>
> So this doesn't seem like a valid solution, even if you were wanting to
> paper over a udev bug.

The problem with udev-182 is that it blocks firmware load while
mode_init() is happening. Only after the end of module_init(), udev will
handle request_firmware.

This is what happens before this patch series:

[    3.605783] tvp5150 0-005c: tvp5150am1 detected.
[    3.627674] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
[    3.633695] xc2028 0-0061: creating new instance
[    3.638406] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner
[   64.422633] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
[   64.429090] em28xx #0: Config register raw data: 0xd0
[   64.434959] em28xx #0: AC97 vendor ID = 0xffffffff
[   64.440206] em28xx #0: AC97 features = 0x6a90
[   64.444654] em28xx #0: Empia 202 AC97 audio processor detected
[   64.607494] em28xx #0: v4l2 driver version 0.1.3
[  125.574760] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
[  125.645012] em28xx #0: V4L2 video device registered as video0
[  125.650851] em28xx #0: V4L2 VBI device registered as vbi0

The 60s delay is due to the bug (firmware doesn't load there just
because I didn't ask dracut to add it there).

After the patch series, the artificial delay introduced due to udev-182
goes away:

[    2.884657] usbcore: registered new interface driver em28xx
[    2.884657] em28xx driver loaded
[    3.123482] em28xx: init = 1, userspace_is_disabled = 0, needs firmware = 1
[    3.123489] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, interface 0, class 0)
[    3.123491] em28xx: Audio Vendor Class interface 0 found
[    3.123492] em28xx: Video interface 0 found
[    3.123493] em28xx: DVB interface 0 found
[    3.123633] em28xx #0: chip ID is em2882/em2883
[    3.267680] em28xx #0: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 82 1e
[    3.283034] em28xx #0: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 00 00 00 00
[    3.291857] em28xx #0: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 5b 1c 00 00
[    3.300682] em28xx #0: i2c eeprom 30: 00 00 20 40 20 80 02 20
[    3.315397] em28xx #0: i2c eeprom 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    3.324263] em28xx #0: i2c eeprom 50: 00 00 00 00 00 00
[    3.339330] em28xx #0: i2c eeprom 60: 00 00 00 00 00 00 00 00 00 00 18 03 34 00 30 00
[    3.348164] em28xx #0: i2c eeprom 70: 32 00 38 00 34 00 34 00 39 00 30 00 31 00 38 00
[    3.356996] em28xx #0: i2c eeprom 80: 00 00 1e 03 57 00 69 00 6e 00 54 00 56 00 20 00
[    3.365834] em28xx #0: i2c eeprom 90: 48 00 56 00 52 00 2d 00 39 00 38 00 30 00 00 00
[    3.371533] em28xx #0: i2c eeprom a0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[    3.371537] em28xx #0: i2c eeprom b0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[    3.371540] em28xx #0: i2c eeprom c0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[    3.371544] em28xx #0: i2c eeprom d0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[    3.371547] em28xx #0: i2c eeprom e0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[    3.371551] em28xx #0: i2c eeprom f0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[    3.371556] em28xx #0: EEPROM ID= 0x9567eb1a, EEPROM hash = 0x994b2bdd
[    3.371557] em28xx #0: EEPROM info:
[    3.371558] em28xx #0:	AC97 audio (5 sample rates)
[    3.371558] em28xx #0:	500mA max power
[    3.371560] em28xx #0:	Table at 0x24, strings=0x1e82, 0x186a, 0x0000
[    3.371562] em28xx #0: Identified as Hauppauge WinTV HVR 950 (card=16)
[    3.487464] tvp5150 0-005c: chip found @ 0xb8 (em28xx #0)
[    3.814980] em28xx #0: Config register raw data: 0xd0
[    3.820851] em28xx #0: AC97 vendor ID = 0xffffffff
[    3.826129] em28xx #0: AC97 features = 0x6a90
[    3.830561] em28xx #0: Empia 202 AC97 audio processor detected
[    3.996943] em28xx #0: v4l2 driver version 0.1.3
[    4.078987] em28xx #0: V4L2 video device registered as video0
[    4.078989] em28xx #0: V4L2 VBI device registered as vbi0
[    4.137199] em28xx #0: em28xx #0/2: xc3028 attached
[    4.173927] DVB: registering new adapter (em28xx #0)
[    4.188846] em28xx #0: Successfully loaded em28xx-dvb

In other words, only the .probe() that is called during usb_register() is affected
by this behavior. Subsequent device additions/removals aren't affected:

[ 8125.049183] xc2028 0-0061: destroying instance
[ 8127.217933] usb 1-6: new high-speed USB device number 3 using ehci_hcd
[ 8127.343456] usb 1-6: New USB device found, idVendor=2040, idProduct=6513
[ 8127.350144] usb 1-6: New USB device strings: Mfr=0, Product=1, SerialNumber=2
[ 8127.357261] usb 1-6: Product: WinTV HVR-980
[ 8127.361428] usb 1-6: SerialNumber: 4028449018
[ 8127.366176] em28xx: init = 1, userspace_is_disabled = 0, needs firmware = 1
[ 8127.373153] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, interface 0, class 0)
[ 8127.381416] em28xx: Audio Vendor Class interface 0 found
[ 8127.386709] em28xx: Video interface 0 found
[ 8127.390879] em28xx: DVB interface 0 found
[ 8127.395049] em28xx #0: chip ID is em2882/em2883
[ 8127.542105] em28xx #0: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 82 1e 6a 18
[ 8127.550112] em28xx #0: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 00 00 00 00
[ 8127.558090] em28xx #0: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 5b 1c 00 00
[ 8127.566074] em28xx #0: i2c eeprom 30: 00 00 20 40 20 80 02 20 01 01 01 01 00 00 00 00
[ 8127.574065] em28xx #0: i2c eeprom 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 8127.582050] em28xx #0: i2c eeprom 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 8127.590037] em28xx #0: i2c eeprom 60: 00 00 00 00 00 00 00 00 00 00 18 03 34 00 30 00
[ 8127.598023] em28xx #0: i2c eeprom 70: 32 00 38 00 34 00 34 00 39 00 30 00 31 00 38 00
[ 8127.606008] em28xx #0: i2c eeprom 80: 00 00 1e 03 57 00 69 00 6e 00 54 00 56 00 20 00
[ 8127.613989] em28xx #0: i2c eeprom 90: 48 00 56 00 52 00 2d 00 39 00 38 00 30 00 00 00
[ 8127.621979] em28xx #0: i2c eeprom a0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[ 8127.629967] em28xx #0: i2c eeprom b0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[ 8127.637951] em28xx #0: i2c eeprom c0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[ 8127.645938] em28xx #0: i2c eeprom d0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[ 8127.653924] em28xx #0: i2c eeprom e0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[ 8127.661912] em28xx #0: i2c eeprom f0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[ 8127.669898] em28xx #0: EEPROM ID= 0x9567eb1a, EEPROM hash = 0x994b2bdd
[ 8127.676402] em28xx #0: EEPROM info:
[ 8127.679875] em28xx #0:	AC97 audio (5 sample rates)
[ 8127.684648] em28xx #0:	500mA max power
[ 8127.688384] em28xx #0:	Table at 0x24, strings=0x1e82, 0x186a, 0x0000
[ 8127.694714] em28xx #0: Identified as Hauppauge WinTV HVR 950 (card=16)
[ 8127.701218] tveeprom 0-0050: Hauppauge model 65201, rev A1C0, serial# 1917178
[ 8127.708327] tveeprom 0-0050: tuner model is Xceive XC3028 (idx 120, type 71)
[ 8127.715351] tveeprom 0-0050: TV standards PAL(B/G) PAL(I) PAL(D/D1/K) ATSC/DVB Digital (eeprom 0xd4)
[ 8127.724451] tveeprom 0-0050: audio processor is None (idx 0)
[ 8127.730090] tveeprom 0-0050: has radio
[ 8127.735621] tvp5150 0-005c: chip found @ 0xb8 (em28xx #0)
[ 8127.788094] tvp5150 0-005c: tvp5150am1 detected.
[ 8127.810607] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
[ 8127.816574] xc2028 0-0061: creating new instance
[ 8127.821183] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner

>>   	if (em28xx_boards[id->driver_info].needs_firmware &&
>> -	    is_usermodehelp_disabled()) {
>> -		printk_once(KERN_DEBUG DRIVER_NAME
>> -		            ": probe deferred for board %d.\n",
>> -		            (unsigned)id->driver_info);
>> +	    (!is_em28xx_initialized || is_usermodehelp_disabled())) {
>> +		printk(KERN_DEBUG DRIVER_NAME
>> +		       ": probe deferred for board %d.\n",
>> +		       (unsigned)id->driver_info);
>>   		return -EPROBE_DEFER;
>>   	}
>>   
>> @@ -3456,4 +3461,28 @@ static struct usb_driver em28xx_usb_driver = {
>>   	.id_table = em28xx_id_table,
>>   };
>>   
>> -module_usb_driver(em28xx_usb_driver);
> 
> Hint, if you are removing this macro, you can almost be assured that you
> are doing something wrong :)

:)

Regards,
Mauro

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Greg KH]

On Tue, Jun 26, 2012 at 06:21:02PM -0300, Mauro Carvalho Chehab wrote:
> Em 26-06-2012 17:42, Greg KH escreveu:
> > On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
> >> New udev-182 seems to be buggy: even when usermode is enabled, it
> >> insists on needing that probe would defer any firmware requests.
> >> So, drivers with firmware need to defer probe for the first
> >> driver's core request, otherwise an useless penalty of 30 seconds
> >> happens, as udev will refuse to load any firmware.
> >>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >> ---
> >>
> >> Note: this patch adds an ugly printk there, in order to allow testing it better.
> >> This will be removed at the final version.
> >>
> >>   drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
> >>   1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
> >> index 9229cd2..9a1c16c 100644
> >> --- a/drivers/media/video/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> >> @@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
> >>   module_param_array(card,  int, NULL, 0444);
> >>   MODULE_PARM_DESC(card,     "card type");
> >>   
> >> +static bool is_em28xx_initialized;
> >> +
> >>   /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
> >>   static unsigned long em28xx_devused;
> >>   
> >> @@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> >>   	 * postponed, as udev may not be ready yet to honour firmware
> >>   	 * load requests.
> >>   	 */
> >> +printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
> >> +	is_em28xx_initialized,
> >> +	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);
> > 
> > debug code?
> 
> Yes, temporary debug code, for people @linux-media that might be interested
> on testing the patch. It will be removed at the final version, of course.
> 
> > Also, this doesn't seem wise.  probe() will be called and
> > is_em28xx_initialized will be 0 before it can be set if the device is
> > present when the module is loaded.  But, if a new device is added to the
> > system after probe() already runs, is_em28xx_initialized will be 1, yet
> > it isn't true for this new device.
> 
> Yes.

So you really want that?  That doesn't make any sense, sorry.

> > So this doesn't seem like a valid solution, even if you were wanting to
> > paper over a udev bug.
> 
> The problem with udev-182 is that it blocks firmware load while
> mode_init() is happening. Only after the end of module_init(), udev will
> handle request_firmware.

How does udev "know" that module_init() is completed, or even in the
picture at all?  Is this due to the change to use kmod?

And you really should be using the async firmware loading path, that
would solve this problem entirely, right?

> This is what happens before this patch series:
> 
> [    3.605783] tvp5150 0-005c: tvp5150am1 detected.
> [    3.627674] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
> [    3.633695] xc2028 0-0061: creating new instance
> [    3.638406] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner
> [   64.422633] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
> [   64.429090] em28xx #0: Config register raw data: 0xd0
> [   64.434959] em28xx #0: AC97 vendor ID = 0xffffffff
> [   64.440206] em28xx #0: AC97 features = 0x6a90
> [   64.444654] em28xx #0: Empia 202 AC97 audio processor detected
> [   64.607494] em28xx #0: v4l2 driver version 0.1.3
> [  125.574760] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
> [  125.645012] em28xx #0: V4L2 video device registered as video0
> [  125.650851] em28xx #0: V4L2 VBI device registered as vbi0
> 
> The 60s delay is due to the bug (firmware doesn't load there just
> because I didn't ask dracut to add it there).

Ok, if you ask for firmware that you don't have, stalling is normal.
Not good though, and one big reason you should switch to using the async
model of firmware loading (a long time ago I wanted that to be the only
model, but I lost that argument...)

> After the patch series, the artificial delay introduced due to udev-182
> goes away:

Wait, if the firmware isn't present, how could any delay go away?  Why
would it go away?

still confused,

greg k-h

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Mauro Carvalho Chehab]

Em 26-06-2012 18:27, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 06:21:02PM -0300, Mauro Carvalho Chehab wrote:
>> Em 26-06-2012 17:42, Greg KH escreveu:
>>> On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
>>>> New udev-182 seems to be buggy: even when usermode is enabled, it
>>>> insists on needing that probe would defer any firmware requests.
>>>> So, drivers with firmware need to defer probe for the first
>>>> driver's core request, otherwise an useless penalty of 30 seconds
>>>> happens, as udev will refuse to load any firmware.
>>>>
>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>> ---
>>>>
>>>> Note: this patch adds an ugly printk there, in order to allow testing it better.
>>>> This will be removed at the final version.
>>>>
>>>>    drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
>>>>    1 file changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
>>>> index 9229cd2..9a1c16c 100644
>>>> --- a/drivers/media/video/em28xx/em28xx-cards.c
>>>> +++ b/drivers/media/video/em28xx/em28xx-cards.c
>>>> @@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>>>>    module_param_array(card,  int, NULL, 0444);
>>>>    MODULE_PARM_DESC(card,     "card type");
>>>>    
>>>> +static bool is_em28xx_initialized;
>>>> +
>>>>    /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
>>>>    static unsigned long em28xx_devused;
>>>>    
>>>> @@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>>>    	 * postponed, as udev may not be ready yet to honour firmware
>>>>    	 * load requests.
>>>>    	 */
>>>> +printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
>>>> +	is_em28xx_initialized,
>>>> +	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);
>>>
>>> debug code?
>>
>> Yes, temporary debug code, for people @linux-media that might be interested
>> on testing the patch. It will be removed at the final version, of course.
>>
>>> Also, this doesn't seem wise.  probe() will be called and
>>> is_em28xx_initialized will be 0 before it can be set if the device is
>>> present when the module is loaded.  But, if a new device is added to the
>>> system after probe() already runs, is_em28xx_initialized will be 1, yet
>>> it isn't true for this new device.
>>
>> Yes.
> 
> So you really want that?  That doesn't make any sense, sorry.
> 
>>> So this doesn't seem like a valid solution, even if you were wanting to
>>> paper over a udev bug.
>>
>> The problem with udev-182 is that it blocks firmware load while
>> mode_init() is happening. Only after the end of module_init(), udev will
>> handle request_firmware.
> 
> How does udev "know" that module_init() is completed, or even in the
> picture at all?  Is this due to the change to use kmod?

Yes, I suspect so. It is probably running on a single thread mode. So,
while driver is loaading/probing, it is blocking request firmwares on
that driver.

> And you really should be using the async firmware loading path, that
> would solve this problem entirely, right?

As already explained, async firmware won't solve, as probe() cannot complete
without firmware. Em28xx is a good media device Citizen (and it might be possible
to use async firmware on this particular case), as the em28xx bridge
firmware is stored on a ROM memory inside the chip, but devices based
on Cypress FX2 CPU can only be probed after firmware load.

> 
>> This is what happens before this patch series:
>>
>> [    3.605783] tvp5150 0-005c: tvp5150am1 detected.
>> [    3.627674] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
>> [    3.633695] xc2028 0-0061: creating new instance
>> [    3.638406] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner
>> [   64.422633] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
>> [   64.429090] em28xx #0: Config register raw data: 0xd0
>> [   64.434959] em28xx #0: AC97 vendor ID = 0xffffffff
>> [   64.440206] em28xx #0: AC97 features = 0x6a90
>> [   64.444654] em28xx #0: Empia 202 AC97 audio processor detected
>> [   64.607494] em28xx #0: v4l2 driver version 0.1.3
>> [  125.574760] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
>> [  125.645012] em28xx #0: V4L2 video device registered as video0
>> [  125.650851] em28xx #0: V4L2 VBI device registered as vbi0
>>
>> The 60s delay is due to the bug (firmware doesn't load there just
>> because I didn't ask dracut to add it there).
> 
> Ok, if you ask for firmware that you don't have, stalling is normal.
> Not good though, and one big reason you should switch to using the async
> model of firmware loading (a long time ago I wanted that to be the only
> model, but I lost that argument...)
> 
>> After the patch series, the artificial delay introduced due to udev-182
>> goes away:
> 
> Wait, if the firmware isn't present, how could any delay go away?  Why
> would it go away?
> 
> still confused,

Good point. I'll do more tests there, forcing dracut to store the firmware
for this device at initfs and see what happens with and without the patch.

Maybe there's something else happening here.

Thanks!
Mauro

Re: [PATCH RFC 3/4] em28xx: Workaround for new udev versions

[Mauro Carvalho Chehab]

Em 26-06-2012 19:01, Mauro Carvalho Chehab escreveu:
> Em 26-06-2012 18:27, Greg KH escreveu:
>> On Tue, Jun 26, 2012 at 06:21:02PM -0300, Mauro Carvalho Chehab wrote:
>>> Em 26-06-2012 17:42, Greg KH escreveu:
>>>> On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:
>>>>> New udev-182 seems to be buggy: even when usermode is enabled, it
>>>>> insists on needing that probe would defer any firmware requests.
>>>>> So, drivers with firmware need to defer probe for the first
>>>>> driver's core request, otherwise an useless penalty of 30 seconds
>>>>> happens, as udev will refuse to load any firmware.
>>>>>
>>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>>> ---
>>>>>
>>>>> Note: this patch adds an ugly printk there, in order to allow testing it better.
>>>>> This will be removed at the final version.
>>>>>
>>>>>     drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
>>>>>     1 file changed, 34 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
>>>>> index 9229cd2..9a1c16c 100644
>>>>> --- a/drivers/media/video/em28xx/em28xx-cards.c
>>>>> +++ b/drivers/media/video/em28xx/em28xx-cards.c
>>>>> @@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>>>>>     module_param_array(card,  int, NULL, 0444);
>>>>>     MODULE_PARM_DESC(card,     "card type");
>>>>>     
>>>>> +static bool is_em28xx_initialized;
>>>>> +
>>>>>     /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
>>>>>     static unsigned long em28xx_devused;
>>>>>     
>>>>> @@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>>>>     	 * postponed, as udev may not be ready yet to honour firmware
>>>>>     	 * load requests.
>>>>>     	 */
>>>>> +printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
>>>>> +	is_em28xx_initialized,
>>>>> +	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);
>>>>
>>>> debug code?
>>>
>>> Yes, temporary debug code, for people @linux-media that might be interested
>>> on testing the patch. It will be removed at the final version, of course.
>>>
>>>> Also, this doesn't seem wise.  probe() will be called and
>>>> is_em28xx_initialized will be 0 before it can be set if the device is
>>>> present when the module is loaded.  But, if a new device is added to the
>>>> system after probe() already runs, is_em28xx_initialized will be 1, yet
>>>> it isn't true for this new device.
>>>
>>> Yes.
>>
>> So you really want that?  That doesn't make any sense, sorry.
>>
>>>> So this doesn't seem like a valid solution, even if you were wanting to
>>>> paper over a udev bug.
>>>
>>> The problem with udev-182 is that it blocks firmware load while
>>> mode_init() is happening. Only after the end of module_init(), udev will
>>> handle request_firmware.
>>
>> How does udev "know" that module_init() is completed, or even in the
>> picture at all?  Is this due to the change to use kmod?
> 
> Yes, I suspect so. It is probably running on a single thread mode. So,
> while driver is loaading/probing, it is blocking request firmwares on
> that driver.
> 
>> And you really should be using the async firmware loading path, that
>> would solve this problem entirely, right?
> 
> As already explained, async firmware won't solve, as probe() cannot complete
> without firmware. Em28xx is a good media device Citizen (and it might be possible
> to use async firmware on this particular case), as the em28xx bridge
> firmware is stored on a ROM memory inside the chip, but devices based
> on Cypress FX2 CPU can only be probed after firmware load.
> 
>>
>>> This is what happens before this patch series:
>>>
>>> [    3.605783] tvp5150 0-005c: tvp5150am1 detected.
>>> [    3.627674] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
>>> [    3.633695] xc2028 0-0061: creating new instance
>>> [    3.638406] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner
>>> [   64.422633] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
>>> [   64.429090] em28xx #0: Config register raw data: 0xd0
>>> [   64.434959] em28xx #0: AC97 vendor ID = 0xffffffff
>>> [   64.440206] em28xx #0: AC97 features = 0x6a90
>>> [   64.444654] em28xx #0: Empia 202 AC97 audio processor detected
>>> [   64.607494] em28xx #0: v4l2 driver version 0.1.3
>>> [  125.574760] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
>>> [  125.645012] em28xx #0: V4L2 video device registered as video0
>>> [  125.650851] em28xx #0: V4L2 VBI device registered as vbi0
>>>
>>> The 60s delay is due to the bug (firmware doesn't load there just
>>> because I didn't ask dracut to add it there).
>>
>> Ok, if you ask for firmware that you don't have, stalling is normal.
>> Not good though, and one big reason you should switch to using the async
>> model of firmware loading (a long time ago I wanted that to be the only
>> model, but I lost that argument...)
>>
>>> After the patch series, the artificial delay introduced due to udev-182
>>> goes away:
>>
>> Wait, if the firmware isn't present, how could any delay go away?  Why
>> would it go away?
>>
>> still confused,
> 
> Good point. I'll do more tests there, forcing dracut to store the firmware
> for this device at initfs and see what happens with and without the patch.
> 
> Maybe there's something else happening here.

The deferred probe() doesn't work as I would expect. It will only "flush" the
deferred events when a new device is plugged.

This is what happens with the patches applied and driver compiled as module
(so, the firmware file for xc3028 is there):

[   13.466116] Linux video capture interface: v2.00
[   14.161928] em28xx: init = 0, userspace_is_disabled = 0, needs firmware = 1
[   14.169149] em28xx: probe deferred for board 16.
[   14.169167] usb 1-6:1.0: Driver em28xx requests probe deferral
[   14.175271] usbcore: registered new interface driver em28xx
[   14.181062] em28xx driver loaded
[   14.616160] snd_hda_intel 0000:00:1b.0: irq 67 for MSI/MSI-X
...

[   14.810557] em28xx: init = 1, userspace_is_disabled = 0, needs firmware = 1
[   14.817698] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, interface 0, class 0)
[   14.826135] em28xx: Audio Vendor Class interface 0 found
...
[   16.570085] xc2028: Xcv2028/3028 init called!
...
[   16.581414] xc2028 3-0061: Reading firmware xc3028-v27.fw


However, If I remove the modules and re-insert them, with the device connected,
the em28xx device load fails:

[ 1081.216543] usbcore: registered new interface driver em28xx
[ 1081.222094] em28xx driver loaded

Only after connecting another device, the probe will happen:

[ 1093.278511] usb 1-6.4: new high-speed USB device number 11 using ehci_hcd
[ 1093.360211] usb 1-6.4: New USB device found, idVendor=eb1a, idProduct=2875
[ 1093.367074] usb 1-6.4: New USB device strings: Mfr=0, Product=1, SerialNumber=2
[ 1093.374363] usb 1-6.4: Product: USB 2875 Device
[ 1093.378876] usb 1-6.4: SerialNumber: 123456789ABCD?USB 2875 Device
[ 1093.385431] em28xx: init = 1, userspace_is_disabled = 0, needs firmware = 0
[ 1093.392449] em28xx: New device  USB 2875 Device @ 480 Mbps (eb1a:2875, interface 0, class 0)
[ 1093.400884] em28xx: DVB interface 0 found
[ 1093.405063] em28xx #0: chip ID is em2874
[ 1093.506333] em28xx #0: found i2c device @ 0xa0 [eeprom]
[ 1093.523324] em28xx #0: Your board has no unique USB ID.
[ 1093.528535] em28xx #0: A hint were successfully done, based on i2c devicelist hash.
[ 1093.536168] em28xx #0: This method is not 100% failproof.
[ 1093.541552] em28xx #0: If the board were missdetected, please email this log to:
[ 1093.548919] em28xx #0: 	V4L Mailing List  <linux-media@vger.kernel.org>
[ 1093.555508] em28xx #0: Board detected as EM2874 Leadership ISDBT
[ 1093.645024] em28xx #0: Identified as EM2874 Leadership ISDBT (card=77)
[ 1093.651536] em28xx #0: v4l2 driver version 0.1.3
[ 1093.660990] em28xx #0: V4L2 video device registered as video0
[ 1093.667177] em28xx: init = 1, userspace_is_disabled = 0, needs firmware = 1

The above logs are due to the new device that doesn't require any firmware:
As already said, em28xx firmware is written inside the chip's ROM. On this
device, both ISDB-T tuning and demod are provided by s921 frontend, and this
frontend doesn't require firmware. So, probing here is never deferred.

After finishing the non-deferred probe(), the deferred one happens:

[ 1093.674162] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, interface 0, class 0)
[ 1093.682398] em28xx: Audio Vendor Class interface 0 found
[ 1093.687693] em28xx: Video interface 0 found
[ 1093.691890] em28xx: DVB interface 0 found
[ 1093.696103] em28xx #1: chip ID is em2882/em2883
[ 1093.713968] s921: s921_attach: 
[ 1093.713972] DVB: registering new adapter (em28xx #0)
[ 1093.718934] dvb_register_frontend
[ 1093.722251] DVB: registering adapter 0 frontend 0 (Sharp S921)...
[ 1093.728421] dvb_frontend_clear_cache() Clearing cache for delivery system 8
[ 1093.736805] em28xx #0: Successfully loaded em28xx-dvb
[ 1093.741846] Em28xx: Initialized (Em28xx dvb Extension) extension
[ 1093.843211] em28xx #1: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 82 1e 6a 18
[ 1093.851221] em28xx #1: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 00 00 00 00
[ 1093.859206] em28xx #1: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 5b 1c 00 00
[ 1093.867189] em28xx #1: i2c eeprom 30: 00 00 20 40 20 80 02 20 01 01 01 01 00 00 00 00
[ 1093.875178] em28xx #1: i2c eeprom 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1093.883158] em28xx #1: i2c eeprom 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1093.891200] em28xx #1: i2c eeprom 60: 00 00 00 00 00 00 00 00 00 00 18 03 34 00 30 00
[ 1093.899192] em28xx #1: i2c eeprom 70: 32 00 38 00 34 00 34 00 39 00 30 00 31 00 38 00
[ 1093.907179] em28xx #1: i2c eeprom 80: 00 00 1e 03 57 00 69 00 6e 00 54 00 56 00 20 00
[ 1093.915165] em28xx #1: i2c eeprom 90: 48 00 56 00 52 00 2d 00 39 00 38 00 30 00 00 00
[ 1093.923150] em28xx #1: i2c eeprom a0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[ 1093.931133] em28xx #1: i2c eeprom b0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[ 1093.939132] em28xx #1: i2c eeprom c0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[ 1093.947128] em28xx #1: i2c eeprom d0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe d0 18 85
[ 1093.955118] em28xx #1: i2c eeprom e0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 40 fa 40
[ 1093.963122] em28xx #1: i2c eeprom f0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 00 00 00
[ 1093.971113] em28xx #1: EEPROM ID= 0x9567eb1a, EEPROM hash = 0x994b2bdd
[ 1093.977617] em28xx #1: EEPROM info:
[ 1093.981091] em28xx #1:	AC97 audio (5 sample rates)
[ 1093.985865] em28xx #1:	500mA max power
[ 1093.989599] em28xx #1:	Table at 0x24, strings=0x1e82, 0x186a, 0x0000
[ 1093.995929] em28xx #1: Identified as Hauppauge WinTV HVR 950 (card=16)
[ 1094.003609] tveeprom 4-0050: Hauppauge model 65201, rev A1C0, serial# 1917178
[ 1094.010755] tveeprom 4-0050: tuner model is Xceive XC3028 (idx 120, type 71)
[ 1094.017809] tveeprom 4-0050: TV standards PAL(B/G) PAL(I) PAL(D/D1/K) ATSC/DVB Digital (eeprom 0xd4)
[ 1094.026962] tveeprom 4-0050: audio processor is None (idx 0)
[ 1094.032654] tveeprom 4-0050: has radio
[ 1094.038164] tvp5150 4-005c: chip found @ 0xb8 (em28xx #1)
[ 1094.091327] tvp5150 4-005c: tvp5150am1 detected.
[ 1094.114072] tuner 4-0061: Tuner -1 found with type(s) Radio TV.
[ 1094.120031] xc2028: Xcv2028/3028 init called!
[ 1094.120038] xc2028 4-0061: creating new instance
[ 1094.124644] xc2028 4-0061: type set to XCeive xc2028/xc3028 tuner
[ 1094.130716] xc2028 4-0061: xc2028_set_config called
[ 1094.130734] xc2028 4-0061: xc2028_set_analog_freq called
[ 1094.130736] xc2028 4-0061: generic_set_freq called
[ 1094.130739] xc2028 4-0061: should set frequency 567250 kHz
[ 1094.130741] xc2028 4-0061: check_firmware called
[ 1094.130743] xc2028 4-0061: load_all_firmwares called
[ 1094.130746] xc2028 4-0061: Reading firmware xc3028-v27.fw
[ 1094.131248] xc2028 4-0061: Loading 80 firmware images from xc3028-v27.fw, type: xc2028 firmware, ver 2.7
[ 1094.140707] xc2028 4-0061: Reading firmware type BASE F8MHZ (3), id 0, size=8718.
[ 1094.140714] xc2028 4-0061: Reading firmware type BASE F8MHZ MTS (7), id 0, size=8712.
[ 1094.140719] xc2028 4-0061: Reading firmware type BASE FM (401), id 0, size=8562.
[ 1094.140725] xc2028 4-0061: Reading firmware type BASE FM INPUT1 (c01), id 0, size=8576.
[ 1094.140731] xc2028 4-0061: Reading firmware type BASE (1), id 0, size=8706.
[ 1094.140735] xc2028 4-0061: Reading firmware type BASE MTS (5), id 0, size=8682.
[ 1094.140739] xc2028 4-0061: Reading firmware type (0), id 100000007, size=161.
[ 1094.140741] xc2028 4-0061: Reading firmware type MTS (4), id 100000007, size=169.
[ 1094.140744] xc2028 4-0061: Reading firmware type (0), id 200000007, size=161.
[ 1094.140746] xc2028 4-0061: Reading firmware type MTS (4), id 200000007, size=169.
[ 1094.140748] xc2028 4-0061: Reading firmware type (0), id 400000007, size=161.
[ 1094.140750] xc2028 4-0061: Reading firmware type MTS (4), id 400000007, size=169.
[ 1094.140752] xc2028 4-0061: Reading firmware type (0), id 800000007, size=161.
[ 1094.140755] xc2028 4-0061: Reading firmware type MTS (4), id 800000007, size=169.
[ 1094.140757] xc2028 4-0061: Reading firmware type (0), id 3000000e0, size=161.
[ 1094.140759] xc2028 4-0061: Reading firmware type MTS (4), id 3000000e0, size=169.
[ 1094.140761] xc2028 4-0061: Reading firmware type (0), id c000000e0, size=161.
[ 1094.140764] xc2028 4-0061: Reading firmware type MTS (4), id c000000e0, size=169.
[ 1094.140766] xc2028 4-0061: Reading firmware type (0), id 200000, size=161.
[ 1094.140768] xc2028 4-0061: Reading firmware type MTS (4), id 200000, size=169.
[ 1094.140770] xc2028 4-0061: Reading firmware type (0), id 4000000, size=161.
[ 1094.140772] xc2028 4-0061: Reading firmware type MTS (4), id 4000000, size=169.
[ 1094.140775] xc2028 4-0061: Reading firmware type D2633 DTV6 ATSC (10030), id 0, size=149.
[ 1094.140778] xc2028 4-0061: Reading firmware type D2620 DTV6 QAM (68), id 0, size=149.
[ 1094.140781] xc2028 4-0061: Reading firmware type D2633 DTV6 QAM (70), id 0, size=149.
[ 1094.140784] xc2028 4-0061: Reading firmware type D2620 DTV7 (88), id 0, size=149.
[ 1094.140787] xc2028 4-0061: Reading firmware type D2633 DTV7 (90), id 0, size=149.
[ 1094.140790] xc2028 4-0061: Reading firmware type D2620 DTV78 (108), id 0, size=149.
[ 1094.140792] xc2028 4-0061: Reading firmware type D2633 DTV78 (110), id 0, size=149.
[ 1094.140795] xc2028 4-0061: Reading firmware type D2620 DTV8 (208), id 0, size=149.
[ 1094.140798] xc2028 4-0061: Reading firmware type D2633 DTV8 (210), id 0, size=149.
[ 1094.140800] xc2028 4-0061: Reading firmware type FM (400), id 0, size=135.
[ 1094.140803] xc2028 4-0061: Reading firmware type (0), id 10, size=161.
[ 1094.140805] xc2028 4-0061: Reading firmware type MTS (4), id 10, size=169.
[ 1094.140807] xc2028 4-0061: Reading firmware type (0), id 1000400000, size=169.
[ 1094.140809] xc2028 4-0061: Reading firmware type (0), id c00400000, size=161.
[ 1094.140812] xc2028 4-0061: Reading firmware type (0), id 800000, size=161.
[ 1094.140814] xc2028 4-0061: Reading firmware type (0), id 8000, size=161.
[ 1094.140816] xc2028 4-0061: Reading firmware type LCD (1000), id 8000, size=161.
[ 1094.140818] xc2028 4-0061: Reading firmware type LCD NOGD (3000), id 8000, size=161.
[ 1094.140821] xc2028 4-0061: Reading firmware type MTS (4), id 8000, size=169.
[ 1094.140823] xc2028 4-0061: Reading firmware type (0), id b700, size=161.
[ 1094.140825] xc2028 4-0061: Reading firmware type LCD (1000), id b700, size=161.
[ 1094.140827] xc2028 4-0061: Reading firmware type LCD NOGD (3000), id b700, size=161.
[ 1094.140830] xc2028 4-0061: Reading firmware type (0), id 2000, size=161.
[ 1094.140832] xc2028 4-0061: Reading firmware type MTS (4), id b700, size=169.
[ 1094.140834] xc2028 4-0061: Reading firmware type MTS LCD (1004), id b700, size=169.
[ 1094.140837] xc2028 4-0061: Reading firmware type MTS LCD NOGD (3004), id b700, size=169.
[ 1094.140840] xc2028 4-0061: Reading firmware type SCODE HAS_IF_3280 (60000000), id 0, size=192.
[ 1094.140843] xc2028 4-0061: Reading firmware type SCODE HAS_IF_3300 (60000000), id 0, size=192.
[ 1094.140846] xc2028 4-0061: Reading firmware type SCODE HAS_IF_3440 (60000000), id 0, size=192.
[ 1094.140849] xc2028 4-0061: Reading firmware type SCODE HAS_IF_3460 (60000000), id 0, size=192.
[ 1094.140852] xc2028 4-0061: Reading firmware type DTV6 ATSC OREN36 SCODE HAS_IF_3800 (60210020), id 0, size=192.
[ 1094.140856] xc2028 4-0061: Reading firmware type SCODE HAS_IF_4000 (60000000), id 0, size=192.
[ 1094.140859] xc2028 4-0061: Reading firmware type DTV6 ATSC TOYOTA388 SCODE HAS_IF_4080 (60410020), id 0, size=192.
[ 1094.140862] xc2028 4-0061: Reading firmware type SCODE HAS_IF_4200 (60000000), id 0, size=192.
[ 1094.140865] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_4320 (60008000), id 8000, size=192.
[ 1094.140869] xc2028 4-0061: Reading firmware type SCODE HAS_IF_4450 (60000000), id 0, size=192.
[ 1094.140872] xc2028 4-0061: Reading firmware type MTS LCD NOGD MONO IF SCODE HAS_IF_4500 (6002b004), id b700, size=192.
[ 1094.140876] xc2028 4-0061: Reading firmware type LCD NOGD IF SCODE HAS_IF_4600 (60023000), id 8000, size=192.
[ 1094.140880] xc2028 4-0061: Reading firmware type DTV6 QAM DTV7 DTV78 DTV8 ZARLINK456 SCODE HAS_IF_4760 (620003e0), id 0, size=192.
[ 1094.140884] xc2028 4-0061: Reading firmware type SCODE HAS_IF_4940 (60000000), id 0, size=192.
[ 1094.140887] xc2028 4-0061: Reading firmware type SCODE HAS_IF_5260 (60000000), id 0, size=192.
[ 1094.140890] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_5320 (60008000), id f00000007, size=192.
[ 1094.140893] xc2028 4-0061: Reading firmware type DTV7 DTV78 DTV8 DIBCOM52 CHINA SCODE HAS_IF_5400 (65000380), id 0, size=192.
[ 1094.140897] xc2028 4-0061: Reading firmware type DTV6 ATSC OREN538 SCODE HAS_IF_5580 (60110020), id 0, size=192.
[ 1094.140901] xc2028 4-0061: Reading firmware type SCODE HAS_IF_5640 (60000000), id 300000007, size=192.
[ 1094.140904] xc2028 4-0061: Reading firmware type SCODE HAS_IF_5740 (60000000), id c00000007, size=192.
[ 1094.140907] xc2028 4-0061: Reading firmware type SCODE HAS_IF_5900 (60000000), id 0, size=192.
[ 1094.140910] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_6000 (60008000), id c04c000f0, size=192.
[ 1094.140913] xc2028 4-0061: Reading firmware type DTV6 QAM ATSC LG60 F6MHZ SCODE HAS_IF_6200 (68050060), id 0, size=192.
[ 1094.140917] xc2028 4-0061: Reading firmware type SCODE HAS_IF_6240 (60000000), id 10, size=192.
[ 1094.140920] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_6320 (60008000), id 200000, size=192.
[ 1094.140923] xc2028 4-0061: Reading firmware type SCODE HAS_IF_6340 (60000000), id 200000, size=192.
[ 1094.140926] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_6500 (60008000), id c044000e0, size=192.
[ 1094.140929] xc2028 4-0061: Reading firmware type DTV6 ATSC ATI638 SCODE HAS_IF_6580 (60090020), id 0, size=192.
[ 1094.140933] xc2028 4-0061: Reading firmware type SCODE HAS_IF_6600 (60000000), id 3000000e0, size=192.
[ 1094.140936] xc2028 4-0061: Reading firmware type MONO SCODE HAS_IF_6680 (60008000), id 3000000e0, size=192.
[ 1094.140939] xc2028 4-0061: Reading firmware type DTV6 ATSC TOYOTA794 SCODE HAS_IF_8140 (60810020), id 0, size=192.
[ 1094.140942] xc2028 4-0061: Reading firmware type SCODE HAS_IF_8200 (60000000), id 0, size=192.
[ 1094.140950] xc2028 4-0061: Firmware files loaded.
[ 1094.140952] xc2028 4-0061: checking firmware, user requested type=MTS (4), id 000000000000b700, scode_tbl (0), scode_nr 0
[ 1094.173746] xc2028 4-0061: load_firmware called
[ 1094.173749] xc2028 4-0061: seek_firmware called, want type=BASE MTS (5), id 0000000000000000.
[ 1094.173755] xc2028 4-0061: Found firmware for type=BASE MTS (5), id 0000000000000000.
[ 1094.173759] xc2028 4-0061: Loading firmware for type=BASE MTS (5), id 0000000000000000.
[ 1095.319660] xc2028 4-0061: Load init1 firmware, if exists
[ 1095.319664] xc2028 4-0061: load_firmware called
[ 1095.319666] xc2028 4-0061: seek_firmware called, want type=BASE INIT1 MTS (4005), id 0000000000000000.
[ 1095.319672] xc2028 4-0061: Can't find firmware for type=BASE INIT1 MTS (4005), id 0000000000000000.
[ 1095.319677] xc2028 4-0061: load_firmware called
[ 1095.319680] xc2028 4-0061: seek_firmware called, want type=BASE INIT1 MTS (4005), id 0000000000000000.
[ 1095.319685] xc2028 4-0061: Can't find firmware for type=BASE INIT1 MTS (4005), id 0000000000000000.
[ 1095.319690] xc2028 4-0061: load_firmware called
[ 1095.319692] xc2028 4-0061: seek_firmware called, want type=MTS (4), id 000000000000b700.
[ 1095.319696] xc2028 4-0061: Found firmware for type=MTS (4), id 000000000000b700.
[ 1095.319700] xc2028 4-0061: Loading firmware for type=MTS (4), id 000000000000b700.
[ 1095.345750] xc2028 4-0061: Trying to load scode 0
[ 1095.345754] xc2028 4-0061: load_scode called
[ 1095.345756] xc2028 4-0061: seek_firmware called, want type=MTS SCODE (20000004), id 000000000000b700.
[ 1095.345762] xc2028 4-0061: Found firmware for type=MTS SCODE (20000004), id 000000000000b700.
[ 1095.345766] xc2028 4-0061: Loading SCODE for type=MTS LCD NOGD MONO IF SCODE HAS_IF_4500 (6002b004), id 000000000000b700.
[ 1095.359368] xc2028 4-0061: xc2028_get_reg 0004 called
[ 1095.360387] xc2028 4-0061: xc2028_get_reg 0008 called
[ 1095.361389] xc2028 4-0061: Device is Xceive 3028 version 1.0, firmware version 2.7

The last log shows that the firmware load went fine and that the device got successuly bind/recognized,
as the get_firmware_version firmware call returned the expected firmware version (2.7).

So, if we go to the deferred probe(), we'll need to add a way to queue the deferred probe after
the end of the driver's load.

/me is still trying to figure out an approach that would work.

Regards,
Mauro

[PATCH RFC 4/4] tuner-xc2028: tag the usual firmwares to help dracut

[Mauro Carvalho Chehab]

When tuner-xc2028 is not compiled as a module, dracut will
need to copy the firmware inside the initfs image.

So, use MODULE_FIRMWARE() to indicate such need.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/common/tuners/tuner-xc2028.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c
index b5ee3eb..2e6c966 100644
--- a/drivers/media/common/tuners/tuner-xc2028.c
+++ b/drivers/media/common/tuners/tuner-xc2028.c
@@ -1375,3 +1375,5 @@ MODULE_DESCRIPTION("Xceive xc2028/xc3028 tuner driver");
 MODULE_AUTHOR("Michel Ludwig <michel.ludwig@gmail.com>");
 MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@infradead.org>");
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE(XC2028_DEFAULT_FIRMWARE);
+MODULE_FIRMWARE(XC3028L_DEFAULT_FIRMWARE);
-- 
1.7.10.2

Re: [PATCH RFC 4/4] tuner-xc2028: tag the usual firmwares to help dracut

[Greg KH]

On Tue, Jun 26, 2012 at 04:34:22PM -0300, Mauro Carvalho Chehab wrote:
> When tuner-xc2028 is not compiled as a module, dracut will
> need to copy the firmware inside the initfs image.
> 
> So, use MODULE_FIRMWARE() to indicate such need.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  drivers/media/common/tuners/tuner-xc2028.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c
> index b5ee3eb..2e6c966 100644
> --- a/drivers/media/common/tuners/tuner-xc2028.c
> +++ b/drivers/media/common/tuners/tuner-xc2028.c
> @@ -1375,3 +1375,5 @@ MODULE_DESCRIPTION("Xceive xc2028/xc3028 tuner driver");
>  MODULE_AUTHOR("Michel Ludwig <michel.ludwig@gmail.com>");
>  MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@infradead.org>");
>  MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE(XC2028_DEFAULT_FIRMWARE);
> +MODULE_FIRMWARE(XC3028L_DEFAULT_FIRMWARE);

This is proabably something that needs to get in now, independant of the
other 3 patches, right?

greg k-h

Re: [PATCH RFC 4/4] tuner-xc2028: tag the usual firmwares to help dracut

[Mauro Carvalho Chehab]

Em 26-06-2012 17:44, Greg KH escreveu:
> On Tue, Jun 26, 2012 at 04:34:22PM -0300, Mauro Carvalho Chehab wrote:
>> When tuner-xc2028 is not compiled as a module, dracut will
>> need to copy the firmware inside the initfs image.
>>
>> So, use MODULE_FIRMWARE() to indicate such need.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>   drivers/media/common/tuners/tuner-xc2028.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c
>> index b5ee3eb..2e6c966 100644
>> --- a/drivers/media/common/tuners/tuner-xc2028.c
>> +++ b/drivers/media/common/tuners/tuner-xc2028.c
>> @@ -1375,3 +1375,5 @@ MODULE_DESCRIPTION("Xceive xc2028/xc3028 tuner driver");
>>   MODULE_AUTHOR("Michel Ludwig <michel.ludwig@gmail.com>");
>>   MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@infradead.org>");
>>   MODULE_LICENSE("GPL");
>> +MODULE_FIRMWARE(XC2028_DEFAULT_FIRMWARE);
>> +MODULE_FIRMWARE(XC3028L_DEFAULT_FIRMWARE);
> 
> This is proabably something that needs to get in now, independant of the
> other 3 patches, right?

Yes, this one is independent. 

This were not added before as the firmware names are/used to be device-specific,
and there's a modprobe parameter that allows overriding it.

Currently, all xc3028 devices since a long time uses version 2.7, and all
XC3028L uses version 3.2. (I still have here a device that only works with
a custom v 1.f firmware).

Anyway, it makes sense to at least use the two most common possible firmwares
there.

Regards,
Mauro

udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Hi Greg,

Em Mon, 25 Jun 2012 22:55:41 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> Em 25-06-2012 19:33, Greg KH escreveu:
> > On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote:
> >> Greg,
> >>
> >> Basically, the recent changes at request_firmware() exposed an issue that
> >> affects all media drivers that use firmware (64 drivers).
> > 
> > What change was that?  How did it break anything?
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=827538
> 
> Basically, userspace changes and some pm-related patches, ...
...

As I pinged you back in June, the recent changes on udev made lots of regression
on all media drivers that require firmware. The news is that the fixes are also
causing more regressions...

Btw, Linus also complained about that:
	https://lkml.org/lkml/2012/8/25/136

I basically tried a few different approaches, including deferred probe(),
as you suggested, and request_firmware_async(), as Kay suggested.

The deferred probe() doesn't work as-is, as it will re-probe the driver 
only if a new driver is modprobed. It should likely not be that hard to modify
the drivers core to allow it to re-probe after returning the probe status to 
udev, but I failed to find a way of doing that (mostly because of the lack of 
time, as I'm being too busy those days).

I tried a few other things, like using request_firmware_async() at a few
drivers that require firmware.

That caused a big regression on Kernel 3.6 that we're currently discussing 
how to fix, but basically, media devices are very complex, as they have lots
of different blocks there: tuners, analog video demod, digital video demod, 
audio demod, etc. On most cases, each of those "extra" blocks are implemented
via I2C, and the device initialization happens serialized.

By letting an I2C driver to do asynchronous initialization, other dependent
drivers broke, because there are some signals used by the other blocks that 
are dependent on a proper initialization of a block previously initialized.

One of the specific case is a device very popular in Europe nowadays: the
PCTV 520e. This device uses an em28xx USB bridge chip, a tda18271 i2c tuner
and a drx-k DVB i2c demod (among other things). The DRX-K chipset requires a
firmware; the other chipsets there don't.

The drx-k driver is used in combination with several other drivers, like
em28xx, az6027, xc5000, tda18271, tda18271dd, etc. On my tests, with
the devices I have available (Terratec H5, H6, H7, HTC; Hauppauge HVR 530C),
the usage of request_firmware_async() worked as expected.

So, such patch that made DRX-K firmware load to be deferred were applied at
Kernel 3.6 on this changeset:

commit  bd02dbcd008f92135b2c7a92b6
Author: Mauro Carvalho Chehab <mchehab@redhat.com>
Date:   Thu Jun 21 09:36:38 2012 -0300

    [media] drxk: change it to use request_firmware_nowait()
    
    The firmware blob may not be available when the driver probes.
    
    Instead of blocking the whole kernel use request_firmware_nowait() and
    continue without firmware.
    
    This shouldn't be that bad on drx-k devices, as they all seem to have an
    internal firmware. So, only the firmware update will take a little longer
    to happen.
    
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

It should be noticed that, for the DVB demodulation to work, the DVB demod 
should be attached with the tuner module, via this code snippet[1]:

		fe = drxk_attach(&pctv_520e_drxk_cfg, &i2c_adap);
		tda18271_attach(fe, i2c_tuner_address, &i2c_adap, &pctv_520e_tda18271_cfg);

The evil are in the details: the DRX-K chip produces some clocks needed by
the tuner, and it has some generic GPIO pins that allow to control random 
things, with are device-dependent. Also, there is an I2C switch at the board,
used to control I2C access to the tuner: when the switch is off, the tuner is
not visible at the I2C bus[2].

The tda18271 tuner driver needs to read one device register, in order to 
identify the chipset version, as there are a few different setups there,
depending if the silicon is version 1 or 2. As this driver doesn't require
any firmware, such read happens at driver's attach time, with should be fine.

Well, before the patch:

	1) drxk_attach() would synchronously read the firmware and initialize the
	   device, setting the needed GPIO pins and clock signals;

	2) tda18271_attach() would read the version ID register and initialize the
	   device.

However, after the patch, drxk_attach() does nothing but filling some internal data
structures and defer the device init job to happen after the firmware load.

So, when tda18271_attach() is called, there wasn't enough time yet to initialize
the DRX-K device. The end result is that the tda18271 version is not identified
and the I2C driver read fails:

	tda18271_read_regs: [5-0060|M] ERROR: i2c_transfer returned: -19
	Unknown device (16) detected @ 5-0060, device not supported.

Ok, there are lots of ways to fix it, but I suspect that we'll just push the
problem to happen on some place else. 

The fact is that coordinating the initialization between the several parts of 
those devices is already complex enough when done serialized; doing it
asynchronously will make the initialization code complex and won't bring any
benefit, as the I2C bus will serialize the initialization anyway.

Also, this is just one of the 495 media drivers. Several of them require firmware
during probe() and are currently broken. Fixing all of them will likely require
years of hard work. So, we need to do something else.

For Kernel 3.6, we'll likely apply some quick hack.

However, for 3.7 or 3.8, I think that the better is to revert changeset 177bc7dade38b5
and to stop with udev's insanity of requiring asynchronous firmware load during
device driver initialization. If udev's developers are not willing to do that,
we'll likely need to add something at the drivers core to trick udev for it to
think that the modules got probed before the probe actually happens.

What do you think?

[1] This is a simplified version of the code: I removed a macro and the error
    logic from it, to make the code cleaner for reading.

[2] The I2C switch is there to prevent I2C traffic to generate noise that might
    interfere at the tuner functions.

Thanks,
Mauro

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Tue, Oct 2, 2012 at 6:03 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
>
> I basically tried a few different approaches, including deferred probe(),
> as you suggested, and request_firmware_async(), as Kay suggested.

Stop this crazy. FIX UDEV ALREADY, DAMMIT.

Who maintains udev these days? Is it Lennart/Kai, as part of systemd?

Lennart/Kai, fix the udev regression already. Lennart was the one who
brought up kernel ABI regressions at some conference, and if you now
you have the *gall* to break udev in an incompatible manner that
requires basically impossible kernel changes for the kernel to "fix"
the udev interface, I don't know what to say.

"Two-faced lying weasel" would be the most polite thing I could say.
But it almost certainly will involve a lot of cursing.

> However, for 3.7 or 3.8, I think that the better is to revert changeset 177bc7dade38b5
> and to stop with udev's insanity of requiring asynchronous firmware load during
> device driver initialization. If udev's developers are not willing to do that,
> we'll likely need to add something at the drivers core to trick udev for it to
> think that the modules got probed before the probe actually happens.

The fact is, udev made new - and insane - rules that are simply
*invalid*. Modern udev is broken, and needs to be fixed.

I don't know where the problem started in udev, but the report I saw
was that udev175 was fine, and udev182 was broken, and would deadlock
if module_init() did a request_firmware(). That kind of nested
behavior is absolutely *required* to work, in order to not cause
idiotic problems for the kernel for no good reason.

What kind of insane udev maintainership do we have? And can we fix it?

Greg, I think you need to step up here too. You were the one who let
udev go. If the new maintainers are causing problems, they need to be
fixed some way.

                Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Ivan Kalvachev]

On 10/2/12, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Tue, Oct 2, 2012 at 6:03 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>>
>> I basically tried a few different approaches, including deferred probe(),
>> as you suggested, and request_firmware_async(), as Kay suggested.
>
> Stop this crazy. FIX UDEV ALREADY, DAMMIT.
>
> Who maintains udev these days? Is it Lennart/Kai, as part of systemd?
>
> Lennart/Kai, fix the udev regression already. Lennart was the one who
> brought up kernel ABI regressions at some conference, and if you now
> you have the *gall* to break udev in an incompatible manner that
> requires basically impossible kernel changes for the kernel to "fix"
> the udev interface, I don't know what to say.
>
> "Two-faced lying weasel" would be the most polite thing I could say.
> But it almost certainly will involve a lot of cursing.
>
>> However, for 3.7 or 3.8, I think that the better is to revert changeset
>> 177bc7dade38b5
>> and to stop with udev's insanity of requiring asynchronous firmware load
>> during
>> device driver initialization. If udev's developers are not willing to do
>> that,
>> we'll likely need to add something at the drivers core to trick udev for
>> it to
>> think that the modules got probed before the probe actually happens.
>
> The fact is, udev made new - and insane - rules that are simply
> *invalid*. Modern udev is broken, and needs to be fixed.
>
> I don't know where the problem started in udev, but the report I saw
> was that udev175 was fine, and udev182 was broken, and would deadlock
> if module_init() did a request_firmware(). That kind of nested
> behavior is absolutely *required* to work, in order to not cause
> idiotic problems for the kernel for no good reason.
>
> What kind of insane udev maintainership do we have? And can we fix it?
>
> Greg, I think you need to step up here too. You were the one who let
> udev go. If the new maintainers are causing problems, they need to be
> fixed some way.

I'm not kernel developer and probably my opinion would be a little
naive, but here it is.


Please, make the kernel load firmware from the filesystem on its own.


This should solve almost 99.9% of the problems related to firmware loading.

I don't mind if there is still userland component that could be used
to request a firmware from repository. You can even keep the udev
userland piping as a fallback if you want, but I think you can
simplify a lot of code if you phase it out. The firmware loading
should follow the same concept as modules loading.

I've heard that the udev userland piping of firmware is done to avoid
some licensing issues. But honestly, if you can not store the firmware
on the user's disk, no free operating system should support it at all.

This piping thing have been feature creep that have metastasized all
over the kernel while keeping a lot of obscure modules broken (in hard
to find way) and requiring increasingly complicated schemes to
workaround its flaws.

KiSS.

Best Regards
   Ivan Kalvachev

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Tue, Oct 2, 2012 at 2:03 PM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>
> I'm not kernel developer and probably my opinion would be a little
> naive, but here it is.
>
> Please, make the kernel load firmware from the filesystem on its own.

We probably should do that, not just for firmware, but for modules
too. It would also simplify the whole "built-in firmware" thing

Afaik, the only thing udev really does is to lok in
/lib/firmware/updates and /lib/firmware for the file, load it, and
pass it back to the kernel. We could make the kernel try to do it
manually first, and only fall back to udev if that fails.

Afaik, nobody ever does anything else anyway.

I'd prefer to not have to do that, but if the udev code is buggy or
the maintainership is flaky, I guess we don't really have much choice.

Doing the same thing for module loading is probably a good idea too.
There were already people (from the google/Android camp) who wanted to
do module loading based on filename rather than the buffer passed to
it, because they have a "I trust this filesystem" model.

> I've heard that the udev userland piping of firmware is done to avoid
> some licensing issues.

No, I think it was mainly a combination of

 - some people like the whole "let's do things in user land" model
even when it makes things more complicated

 - we do tend to try to punt "policy" issues to user space, and the
whole "/lib/firmware" location is an example of such a policy issue.

along with the fact that we already had the hotplug model for these
kinds of things (eg module loading used to actually have a big user
space component that did the whole relocation etc, so we had real
historical reasons to do that in user space)

Does anybody want to try to cook up a patch, leaving the udev path as
a fallback?

We already have the case of "builtin firmware" as one option, this
would go after that..

                 Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Lucas De Marchi]

On Tue, Oct 2, 2012 at 7:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Oct 2, 2012 at 2:03 PM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>>
>> I'm not kernel developer and probably my opinion would be a little
>> naive, but here it is.
>>
>> Please, make the kernel load firmware from the filesystem on its own.
>
> We probably should do that, not just for firmware, but for modules
> too. It would also simplify the whole "built-in firmware" thing
>
> Afaik, the only thing udev really does is to lok in
> /lib/firmware/updates and /lib/firmware for the file, load it, and
> pass it back to the kernel. We could make the kernel try to do it
> manually first, and only fall back to udev if that fails.
>
> Afaik, nobody ever does anything else anyway.
>
> I'd prefer to not have to do that, but if the udev code is buggy or
> the maintainership is flaky, I guess we don't really have much choice.
>
> Doing the same thing for module loading is probably a good idea too.

humn... I don't think so. It would work perfectly well for firmware,
but for modules there are more things involved like fulfilling
dependencies, soft-dependencies, aliases and the like. It would create
several regressions.

> There were already people (from the google/Android camp) who wanted to
> do module loading based on filename rather than the buffer passed to
> it, because they have a "I trust this filesystem" model.

They wanted to pass a fd instead of a buffer. That is being done in
the new finit_module syscall being discussed:
http://www.gossamer-threads.com/lists/linux/kernel/1592271?do=post_view_flat


Lucas De Marchi

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Shea Levy]

On 10/02/2012 06:37 PM, Linus Torvalds wrote:
> On Tue, Oct 2, 2012 at 2:03 PM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>> I'm not kernel developer and probably my opinion would be a little
>> naive, but here it is.
>>
>> Please, make the kernel load firmware from the filesystem on its own.
> We probably should do that, not just for firmware, but for modules
> too. It would also simplify the whole "built-in firmware" thing
>
> Afaik, the only thing udev really does is to lok in
> /lib/firmware/updates and /lib/firmware for the file, load it, and
> pass it back to the kernel. We could make the kernel try to do it
> manually first, and only fall back to udev if that fails.
>
> Afaik, nobody ever does anything else anyway.
>

FWIW (and probably that's not much), the NixOS[0] distro doesn't 
currently use /lib/firmware. There is no /lib directory by default on 
NixOS, instead we create a new symlink tree representing the current 
system on each system change and symlink /run/current-system to that 
tree. We currently build udev/systemd with the 
--with-firmware-path=/run/current-system/firmware configuration-time 
option, but we also patch module-init-tools and kmod to respect the 
$MODULE_DIR env var and may do the same for firmware in the future. The 
way we do things has significant advantages (or at least we like to 
think so), but we already have exceptions for /bin/sh and /usr/bin/env, 
so I suspect we'll probably add in /lib/firmware if this functionality 
moves into the kernel.

[0]: https://nixos.org/

Cheers,
Shea Levy

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Ming Lei]

On Fri, Oct 12, 2012 at 2:33 AM, Shea Levy <shea@shealevy.com> wrote:

>
> FWIW (and probably that's not much), the NixOS[0] distro doesn't currently
> use /lib/firmware. There is no /lib directory by default on NixOS, instead
> we create a new symlink tree representing the current system on each system
> change and symlink /run/current-system to that tree. We currently build
> udev/systemd with the --with-firmware-path=/run/current-system/firmware
> configuration-time option, but we also patch module-init-tools and kmod to
> respect the $MODULE_DIR env var and may do the same for firmware in the
> future. The way we do things has significant advantages (or at least we like
> to think so), but we already have exceptions for /bin/sh and /usr/bin/env,
> so I suspect we'll probably add in /lib/firmware if this functionality moves
> into the kernel.

The kernel parameter for customizing firmware search path will be added, so
you use can pass your search path from kernel command too.

Thanks,
-- 
Ming Lei

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Tue, Oct 02, 2012 at 09:33:03AM -0700, Linus Torvalds wrote:
> I don't know where the problem started in udev, but the report I saw
> was that udev175 was fine, and udev182 was broken, and would deadlock
> if module_init() did a request_firmware(). That kind of nested
> behavior is absolutely *required* to work, in order to not cause
> idiotic problems for the kernel for no good reason.
> 
> What kind of insane udev maintainership do we have? And can we fix it?
> 
> Greg, I think you need to step up here too. You were the one who let
> udev go. If the new maintainers are causing problems, they need to be
> fixed some way.

I've talked about this with Kay in the past (Plumbers conference I
think) and I thought he said it was all fixed in the latest version of
udev so there shouldn't be any problems anymore with this.

Mauro, what version of udev are you using that is still showing this
issue?

Kay, didn't you resolve this already?  If not, what was the reason why?

thanks,

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Tue, Oct 02, 2012 at 03:12:39PM -0700, Greg KH wrote:
> On Tue, Oct 02, 2012 at 09:33:03AM -0700, Linus Torvalds wrote:
> > I don't know where the problem started in udev, but the report I saw
> > was that udev175 was fine, and udev182 was broken, and would deadlock
> > if module_init() did a request_firmware(). That kind of nested
> > behavior is absolutely *required* to work, in order to not cause
> > idiotic problems for the kernel for no good reason.
> > 
> > What kind of insane udev maintainership do we have? And can we fix it?
> > 
> > Greg, I think you need to step up here too. You were the one who let
> > udev go. If the new maintainers are causing problems, they need to be
> > fixed some way.
> 
> I've talked about this with Kay in the past (Plumbers conference I
> think) and I thought he said it was all fixed in the latest version of
> udev so there shouldn't be any problems anymore with this.
> 
> Mauro, what version of udev are you using that is still showing this
> issue?
> 
> Kay, didn't you resolve this already?  If not, what was the reason why?

Hm, in digging through the udev tree, the only change I found was this
one:

commit 39177382a4f92a834b568d6ae5d750eb2a5a86f9
Author: Kay Sievers <kay@vrfy.org>
Date:   Thu Jul 19 12:32:24 2012 +0200

    udev: firmware - do not cancel requests in the initrd

diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
index 56dc8fc..de93d7b 100644
--- a/src/udev/udev-builtin-firmware.c
+++ b/src/udev/udev-builtin-firmware.c
@@ -129,7 +129,13 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
                                 err = -errno;
                 } while (err == -ENOENT);
                 rc = EXIT_FAILURE;
-                set_loading(udev, loadpath, "-1");
+                /*
+                 * Do not cancel the request in the initrd, the real root might have
+                 * the firmware file and the 'coldplug' run in the real root will find
+                 * this pending request and fulfill or cancel it.
+                 * */
+                if (!in_initrd())
+                        set_loading(udev, loadpath, "-1");
                 goto exit;
         }
 

which went into udev release 187 which I think corresponds to the place
when people started having problems, right Mauro?

If so, Mauro, is the solution just putting the firmware into the initrd?
No wait, it looks like this change was trying to fix the problem where
firmware files were not in the initrd, so it would stick around for the
real root to show up so that they could be loaded.  So this looks like
it was fixing firmware loading problems for people?

Kay, am I just looking at the totally wrong place here, and this file in
udev didn't have anything to do with the breakage?

thanks,

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Tue, Oct 2, 2012 at 3:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> which went into udev release 187 which I think corresponds to the place
> when people started having problems, right Mauro?

According to what I've seen, people started complaining in 182, not 187.

See for example

  http://patchwork.linuxtv.org/patch/13085/

which is a thread where you were involved too..

See also the arch linux thread:

  https://bbs.archlinux.org/viewtopic.php?id=134012&p=1

And see this email from Kay Sievers that shows that it was all known
about and intentional in the udev camp:

  http://www.spinics.net/lists/netdev/msg185742.html

There's a possible patch suggested here:

  http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html

but quite frankly, I am leery of the fact that the udev maintenance
seems to have gone into some "crazy mode" where they have made changes
that were known to be problematic, and are pure and utter stupidity.

Having the module init path load the firmware IS THE SENSIBLE AND
OBVIOUS THING TO DO for many cases. The fact that udev people have
apparently unilaterally decided that it's somehow wrong is scary.

               Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Jiri Kosina]

On Tue, 2 Oct 2012, Linus Torvalds wrote:

> And see this email from Kay Sievers that shows that it was all known
> about and intentional in the udev camp:
> 
>   http://www.spinics.net/lists/netdev/msg185742.html

This seems confusing indeed.

That e-mail referenced above is talking about loading firmware at ifup 
time. While that might work for network device drivers (I am not sure even 
about that), what are the udev maintainers advice for other drivers, where 
there is no analogy to ifup?

-- 
Jiri Kosina
SUSE Labs

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Tue, Oct 2, 2012 at 5:01 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 2 Oct 2012, Linus Torvalds wrote:
>
>> And see this email from Kay Sievers that shows that it was all known
>> about and intentional in the udev camp:
>>
>>   http://www.spinics.net/lists/netdev/msg185742.html
>
> This seems confusing indeed.
>
> That e-mail referenced above is talking about loading firmware at ifup
> time. While that might work for network device drivers (I am not sure even
> about that), what are the udev maintainers advice for other drivers, where
> there is no analogy to ifup?

Yeah, it's an udev bug. It really is that simple.

This is why I'm complaining. There's no way in hell we're fixing this
in kernel space, unless we call the "bypass udev entirely because the
maintainership quality of it has taken a nose dive". Yes, I've seen
some work-around patches, but quite frankly, I think it would be
absolutely insane for the kernel to work around the fact that udev is
buggy.

The fact is, doing request_firmware() from within module_init() is
simply the easiest approach for some devices.

Now, at the same time, I do agree that network devices should
generally try to delay it until ifup time, so I'm not arguing against
that part per se. I do think that when possible, people should aim to
delay firmware loading until as late as reasonable.

But as you point out, it's simply not always reasonable, and the media
people are clearly hitting the cases where it's just painful. Now,
those cases seem to be happily fairly *rare*, so this isn't getting a
ton of attention, but we should fix it.

Because the udev behavior is all pain, no gain. There's no *reason*
for udev to be pissy about this. And it didn't use to be.

                    Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Jiri Kosina]

On Tue, 2 Oct 2012, Linus Torvalds wrote:

> Now, at the same time, I do agree that network devices should generally 
> try to delay it until ifup time

Slightly tangential to the ongoing discussion, but still ... I think that 
even "all network drivers should delay firmware loading to ifup time" 
might be too general.

I would expect that there are network cards which require firmware to be 
present for PHY to work, right? On such cards, if you want to have link 
detection even on interfaces that are down (so that things like ifplugd 
can detect the link presence and configure the interface), ifup is too 
late.

I admit I haven't checked whether there actually are such cards out there, 
but it doesn't seem to be completely unrealistic to me.

-- 
Jiri Kosina
SUSE Labs

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 02-10-2012 19:47, Linus Torvalds escreveu:
> On Tue, Oct 2, 2012 at 3:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> which went into udev release 187 which I think corresponds to the place
>> when people started having problems, right Mauro?
> 
> According to what I've seen, people started complaining in 182, not 187.

Yes. The issue was noticed with media drivers when people started using the
drivers on Fedora 17, witch came with udev-182. There's an open
bugzilla there:
	https://bugzilla.redhat.com/show_bug.cgi?id=827538

> See for example
> 
>   http://patchwork.linuxtv.org/patch/13085/
> 
> which is a thread where you were involved too..
> 
> See also the arch linux thread:
> 
>   https://bbs.archlinux.org/viewtopic.php?id=134012&p=1
> 
> And see this email from Kay Sievers that shows that it was all known
> about and intentional in the udev camp:
> 
>   http://www.spinics.net/lists/netdev/msg185742.html
> 
> There's a possible patch suggested here:
> 
>   http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html
> 
> but quite frankly, I am leery of the fact that the udev maintenance
> seems to have gone into some "crazy mode" where they have made changes
> that were known to be problematic, and are pure and utter stupidity.
> 
> Having the module init path load the firmware IS THE SENSIBLE AND
> OBVIOUS THING TO DO for many cases. 

Yes, that is the case for most media devices. Some devices can only be
detected as a supported device after the firmware load, as we need the
firmware for the USB (or PCI) bridge to be there, in order to talk with
the media components under the board's internal I2C bus, as sometimes
the same USB/PCI ID is used by boards with different internal components.

> The fact that udev people have
> apparently unilaterally decided that it's somehow wrong is scary.
>

Thanks,
Mauro

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On Wed, Oct 3, 2012 at 8:13 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
>
> Yes. The issue was noticed with media drivers when people started using the
> drivers on Fedora 17, witch came with udev-182. There's an open
> bugzilla there:
>         https://bugzilla.redhat.com/show_bug.cgi?id=827538

Yeah, that bugzilla shows the problem with Kay as a maintainer too,
not willing to own up to problems he caused.

Can you actually see the problem? I did add the attached patch as an
attachment to the bugzilla, so the reporter there may be able to test
it, but it's been open for a long while..

Anyway. Attached is a really stupid patch that tries to do the "direct
firmware load" as suggested by Ivan. It has not been tested very
extensively at all (but I did test that it loaded the brcmsmac
firmware images on my laptop so it has the *potential* to work).

It has a few extra printk's sprinkled in (to show whether it does
anything or not), and it has a few other issues, but it might be worth
testing as a starting point.

We are apparently better off trying to avoid udev like the plague.
Doing something very similar to this for module loading is probably a
good idea too.

I'm adding Ming Lei to the participants too, because hooking into the
firmware loader like this means that the image doesn't get cached.
Which is sad. I'm hoping Ming Lei might be open to trying to fix that.

Hmm?

                   Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 2375 bytes --]

 drivers/base/firmware_class.c | 59 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6e210802c37b..2ffcb4030065 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -55,6 +55,54 @@ static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
 	return false;
 }
 
+static bool fw_read_file_contents(struct file *file, struct firmware *fw)
+{
+	loff_t size, pos;
+	struct inode *inode = file->f_dentry->d_inode;
+	char *buf;
+
+	if (!S_ISREG(inode->i_mode))
+		return false;
+	size = i_size_read(inode);
+	buf = vmalloc(size);
+	if (!buf)
+		return false;
+	pos = 0;
+	if (vfs_read(file, buf, size, &pos) != size) {
+		vfree(buf);
+		return false;
+	}
+	fw->data = buf;
+	fw->size = size;
+	return true;
+}
+
+static bool fw_get_filesystem_firmware(struct firmware *fw, const char *name)
+{
+	int i;
+	bool success = false;
+	const char *fw_path[] = { "/lib/firmware/update", "/firmware", "/lib/firmware" };
+	char *path = __getname();
+
+printk("Trying to load fw '%s' ", name);
+	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+		struct file *file;
+		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], name);
+
+		file = filp_open(path, O_RDONLY, 0);
+		if (IS_ERR(file))
+			continue;
+printk("from file '%s' ", path);
+		success = fw_read_file_contents(file, fw);
+		filp_close(file, NULL);
+		if (success)
+			break;
+	}
+printk(" %s.\n", success ? "Ok" : "failed");
+	__putname(path);
+	return success;
+}
+
 static bool fw_is_builtin_firmware(const struct firmware *fw)
 {
 	struct builtin_fw *b_fw;
@@ -346,7 +394,11 @@ static ssize_t firmware_loading_show(struct device *dev,
 /* firmware holds the ownership of pages */
 static void firmware_free_data(const struct firmware *fw)
 {
-	WARN_ON(!fw->priv);
+	/* Loaded directly? */
+	if (!fw->priv) {
+		vfree(fw->data);
+		return;
+	}
 	fw_free_buf(fw->priv);
 }
 
@@ -709,6 +761,11 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
 		return NULL;
 	}
 
+	if (fw_get_filesystem_firmware(firmware, name)) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n", name);
+		return NULL;
+	}
+
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
 	if (!ret)
 		fw_priv = fw_create_instance(firmware, name, device,

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 9:38 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Anyway. Attached is a really stupid patch that tries to do the "direct
> firmware load" as suggested by Ivan. It has not been tested very
> extensively at all (but I did test that it loaded the brcmsmac
> firmware images on my laptop so it has the *potential* to work).

Oh, and I stupidly put the new functions next to the builtin firmware
loading function, which means that the patch only works if you have
CONFIG_FW_LOADER enabled.

That's bogus, and the functions should be moved out of that #ifdef,
but I don't think it should hurt testing.

                    Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Al Viro]

On Wed, Oct 03, 2012 at 09:38:52AM -0700, Linus Torvalds wrote:
> Yeah, that bugzilla shows the problem with Kay as a maintainer too,
> not willing to own up to problems he caused.
> 
> Can you actually see the problem? I did add the attached patch as an
> attachment to the bugzilla, so the reporter there may be able to test
> it, but it's been open for a long while..
> 
> Anyway. Attached is a really stupid patch that tries to do the "direct
> firmware load" as suggested by Ivan. It has not been tested very
> extensively at all (but I did test that it loaded the brcmsmac
> firmware images on my laptop so it has the *potential* to work).

+	if (!S_ISREG(inode->i_mode))
+		return false;
+	size = i_size_read(inode);

Probably better to do vfs_getattr() and check mode and size in kstat; if
it's sufficiently hot for that to hurt, we are fucked anyway.

+		file = filp_open(path, O_RDONLY, 0);
+		if (IS_ERR(file))
+			continue;
+printk("from file '%s' ", path);
+		success = fw_read_file_contents(file, fw);
+		filp_close(file, NULL);

fput(file), please.  We have enough misuses of filp_close() as it is...

> We are apparently better off trying to avoid udev like the plague.
> Doing something very similar to this for module loading is probably a
> good idea too.

That, or just adding usr/udev in the kernel git tree and telling the
vertical integrators to go kiss a lamprey...

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> +       if (!S_ISREG(inode->i_mode))
> +               return false;
> +       size = i_size_read(inode);
>
> Probably better to do vfs_getattr() and check mode and size in kstat; if
> it's sufficiently hot for that to hurt, we are fucked anyway.
>
> +               file = filp_open(path, O_RDONLY, 0);
> +               if (IS_ERR(file))
> +                       continue;
> +printk("from file '%s' ", path);
> +               success = fw_read_file_contents(file, fw);
> +               filp_close(file, NULL);
>
> fput(file), please.  We have enough misuses of filp_close() as it is...

Ok, like this?

                      Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 2814 bytes --]

 drivers/base/firmware_class.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6e210802c37b..38feac4af991 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -21,6 +21,7 @@
 #include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/file.h>
 #include <linux/list.h>
 #include <linux/async.h>
 #include <linux/pm.h>
@@ -33,6 +34,67 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+/* Don't inline this: 'struct kstat' is biggish */
+static noinline long fw_file_size(struct file *file)
+{
+	struct kstat st;
+	if (vfs_getattr(file->f_path.mnt, file->f_path.dentry, &st))
+		return -1;
+	if (!S_ISREG(st.mode))
+		return -1;
+	if (st.size != (long)st.size)
+		return -1;
+	return st.size;
+}
+
+static bool fw_read_file_contents(struct file *file, struct firmware *fw)
+{
+	loff_t pos;
+	long size;
+	char *buf;
+
+	size = fw_file_size(file);
+	if (size < 0)
+		return false;
+	buf = vmalloc(size);
+	if (!buf)
+		return false;
+	pos = 0;
+	if (vfs_read(file, buf, size, &pos) != size) {
+		vfree(buf);
+		return false;
+	}
+	fw->data = buf;
+	fw->size = size;
+	return true;
+}
+
+static bool fw_get_filesystem_firmware(struct firmware *fw, const char *name)
+{
+	int i;
+	bool success = false;
+	const char *fw_path[] = { "/lib/firmware/update", "/firmware", "/lib/firmware" };
+	char *path = __getname();
+
+printk("Trying to load fw '%s' ", name);
+	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+		struct file *file;
+		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], name);
+
+		file = filp_open(path, O_RDONLY, 0);
+		if (IS_ERR(file))
+			continue;
+printk("from file '%s' ", path);
+		success = fw_read_file_contents(file, fw);
+		fput(file);
+		if (success)
+			break;
+	}
+printk(" %s.\n", success ? "Ok" : "failed");
+	__putname(path);
+	return success;
+}
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -346,7 +408,11 @@ static ssize_t firmware_loading_show(struct device *dev,
 /* firmware holds the ownership of pages */
 static void firmware_free_data(const struct firmware *fw)
 {
-	WARN_ON(!fw->priv);
+	/* Loaded directly? */
+	if (!fw->priv) {
+		vfree(fw->data);
+		return;
+	}
 	fw_free_buf(fw->priv);
 }
 
@@ -709,6 +775,11 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
 		return NULL;
 	}
 
+	if (fw_get_filesystem_firmware(firmware, name)) {
+		dev_dbg(device, "firmware: direct-loading firmware %s\n", name);
+		return NULL;
+	}
+
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
 	if (!ret)
 		fw_priv = fw_create_instance(firmware, name, device,

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Al Viro]

On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote:
> On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > +       if (!S_ISREG(inode->i_mode))
> > +               return false;
> > +       size = i_size_read(inode);
> >
> > Probably better to do vfs_getattr() and check mode and size in kstat; if
> > it's sufficiently hot for that to hurt, we are fucked anyway.
> >
> > +               file = filp_open(path, O_RDONLY, 0);
> > +               if (IS_ERR(file))
> > +                       continue;
> > +printk("from file '%s' ", path);
> > +               success = fw_read_file_contents(file, fw);
> > +               filp_close(file, NULL);
> >
> > fput(file), please.  We have enough misuses of filp_close() as it is...
> 
> Ok, like this?

Looks sane.  TBH, I'd still prefer to see udev forcibly taken over and put into
usr/udev in kernel tree - I don't trust that crowd at all and the fewer
critical userland bits they can play leverage games with, the safer we are.  

Al, that -><- close to volunteering for maintaining that FPOS kernel-side...

Re: udev breakages -

[Nix]

On 3 Oct 2012, Al Viro spake thusly:

> Looks sane.  TBH, I'd still prefer to see udev forcibly taken over and put into
> usr/udev in kernel tree - I don't trust that crowd at all and the fewer
> critical userland bits they can play leverage games with, the safer we are.  
>
> Al, that -><- close to volunteering for maintaining that FPOS kernel-side...

<flamebait type="heartfelt" subtype="fever-induced">

Please! It has already been forked at least once in userspace by people
who have the temerity to *not use systemd*, imagine that! and still want
a udev that is up-to-date in other ways. (We are now being told that,
contrary to what was said when udev was migrated into the systemd tree,
running udev without systemd is now deprecated and untested and might go
away completely. How surprising, nobody ever predicted that when it
migrated in, oh wait yes we did, and were assured that we were wrong,
that standalone udev would always be supported for those of us who
weren't using systemd. Way to destroy your userbase's trust in you...)

Possibly udev 175, the last standalone udev as far as I know, is a good
place to start from: but you might want to go back a release or two
before that, since 174 was where they started doing insane
backward-compatibility breaks. By 175 they were requiring devtmpfs, no
longer creating device nodes (which I thought was the original *point*
of udev), moving lots of install locations on the assumption that / and
/usr were on the same filesystem, and migrating udevd from /sbin into
/lib/udev without bothering to provide a backward-compatibility symlink.
Net benefit of all this thrashing about to udev users: nil. Net sysadmin
overhead on upgrade: substantial, oh and if you don't do it your system
won't boot.

By udev 175 I, and a lot of other people, had simply stopped upgrading
udev entirely on the grounds that we could no longer tolerate the
uncertainty over whether our systems would boot every time we upgraded
it, for no discernible benefit. Yes, all the incompatible changes are
(or were, as of udev 175) called out in the release notes -- but there
are so *many* of them, it's easy to miss one. And they all seem so
completely unnecessary, and their implications for your system
configuration grow more and more invasive all the time.

When gregkh was maintaining udev it was nicely robust and kept working
from release to release, and just did its job without requiring us to
change the way our system booted in backwardly-incompatible ways on
every release, merge filesystems together or mount /usr in early boot
(which is SSH-tunneled over a network in the case of one of my systems,
that was fun to do in the initramfs), or install invasive packages that
extend tentacles throughout the entire system, require a complete
rewriting of my boot process and require millions of kernel features
that may not always be turned on.

I'd like those days back. I can trust the kernel people to maintain some
semblance of userspace compatibility between releases, as is crucial for
boot-critical processes. It is now quite clear that I cannot trust the
present udev maintainers, or anyone else involved in the ongoing Linux
desktop trainwreck, to do any such thing.

</flamebait>

-- 
NULL && (void)

Re: udev breakages -

[Nix]

[Kay removed because I don't like emailing arguable flamebait directly
to the person flamed.]

On 4 Oct 2012, nix@esperi.org.uk stated:

> By udev 175 I, and a lot of other people, had simply stopped upgrading
> udev entirely on the grounds that we could no longer tolerate the
> uncertainty over whether our systems would boot every time we upgraded
> it, for no discernible benefit. Yes, all the incompatible changes are
> (or were, as of udev 175) called out in the release notes -- but there
> are so *many* of them, it's easy to miss one. And they all seem so
> completely unnecessary, and their implications for your system
> configuration grow more and more invasive all the time.

In the bright light of day I realize that this post is not as off-topic
for this thread as it appears. This is all of a piece. The udev
maintainer insists that everyone else adapt to udev's demands: before
now, it has been users, sysadmins, and userspace who must adapt, but now
is is pushing its demands in the other direction as well: this thread is
the result.

-- 
NULL && (void)

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote:
> On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > +       if (!S_ISREG(inode->i_mode))
> > +               return false;
> > +       size = i_size_read(inode);
> >
> > Probably better to do vfs_getattr() and check mode and size in kstat; if
> > it's sufficiently hot for that to hurt, we are fucked anyway.
> >
> > +               file = filp_open(path, O_RDONLY, 0);
> > +               if (IS_ERR(file))
> > +                       continue;
> > +printk("from file '%s' ", path);
> > +               success = fw_read_file_contents(file, fw);
> > +               filp_close(file, NULL);
> >
> > fput(file), please.  We have enough misuses of filp_close() as it is...
> 
> Ok, like this?

This looks good to me.  Having udev do firmware loading and tieing it to
the driver model may have not been such a good idea so many years ago.
Doing it this way makes more sense.

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> Ok, like this?
>
> This looks good to me.  Having udev do firmware loading and tieing it to
> the driver model may have not been such a good idea so many years ago.
> Doing it this way makes more sense.

Ok, I wish this had been getting more testing in Linux-next or
something, but I suspect that what I'll do is to commit this patch
asap, and then commit another patch that turns off udev firmware
loading entirely for the synchronous firmware loading case.

Why? Just to get more testing, and seeing if there are reports of
breakage. Maybe some udev out there has a different search path (or
because udev runs in a different filesystem namespace or whatever), in
which case running udev as a fallback would otherwise hide the fact
that he direct kernel firmware loading isn't working.

We can (and will) revert things if that turns out to break things, but
I'd like to make any failures of the firmware direct-load path be fast
and hard, so that we can see when/what it breaks.

Ok? Comments?

              Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kay Sievers]

On Wed, Oct 3, 2012 at 10:39 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>> Ok, like this?
>>
>> This looks good to me.  Having udev do firmware loading and tieing it to
>> the driver model may have not been such a good idea so many years ago.
>> Doing it this way makes more sense.
>
> Ok, I wish this had been getting more testing in Linux-next or
> something, but I suspect that what I'll do is to commit this patch
> asap, and then commit another patch that turns off udev firmware
> loading entirely for the synchronous firmware loading case.
>
> Why? Just to get more testing, and seeing if there are reports of
> breakage. Maybe some udev out there has a different search path (or
> because udev runs in a different filesystem namespace or whatever), in
> which case running udev as a fallback would otherwise hide the fact
> that he direct kernel firmware loading isn't working.

> Ok? Comments?

The current udev directory search order is:
  /lib/firmware/updates/$(uname -r)/
  /lib/firmware/updates/
  /lib/firmware/$(uname -r)/
  /lib/firmware/

There is no commonly known /firmware directory.

http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c#n100
http://cgit.freedesktop.org/systemd/systemd/tree/configure.ac#n548

Kay

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Wed, Oct 03, 2012 at 01:39:23PM -0700, Linus Torvalds wrote:
> On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> Ok, like this?
> >
> > This looks good to me.  Having udev do firmware loading and tieing it to
> > the driver model may have not been such a good idea so many years ago.
> > Doing it this way makes more sense.
> 
> Ok, I wish this had been getting more testing in Linux-next or
> something, but I suspect that what I'll do is to commit this patch
> asap, and then commit another patch that turns off udev firmware
> loading entirely for the synchronous firmware loading case.
> 
> Why? Just to get more testing, and seeing if there are reports of
> breakage. Maybe some udev out there has a different search path (or
> because udev runs in a different filesystem namespace or whatever), in
> which case running udev as a fallback would otherwise hide the fact
> that he direct kernel firmware loading isn't working.
> 
> We can (and will) revert things if that turns out to break things, but
> I'd like to make any failures of the firmware direct-load path be fast
> and hard, so that we can see when/what it breaks.
> 
> Ok? Comments?

I have no objection to this.  As for the firmware path, maybe we should
change that to be modified by userspace (much like /sbin/hotplug was) in
a proc file so that distros can override the location if they need to.
But for now, that's probably overkill.  This solves the problem that
Mauro and others have reported and can be easily backported by any
affected distros if needed.

thanks,

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kay Sievers]

On Wed, Oct 3, 2012 at 11:05 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

> As for the firmware path, maybe we should
> change that to be modified by userspace (much like /sbin/hotplug was) in
> a proc file so that distros can override the location if they need to.

If that's needed, a CONFIG_FIRMWARE_PATH= with the array of locations
would probably be sufficient.

Like udev's defaults here:
  http://cgit.freedesktop.org/systemd/systemd/tree/configure.ac#n550

Kay

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Alan Cox]

On Wed, 3 Oct 2012 23:18:06 +0200
Kay Sievers <kay@vrfy.org> wrote:

> On Wed, Oct 3, 2012 at 11:05 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > As for the firmware path, maybe we should
> > change that to be modified by userspace (much like /sbin/hotplug was) in
> > a proc file so that distros can override the location if they need to.
> 
> If that's needed, a CONFIG_FIRMWARE_PATH= with the array of locations
> would probably be sufficient.

The current system permits firmware to be served by a daemon, or even
assembled on the fly from parts. You break that for one.

Just fix udev, and if you can't fix it someone please just fork the last
working one.

Alan

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Lucas De Marchi]

On Wed, Oct 3, 2012 at 5:39 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>> Ok, like this?
>>
>> This looks good to me.  Having udev do firmware loading and tieing it to
>> the driver model may have not been such a good idea so many years ago.
>> Doing it this way makes more sense.
>
> Ok, I wish this had been getting more testing in Linux-next or
> something, but I suspect that what I'll do is to commit this patch
> asap, and then commit another patch that turns off udev firmware
> loading entirely for the synchronous firmware loading case.

This would break non-udev users with different paths. Namely Android,
which uses /system/etc/firmware and /system/vendor/firmware (not sure
about them though, I'm not keen on Android camp)

So maintaining the fallback or adding a configurable entry to set the
firmware paths might be good.


Lucas De Marchi

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 2:58 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
>
> So maintaining the fallback or adding a configurable entry to set the
> firmware paths might be good.

Yeah, I do think we need to make it configurable. Probably both at
kernel compile time and dynamically.

The aim of having a user-mode daemon do this was that it would be easy
to configure. Sadly, if we can't trust the daemon, that is all totally
worthless.

               Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Andy Walls]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

>On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org>
>wrote:
>>>
>>> Ok, like this?
>>
>> This looks good to me.  Having udev do firmware loading and tieing it
>to
>> the driver model may have not been such a good idea so many years
>ago.
>> Doing it this way makes more sense.
>
>Ok, I wish this had been getting more testing in Linux-next or
>something, but I suspect that what I'll do is to commit this patch
>asap, and then commit another patch that turns off udev firmware
>loading entirely for the synchronous firmware loading case.
>
>Why? Just to get more testing, and seeing if there are reports of
>breakage. Maybe some udev out there has a different search path (or
>because udev runs in a different filesystem namespace or whatever), in
>which case running udev as a fallback would otherwise hide the fact
>that he direct kernel firmware loading isn't working.
>
>We can (and will) revert things if that turns out to break things, but
>I'd like to make any failures of the firmware direct-load path be fast
>and hard, so that we can see when/what it breaks.
>
>Ok? Comments?
>
>              Linus
>--
>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

I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev

http://git.busybox.net/busybox/plain/docs/mdev.txt

Regards,
Andy

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>
> I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev
>
> http://git.busybox.net/busybox/plain/docs/mdev.txt

Heh. That web doc documents /lib/firmware as being the place to be.

That said, there's clearly enough variation here that I think that for
now I won't take the step to disable the udev part. I'll do the patch
to support "direct filesystem firmware loading" using the udev default
paths, and that hopefully fixes the particular case people see with
media modules.

We definitely want to have configurable paths and a way to configure
udev entirely off for firmware (together with a lack of paths
configuring the direct filesystem loading off - that way people can
set things up just the way they like), but since I'm travelling
tomorrow and this clearly needs more work, I'll do the first step only
for now..

                Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kay Sievers]

On Thu, Oct 4, 2012 at 12:58 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> That said, there's clearly enough variation here that I think that for
> now I won't take the step to disable the udev part. I'll do the patch
> to support "direct filesystem firmware loading" using the udev default
> paths, and that hopefully fixes the particular case people see with
> media modules.

If that approach looks like it works out, please aim for full
in-kernel-*only* support. I would absolutely like to get udev entirely
out of the sick game of firmware loading here. I would welcome if we
are not falling back to the blocking timeouted behaviour again.

The whole story would be contained entirely in the kernel, and we get
rid of the rather fragile "userspace transaction" to execute
module_init(), where the kernel has no idea if userspace is even up to
ever responding to its requests.

There would be no coordination with userspace tools needed, which
sounds like a better fit in the way we develop things with the loosely
coupled kernel <-> udev requirements.

If that works out, it would a bit like devtmpfs which turned out to be
very simple, reliable and absolutely the right thing we could do to
primarily mange /dev content.

The whole dance with the fake firmware struct device, which has a 60
second timeout to wait for userspace, is a long story of weird
failures at various aspects.

It would not only solve the unfortunate modprobe lockup with
init=/bin/sh we see here, also big servers with an insane amount of
devices happen to run into the 60 sec timeout, because udev, which
runs with 4000-8000 threads in parallel handling things like 30.000
disks is not scheduled in time to fulfill network card firmware
requests. It would be nice if we don't have that arbitrary timeout at
all.

Having any timeout at all to answer the simple question if a file
stored in the rootfs exists, should be a hint that there is something
really wrong with the model that stuff is done.

Thanks,
Kay

Re: udev breakages -

[Eric W. Biederman]

Kay Sievers <kay@vrfy.org> writes:

> If that works out, it would a bit like devtmpfs which turned out to be
> very simple, reliable and absolutely the right thing we could do to
> primarily mange /dev content.

ROFL.

There are still quite a few interesting cases that devtmpfs does not
even think about supporting.  Cases that were reported when devtmpfs was
being reviewed. 

Additionally the devtmpfs maintainership has not dealt with legitimate
concerns any better than this firmware issue has been dealt with.  I
still haven't even hear a productive suggestion back on the hole
/dev/ptmx mess.

As it happens devtmpfs wound up being a userspace process that happens
to reside in the kernel and call mknod.  How it makes sense two layers
of messaging and device management instead of just one I don't know.
Certainly I would not crow about that being a success of anything except
passing the buck.

There is debacle written all over the user space interface for dealing
with devices right now.

Eric

Re: udev breakages -

[Greg KH]

On Thu, Oct 04, 2012 at 10:29:51AM -0700, Eric W. Biederman wrote:
> There are still quite a few interesting cases that devtmpfs does not
> even think about supporting.  Cases that were reported when devtmpfs was
> being reviewed. 

Care to refresh my memory?

> Additionally the devtmpfs maintainership has not dealt with legitimate
> concerns any better than this firmware issue has been dealt with.  I
> still haven't even hear a productive suggestion back on the hole
> /dev/ptmx mess.

I don't know how to handle the /dev/ptmx issue properly from within
devtmpfs, does anyone?  Proposals are always welcome, the last time this
came up a week or so ago, I don't recall seeing any proposals, just a
general complaint.

thanks,

greg k-h

Re: udev breakages -

[Alan Cox]

> I don't know how to handle the /dev/ptmx issue properly from within
> devtmpfs, does anyone?  Proposals are always welcome, the last time this
> came up a week or so ago, I don't recall seeing any proposals, just a
> general complaint.

Is it really a problem - devtmpfs is optional. It's a problem for the
userspace folks to handle and if they made it mandatory in their code
diddums, someone better go fork working versions.

Alan

Re: udev breakages -

[Felipe Contreras]

On Thu, Oct 4, 2012 at 9:17 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> I don't know how to handle the /dev/ptmx issue properly from within
>> devtmpfs, does anyone?  Proposals are always welcome, the last time this
>> came up a week or so ago, I don't recall seeing any proposals, just a
>> general complaint.
>
> Is it really a problem - devtmpfs is optional. It's a problem for the
> userspace folks to handle and if they made it mandatory in their code
> diddums, someone better go fork working versions.

If only there was a viable alternative to udev.

Distributions are being pushed around by the udev+systemd project
precisely because of this reason; udev maintainers have said that udev
on non-systemd systems is a dead end, so everyone that uses udev
(everyone) is being forced to switch to systemd if they want to
receive proper support, and at some point there might not be even a
choice.

I for one would like an alternative to both systemd and udev on my
Linux systems, and as of yet, I don't know of one.

Cheers.

-- 
Felipe Contreras

Re: udev breakages -

[Geert Uytterhoeven]

On Wed, Oct 10, 2012 at 5:19 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Thu, Oct 4, 2012 at 9:17 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>> I don't know how to handle the /dev/ptmx issue properly from within
>>> devtmpfs, does anyone?  Proposals are always welcome, the last time this
>>> came up a week or so ago, I don't recall seeing any proposals, just a
>>> general complaint.
>>
>> Is it really a problem - devtmpfs is optional. It's a problem for the
>> userspace folks to handle and if they made it mandatory in their code
>> diddums, someone better go fork working versions.
>
> If only there was a viable alternative to udev.
>
> Distributions are being pushed around by the udev+systemd project
> precisely because of this reason; udev maintainers have said that udev
> on non-systemd systems is a dead end, so everyone that uses udev
> (everyone) is being forced to switch to systemd if they want to
> receive proper support, and at some point there might not be even a
> choice.
>
> I for one would like an alternative to both systemd and udev on my
> Linux systems, and as of yet, I don't know of one.

A few years ago, the OpenWRT people pointed me to hotplug2 when I mentioned
udev made my poor m68k box with 12 MiB of RAM immediately go OOM.
Don't know if it's suitable for "bigger" machines, though.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: udev breakages -

[Eric W. Biederman]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Thu, Oct 04, 2012 at 10:29:51AM -0700, Eric W. Biederman wrote:
>> There are still quite a few interesting cases that devtmpfs does not
>> even think about supporting.  Cases that were reported when devtmpfs was
>> being reviewed. 
>
> Care to refresh my memory?

Anyone who wants something besides the default policy.   Containers
chroots anyone who doesn't want /dev/console to be c 5 1.

>> Additionally the devtmpfs maintainership has not dealt with legitimate
>> concerns any better than this firmware issue has been dealt with.  I
>> still haven't even hear a productive suggestion back on the hole
>> /dev/ptmx mess.
>
> I don't know how to handle the /dev/ptmx issue properly from within
> devtmpfs, does anyone?  Proposals are always welcome, the last time this
> came up a week or so ago, I don't recall seeing any proposals, just a
> general complaint.

The proposal at that time was to work around the silliness with a little
kernel magic.

To recap for those who haven't watched closely.  devpts now has a ptmx
device node and it would be very nice if we were to use that device
node instead of /dev/ptmx.

Baically it would be nice to tell udev to not create /dev/ptmx, and
instead to make /dev/ptmx a symlink to /dev/pts/ptmx.

I got to looking at the problem and if I don't worry about systemd and
just look at older versions of udev that are out there in the wild it
turns out the following udev configuratoin line does exactly what is
needed.  It creats a symlink from /dev/ptmx to /dev/pts/ptmx.  And if
on the odd chance devpts is not mounted it creates /dev/pts/ptmx as
well.

KERNEL=="ptmx" NAME:="pts/ptmx" SYMLINK="ptmx"

Does assigning to NAME to specify the device naming policy work in
systemd-udev or has that capability been ripped out?

Thinking about it.  Since systemd-udev no longer supports changing the
device name.  And likely it no longer even supports assigning to NAME
even for purposes of changing the target of the symlink.  Then I expect
what we want to do is:

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 147d1a4..7dc5bed 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -377,6 +377,7 @@ static int devtmpfsd(void *p)
                goto out;
        sys_chdir("/.."); /* will traverse into overmounted root */
        sys_chroot(".");
+       sys_symlink("pts/ptmx", "ptmx");
        complete(&setup_done);
        while (1) {
                spin_lock(&req_lock);



Eric

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Josh Boyer]

On Wed, Oct 3, 2012 at 6:58 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>>
>> I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev
>>
>> http://git.busybox.net/busybox/plain/docs/mdev.txt
>
> Heh. That web doc documents /lib/firmware as being the place to be.
>
> That said, there's clearly enough variation here that I think that for
> now I won't take the step to disable the udev part. I'll do the patch
> to support "direct filesystem firmware loading" using the udev default
> paths, and that hopefully fixes the particular case people see with
> media modules.

As you probably noticed, we had a tester in the RH bug report success
with the commit you included yesterday.

Do you think this is something worth including in the stable kernels
after it gets some further testing during the merge window?  Perhaps
not that specific commit as there seems to be some additional changes
needed for configurable paths, etc, but a backport of the fleshed out
changeset might be wanted.

We have a new enough udev in Fedora 17 to hit this issue with 3.5 and
3.6 when we rebase.  I'm sure other distributions will be in similar
circumstances soon if they aren't already.  Udev isn't going to be
fixed, so having something working in these cases would be great.

josh

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Thu, Oct 04, 2012 at 09:39:41AM -0400, Josh Boyer wrote:
> > That said, there's clearly enough variation here that I think that for
> > now I won't take the step to disable the udev part. I'll do the patch
> > to support "direct filesystem firmware loading" using the udev default
> > paths, and that hopefully fixes the particular case people see with
> > media modules.
> 
> As you probably noticed, we had a tester in the RH bug report success
> with the commit you included yesterday.
> 
> Do you think this is something worth including in the stable kernels
> after it gets some further testing during the merge window?  Perhaps
> not that specific commit as there seems to be some additional changes
> needed for configurable paths, etc, but a backport of the fleshed out
> changeset might be wanted.
> 
> We have a new enough udev in Fedora 17 to hit this issue with 3.5 and
> 3.6 when we rebase.  I'm sure other distributions will be in similar
> circumstances soon if they aren't already.  Udev isn't going to be
> fixed, so having something working in these cases would be great.

Yes, I don't have a problem taking this into the stable kernel releases
once it gets some testing and fleshed out.  I'll be watching it to see
how it goes.

thanks,

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

Hi Linus,

On Wed, 3 Oct 2012 13:39:23 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> Ok, I wish this had been getting more testing in Linux-next or
> something

If you ever want a patch tested for a few days, just send it to me and I
will put it in my "fixes" tree which is merged into linux-next
immediately on top of your tree.  If nothing else, that will give it wide
build testing (see http://kisskb.ellerman.id.au/linux-next).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Andy Walls]

Greg KH <gregkh@linuxfoundation.org> wrote:

>On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote:
>> On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk>
>wrote:
>> >
>> > +       if (!S_ISREG(inode->i_mode))
>> > +               return false;
>> > +       size = i_size_read(inode);
>> >
>> > Probably better to do vfs_getattr() and check mode and size in
>kstat; if
>> > it's sufficiently hot for that to hurt, we are fucked anyway.
>> >
>> > +               file = filp_open(path, O_RDONLY, 0);
>> > +               if (IS_ERR(file))
>> > +                       continue;
>> > +printk("from file '%s' ", path);
>> > +               success = fw_read_file_contents(file, fw);
>> > +               filp_close(file, NULL);
>> >
>> > fput(file), please.  We have enough misuses of filp_close() as it
>is...
>> 
>> Ok, like this?
>
>This looks good to me.  Having udev do firmware loading and tieing it
>to
>the driver model may have not been such a good idea so many years ago.
>Doing it this way makes more sense.
>
>greg k-h
>--
>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

I agree that not calling out to userspace for firmware load is better.

Here is an old, unresolved bug about Oops on firmware loading race condition

https://bugzilla.kernel.org/show_bug.cgi?id=15294

The firmware loading timeout in the kernel was cleaning things up, just as udev what trying to say "I'm done loading the firmware" via sysfs; and then *boom*.

Regards,
Andy

Access files from kernel

[Kirill A. Shutemov]

On Wed, Oct 03, 2012 at 09:38:52AM -0700, Linus Torvalds wrote:
>+static bool fw_get_filesystem_firmware(struct firmware *fw, const char *name)
>+{
>+	int i;
>+	bool success = false;
>+	const char *fw_path[] = { "/lib/firmware/update", "/firmware", "/lib/firmware" };
>+	char *path = __getname();
>+
>+printk("Trying to load fw '%s' ", name);
>+	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>+		struct file *file;
>+		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], name);
>+
>+		file = filp_open(path, O_RDONLY, 0);

AFAIK, accessing files on filesystem form kernel directly was no-go for a
long time. What's the new rule here?

Is it worth to introduce an execption, if it's possible to solve the
problem in userspace.

-- 
 Kirill A. Shutemov

Re: Access files from kernel

[Linus Torvalds]

On Wed, Oct 3, 2012 at 12:48 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
>
> AFAIK, accessing files on filesystem form kernel directly was no-go for a
> long time. What's the new rule here?

Oh, we've *always* accessed files from the kernel.

What we don't want is random drivers doing so directly and without a
good abstraction layer, because that just ends up being a total
nightmare.

Still, they've done that too. Ugh. Too many drivers having random
hacks like that.

            Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 6:33 PM, Ming Lei <ming.lei@canonical.com> wrote:
>
> Yes, the patch will make firmware cache not working, I would like to fix
> that when I return from one trip next week.
>
> BTW, firmware cache is still needed even direct loading is taken.

I agree 100%, I'd have liked to do the caching for the direct-loading
case too. It's just that the freeing case for that is so intimately
tied to the firmware_buf format which is actually very inconvenient
for direct-loading, that making that happen looked a lot more
involved.

And I was indeed hoping you'd look at it, since you touched the code
last.  "Tag, you're it"

It shouldn't be *too* bad to instead of doing the "vmalloc()" allocate
an array of pages and then using "vmap()" instead in order to read
them (we end up doing the vmap anyway, since the firmware *user* wants
a virtually contiguous buffer), but the code will definitely get a bit
more opaque.

                        Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 02-10-2012 19:23, Greg KH escreveu:
> On Tue, Oct 02, 2012 at 03:12:39PM -0700, Greg KH wrote:
>> On Tue, Oct 02, 2012 at 09:33:03AM -0700, Linus Torvalds wrote:
>>> I don't know where the problem started in udev, but the report I saw
>>> was that udev175 was fine, and udev182 was broken, and would deadlock
>>> if module_init() did a request_firmware(). That kind of nested
>>> behavior is absolutely *required* to work, in order to not cause
>>> idiotic problems for the kernel for no good reason.
>>>
>>> What kind of insane udev maintainership do we have? And can we fix it?
>>>
>>> Greg, I think you need to step up here too. You were the one who let
>>> udev go. If the new maintainers are causing problems, they need to be
>>> fixed some way.
>>
>> I've talked about this with Kay in the past (Plumbers conference I
>> think) and I thought he said it was all fixed in the latest version of
>> udev so there shouldn't be any problems anymore with this.
>>
>> Mauro, what version of udev are you using that is still showing this
>> issue?
>>
>> Kay, didn't you resolve this already?  If not, what was the reason why?
> 
> Hm, in digging through the udev tree, the only change I found was this
> one:
> 
> commit 39177382a4f92a834b568d6ae5d750eb2a5a86f9
> Author: Kay Sievers <kay@vrfy.org>
> Date:   Thu Jul 19 12:32:24 2012 +0200
> 
>     udev: firmware - do not cancel requests in the initrd
> 
> diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
> index 56dc8fc..de93d7b 100644
> --- a/src/udev/udev-builtin-firmware.c
> +++ b/src/udev/udev-builtin-firmware.c
> @@ -129,7 +129,13 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>                                  err = -errno;
>                  } while (err == -ENOENT);
>                  rc = EXIT_FAILURE;
> -                set_loading(udev, loadpath, "-1");
> +                /*
> +                 * Do not cancel the request in the initrd, the real root might have
> +                 * the firmware file and the 'coldplug' run in the real root will find
> +                 * this pending request and fulfill or cancel it.
> +                 * */
> +                if (!in_initrd())
> +                        set_loading(udev, loadpath, "-1");
>                  goto exit;
>          }
>  
> 
> which went into udev release 187 which I think corresponds to the place
> when people started having problems, right Mauro?

I'm using here udev-182. 

> If so, Mauro, is the solution just putting the firmware into the initrd?

I don't think that putting firmware on initrd is something that we want to
for media drivers. None of the webcam drivers currently need a firmware;
those are required on more complex devices (typically digital TV ones).

While there are a number of PCI devices that require firmware, in practice,
we're seeing more people using USB devices. IMO, it doesn't make any sense
that a hot-pluggable USB device to require a firmware at initrd/initramfs,
with is available only at boot time.

> No wait, it looks like this change was trying to fix the problem where
> firmware files were not in the initrd, so it would stick around for the
> real root to show up so that they could be loaded.
>
> So this looks like it was fixing firmware loading problems for people?

I'll run some tests with this patch applied and see what happens.

> Kay, am I just looking at the totally wrong place here, and this file in
> udev didn't have anything to do with the breakage?
> 
> thanks,
> 
> greg k-h
> 

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kay Sievers]

On Wed, Oct 3, 2012 at 12:12 AM, Greg KH <gregkh@linuxfoundation.org> wrote:

> Mauro, what version of udev are you using that is still showing this
> issue?
>
> Kay, didn't you resolve this already?  If not, what was the reason why?

It's the same in the current release, we still haven't wrapped our
head around how to fix it/work around it.

Unlike what the heated and pretty uncivilized and rude emails here
claim, udev does not dead-lock or "break" things, it's just "slow".
The modprobe event handling runs into a ~30 second event timeout.
Everything is still fully functional though, there's only this delay.

Udev ensures full dependency resolution between parent and child
events. Parent events have to finish the event handling and have to
return, before child event handlers are started. We need to ensure
such things so that (among other things) disk events have finished
their operations before the partition events are started, so they can
rely and access their fully set up parent devices.

What happens here is that the module_init() call blocks in a userspace
transaction, creating a child event that is not started until the
parent event has finished. The event handler for modprobe times out
then the child event loads the firmware.

Having kernel module relying on a running and fully functional
userspace to return from module_init() is surely a broken driver
model, at least it's not how things should work. If userspace does not
respond to firmware requests, module_init() locks up until the
firmware timeout happens.

This all is not so much about how probe() should behave, it's about a
fragile dependency on a specific userspace transaction to link a
loadable module into the kernel. Drivers should avoid such loops for
many reasons. Also, it's unclear in many cases how such a model should
work at all if the module is compiled in and initialized when no
userspace is running.

If that unfortunate module_init() lockup can't be solved properly in
the kernel, we need to find out if we need to make the modprobe
handling in udev async, or let firmware events bypass dependency
resolving. As mentioned, we haven't decided as of now which road to
take here.

Thanks,
Kay

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 7:36 AM, Kay Sievers <kay@vrfy.org> wrote:
>
> If that unfortunate module_init() lockup can't be solved properly in
> the kernel

Stop this idiocy.

The kernel doesn't have a lockup problem. udev does.

As even  you admit, it is *udev* that has the whole serialization
issue, and does excessive (and incorrect) serialization between
events. Resulting in the kernel timing out a udev event that takes too
long.

The fact that you then continually try to make this a "kernel issue"
is just disgusting. Talking about the kernel solving it "properly" is
pretty dishonest, when the kernel wasn't the problem in the first
place. The kernel not only does things right, but this all used to
work fine.

                   Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Greg KH]

On Wed, Oct 03, 2012 at 04:36:53PM +0200, Kay Sievers wrote:
> On Wed, Oct 3, 2012 at 12:12 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > Mauro, what version of udev are you using that is still showing this
> > issue?
> >
> > Kay, didn't you resolve this already?  If not, what was the reason why?
> 
> It's the same in the current release, we still haven't wrapped our
> head around how to fix it/work around it.

Ick, as this is breaking people's previously-working machines, shouldn't
this be resolved quickly?

> Unlike what the heated and pretty uncivilized and rude emails here
> claim, udev does not dead-lock or "break" things, it's just "slow".
> The modprobe event handling runs into a ~30 second event timeout.
> Everything is still fully functional though, there's only this delay.

Mauro said it broke the video drivers.  Mauro, if you wait 30 seconds,
does everything then "work"?

Not to say that waiting 30 seconds is a correct thing here...

> Udev ensures full dependency resolution between parent and child
> events. Parent events have to finish the event handling and have to
> return, before child event handlers are started. We need to ensure
> such things so that (among other things) disk events have finished
> their operations before the partition events are started, so they can
> rely and access their fully set up parent devices.
> 
> What happens here is that the module_init() call blocks in a userspace
> transaction, creating a child event that is not started until the
> parent event has finished. The event handler for modprobe times out
> then the child event loads the firmware.

module_init() can do lots of "bad" things, sleeping, asking for
firmware, and lots of other things.  To have userspace block because of
this doesn't seem very wise.

> Having kernel module relying on a running and fully functional
> userspace to return from module_init() is surely a broken driver
> model, at least it's not how things should work. If userspace does not
> respond to firmware requests, module_init() locks up until the
> firmware timeout happens.

But previously this all "just worked" as we ran 'modprobe' in a new
thread/process right?  What's wrong with going back to just execing
modprobe and letting that process go off and do what ever it wants to
do?  It can't be that "expensive" as modprobe is a very slow thing, and
it should solve this issue.  udev will then have handled the 'a device
has shown up, run modprobe' event in the correct order, and then
anything else that the module_init() process wants to do, it can do
without worrying about stopping anything else in the system that might
want to happen at the same time (like load multiple modules in a row).

> This all is not so much about how probe() should behave, it's about a
> fragile dependency on a specific userspace transaction to link a
> loadable module into the kernel. Drivers should avoid such loops for
> many reasons. Also, it's unclear in many cases how such a model should
> work at all if the module is compiled in and initialized when no
> userspace is running.
> 
> If that unfortunate module_init() lockup can't be solved properly in
> the kernel, we need to find out if we need to make the modprobe
> handling in udev async, or let firmware events bypass dependency
> resolving. As mentioned, we haven't decided as of now which road to
> take here.

It's not a lockup, there have never been rules about what a driver could
and could not do in its module_init() function.  Sure, there are some
not-nice drivers out there, but don't halt the whole system just because
of them.

I recommend making module loading async, like it used to be, and then
all should be fine, right?

That's also the way the mdev works, and I don't think that people have
been having problems there. :)

thanks,

greg k-h

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kay Sievers]

On Wed, Oct 3, 2012 at 6:57 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

>> It's the same in the current release, we still haven't wrapped our
>> head around how to fix it/work around it.
>
> Ick, as this is breaking people's previously-working machines, shouldn't
> this be resolved quickly?

Nothing really "breaks", It's "slow" and it will surely be fixed when
we know what's the right fix, which we haven't sorted out at this
moment.

> module_init() can do lots of "bad" things, sleeping, asking for
> firmware, and lots of other things.  To have userspace block because of
> this doesn't seem very wise.

Not saying that it is right or nice, but it's the kernel itself that
blocks. Run init=/bin/sh and do a modprobe of one of these drivers and
it hangs un-interruptible until the kernel's internal firmware loading
request times out, just because userspace is not there.

> But previously this all "just worked" as we ran 'modprobe' in a new
> thread/process right?

No, we used to un-queue events which had a timeout specified in the
environment, that code caused other issues and was removed.

> it can do without worrying about stopping anything else in the system that might
> want to happen at the same time (like load multiple modules in a row).

It should not be an issue, the serialization is strictly parent <->
child, everything else runs in parallel.

>> If that unfortunate module_init() lockup can't be solved properly in
>> the kernel, we need to find out if we need to make the modprobe
>> handling in udev async, or let firmware events bypass dependency
>> resolving. As mentioned, we haven't decided as of now which road to
>> take here.
>
> It's not a lockup, there have never been rules about what a driver could
> and could not do in its module_init() function.  Sure, there are some
> not-nice drivers out there, but don't halt the whole system just because
> of them.

It is a kind of lock up, just try modprobe with the init=/bin/sh boot.

> I recommend making module loading async, like it used to be, and then
> all should be fine, right?

That's the current idea, and Tom is looking into it how it could look like.

I also have no issues at all if the kernel does load the firmware from
the filesystem on its own; it sounds like the simplest and most robust
solution from a general look at the problem. It would also make the
difference between in-kernel firmware and out-of-kernel firmware less
visible, which sounds good.
Honestly, requiring firmware-loading userspace-transactions to
successfully link a module into the kernel sounds like a pretty bad
idea to start with. Unlike module loading which needs the depmod alias
database and userspace configuration; with firmware loading, there is
no policy involved where userspace would add any single additional
value to that step.

Thanks,
Kay

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Linus Torvalds]

On Wed, Oct 3, 2012 at 10:24 AM, Kay Sievers <kay@vrfy.org> wrote:
>
> Nothing really "breaks", It's "slow" and it will surely be fixed when
> we know what's the right fix, which we haven't sorted out at this
> moment.

A thirty-second pause at bootup is easily long enough that some people
might think the machine is hung.

I also call bullshit on your "it will surely be fixed when we know
what's the right fix" excuses.

The fact is, you've spent the last several months blaming everybody
but yourself, and actively told people to stop blaming you:

    https://bugzilla.redhat.com/show_bug.cgi?id=827538#c12

and have ignored patches that were sent to you:

    http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html

despite having clearly seen the patch (you *replied* to it, for
chissake, and I even told you in that same thread why that reply was
wrong at the time).

> I also have no issues at all if the kernel does load the firmware from
> the filesystem on its own; it sounds like the simplest and most robust
> solution from a general look at the problem. It would also make the
> difference between in-kernel firmware and out-of-kernel firmware less
> visible, which sounds good.

So now, after you've dismissed the patch that did the equivalent fix
in udev (Ming Lei's patch basically disabled your idiotic and wrong
sequence number test for firmware loading), you say it's ok to bypass
udev entirely, because that is "more robust".

Kay, you are so full of sh*t that it's not funny. You're refusing to
acknowledge your bugs, you refuse to fix them even when a patch is
sent to you, and then you make excuses for the fact that we have to
work around *your* bugs, and say that we should have done so from the
very beginning.

Yes, doing it in the kernel is "more robust". But don't play games,
and stop the lying. It's more robust because we have maintainers that
care, and because we know that regressions are not something we can
play fast and loose with. If something breaks, and we don't know what
the right fix for that breakage is, we *revert* the thing that broke.

So yes, we're clearly better off doing it in the kernel.

Not because firmware loading cannot be done in user space. But simply
because udev maintenance since Greg gave it up has gone downhill.

                    Linus

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab]

Em 03-10-2012 13:57, Greg KH escreveu:
> On Wed, Oct 03, 2012 at 04:36:53PM +0200, Kay Sievers wrote:
>> On Wed, Oct 3, 2012 at 12:12 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>>> Mauro, what version of udev are you using that is still showing this
>>> issue?
>>>
>>> Kay, didn't you resolve this already?  If not, what was the reason why?
>>
>> It's the same in the current release, we still haven't wrapped our
>> head around how to fix it/work around it.
> 
> Ick, as this is breaking people's previously-working machines, shouldn't
> this be resolved quickly?
> 
>> Unlike what the heated and pretty uncivilized and rude emails here
>> claim, udev does not dead-lock or "break" things, it's just "slow".
>> The modprobe event handling runs into a ~30 second event timeout.
>> Everything is still fully functional though, there's only this delay.
> 
> Mauro said it broke the video drivers.  Mauro, if you wait 30 seconds,
> does everything then "work"?
> 
> Not to say that waiting 30 seconds is a correct thing here...

Before the implementation of the DVB async firmware load, as requested
by udev maintainers, and on a test case, the additional 30 seconds
wait won't hurt[1].

[1] Well, I didn't test any tm6000 devices with Kernel 3.6. On tm6000,
firmwares are needed for xc3028 tuners, via the board internal I2C bus.
Those devices only support 10 kHz speed for those transfers. Also, due
to a bug at the hardware, extra delay is needed. So, a firmware load
there takes ~30 seconds. I suspect that waiting for ~60 seconds will
trigger the max allowed time for firmware load to happen, actually
breaking completely the driver.

There are applications like MythTV, however, that opens all interfaces
produced by a media device at the same time (alsa, video nodes, dvb nodes).

I'm not a MythTV user myself, but someone told me recently that MythTV
has a 5 seconds timeout there. If it can't open an interface on that time,
it will give up for that device.

As you likely know, MythTV is used as a Media Center: there are distros
that load it (or some other applications like XBMC) at boot time.

As the media drivers are modular, the DVB, alsa and TV API's are implemented
on (almost) independent modules. On such scenario, if the DVB firmware 
load takes 30 seconds, because the DVB demod requires a firmware,
while the analog TV doesn't require a firmware, I suspect that Mythtv
will assume that DVB is not there, causing a failure to the end user.

[1] The attempt to fix the issues with udev forced us to re-design a few
drivers. One of them (drxk) now loads firmware asynchronously. That effectively
broke the driver on Kernel 3.6, for PCTV520e and similar devices, as the
tuner driver, attached just after drx-k, stopped working, likely due to
some missing clock or GPIO signals that drx-k is supposed to rise after
firmware load.

The fixes for it are at:
	http://git.linuxtv.org/media_tree.git/commit/6ae5e060840589f567c1837613e8a9d34fc9188a
	http://git.linuxtv.org/media_tree.git/commit/8e30783b0b3270736b2cff6415c68b894bc411df
	http://git.linuxtv.org/media_tree.git/commit/2425bb3d4016ed95ce83a90b53bd92c7f31091e4

In order to fix that issues, I had to defer DVB initialization for all em28xx
devices, in despite of the fact that only 5 boards (of 86 devices supported there)
require DVB firmwares. Patches are likely a little pedantic, as they are
doing async DVB initialization even when the drivers are compiled builtin,
but, as I do expect to revert the entire async thing from em28xx/drx-k, 
I didn't want to make too big changes there.

FYI, I'm planning to upstream those fixes tomorrow, if nobody detects
any issue on this approach.

> I recommend making module loading async, like it used to be, and then
> all should be fine, right?

IMHO, module loading should be async: it seems a way better and more 
POSIX compliant to take more time during init, than delaying firmware
init to happen open(),  especially when open() is called on non-block mode.

Regards,
Mauro

Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Jean Delvare]

Hi Mauro,

On Mon, 25 Jun 2012 17:06:28 -0300, Mauro Carvalho Chehab wrote:
> That's said, IMO, the best approach is to do:
> 
> 1) add support for asynchronous probe at device core, for devices that requires firmware
> at probe(). The async_probe() will only be active if !usermodehelper_disabled.
> 
> 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface.

Both functions are already exported since kernel 2.6.32. However these
functions were originally made public for the shared pin case, where
two pins can be used for either I2C or something else, and we have to
prevent I2C usage when the other function is used. This does not help
in your case. What you need additionally is that unlocked flavors of
some i2c transfer functions (at least i2c_transfer itself) are exported
as well.

This isn't necessarily trivial though, as the locking and unlocking are
taking place inside i2c_transfer(), not at its boundaries. I'm looking
into this now.

-- 
Jean Delvare

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Kurt H Maier]

On Wed, Oct 03, 2012 at 07:27:01PM +0000, Al Viro wrote:
> 
> Al, that -><- close to volunteering for maintaining that FPOS kernel-side...
> 

This would be fantastic.

Kurt H Maier

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Emmanuel Benisty]

On Thu, Oct 4, 2012 at 9:50 PM, Kurt H Maier <khm@sdf.org> wrote:
> On Wed, Oct 03, 2012 at 07:27:01PM +0000, Al Viro wrote:
>>
>> Al, that -><- close to volunteering for maintaining that FPOS kernel-side...
>>
>
> This would be fantastic.

And that would solve this very much worrying issue [1], quoting:
"(Yes, udev on non-systemd systems is in our eyes a dead end, in case you
haven't noticed it yet. I am looking forward to the day when we can drop
that support entirely.)"

[1] http://lists.freedesktop.org/archives/systemd-devel/2012-August/006066.html

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[david]

> On Thu, Oct 4, 2012 at 9:50 PM, Kurt H Maier <khm@sdf.org> wrote:
>> On Wed, Oct 03, 2012 at 07:27:01PM +0000, Al Viro wrote:
>>>
>>> Al, that -><- close to volunteering for maintaining that FPOS kernel-side...
>>>
>>
>> This would be fantastic.
>
> And that would solve this very much worrying issue [1], quoting:
> "(Yes, udev on non-systemd systems is in our eyes a dead end, in case you
> haven't noticed it yet. I am looking forward to the day when we can drop
> that support entirely.)"
>
> [1] http://lists.freedesktop.org/archives/systemd-devel/2012-August/006066.html

I think it's worth noting that even though udev 189 was recently released, 
the not-yet-released Ubuntu 10.10 is going to be including udev 175.

David Lang

Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Henrique de Moraes Holschuh]

On Fri, 05 Oct 2012, david@lang.hm wrote:
> >On Thu, Oct 4, 2012 at 9:50 PM, Kurt H Maier <khm@sdf.org> wrote:
> >>On Wed, Oct 03, 2012 at 07:27:01PM +0000, Al Viro wrote:
> >>>Al, that -><- close to volunteering for maintaining that FPOS kernel-side...
> >>
> >>This would be fantastic.
> >
> >And that would solve this very much worrying issue [1], quoting:
> >"(Yes, udev on non-systemd systems is in our eyes a dead end, in case you
> >haven't noticed it yet. I am looking forward to the day when we can drop
> >that support entirely.)"
> >
> >[1] http://lists.freedesktop.org/archives/systemd-devel/2012-August/006066.html
> 
> I think it's worth noting that even though udev 189 was recently
> released, the not-yet-released Ubuntu 10.10 is going to be including
> udev 175.

So is Debian.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh