[2/2] ASoC: Intel: sst-acpi: Request firmware before SST platform driver probing

Commit Message

Jarkko Nikula Feb. 18, 2014, 2:42 p.m. UTC

We originally thought to request SST audio DSP firmware during the SST
platform driver initialization. However plain request_firmware doesn't
work in driver probe paths if userspace is not ready to handle it. For
instance when drivers are built-in.

Implementing asynchronous firmware request in SST platform driver
initialization complicates code needlessly since it anyway will fail if
firmware is missing.

This is more simple to handle by requesting firmware asynchronously in
sst_acpi_probe() and register SST platform only after firmware is loaded.

Signed-off-by: Jarkko Nikula <[e-mail]>
Acked-by: Liam Girdwood <[e-mail]>
---
 sound/soc/intel/sst-acpi.c | 54 ++++++++++++++++++++++++++++++----------------
 sound/soc/intel/sst-dsp.h  |  2 +-
 2 files changed, 36 insertions(+), 20 deletions(-)

Comments

Takashi Iwai Feb. 18, 2014, 2:58 p.m. UTC | #1

At Tue, 18 Feb 2014 16:42:03 +0200,
Jarkko Nikula wrote:
> 
> We originally thought to request SST audio DSP firmware during the SST
> platform driver initialization. However plain request_firmware doesn't
> work in driver probe paths if userspace is not ready to handle it. For
> instance when drivers are built-in.
> 
> Implementing asynchronous firmware request in SST platform driver
> initialization complicates code needlessly since it anyway will fail if
> firmware is missing.
> 
> This is more simple to handle by requesting firmware asynchronously in
> sst_acpi_probe() and register SST platform only after firmware is loaded.
> 
> Signed-off-by: Jarkko Nikula <[e-mail]>
> Acked-by: Liam Girdwood <[e-mail]>
> ---
>  sound/soc/intel/sst-acpi.c | 54 ++++++++++++++++++++++++++++++----------------
>  sound/soc/intel/sst-dsp.h  |  2 +-
>  2 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/soc/intel/sst-acpi.c b/sound/soc/intel/sst-acpi.c
> index aba73ca8a923..b76d9d9f15c0 100644
> --- a/sound/soc/intel/sst-acpi.c
> +++ b/sound/soc/intel/sst-acpi.c
> @@ -16,6 +16,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/device.h>
> +#include <linux/firmware.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  
> @@ -56,6 +57,32 @@ struct sst_acpi_priv {
>  	struct sst_acpi_desc *desc;
>  };
>  
> +static void sst_acpi_fw_cb(const struct firmware *fw, void *context)
> +{
> +	struct platform_device *pdev = context;
> +	struct device *dev = &pdev->dev;
> +	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
> +	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
> +	struct sst_acpi_desc *desc = sst_acpi->desc;
> +
> +	sst_pdata->fw = fw;
> +	if (!fw) {
> +		dev_err(dev, "Cannot load firmware %s\n", desc->fw_filename);
> +		return;
> +	}
> +
> +	/* register PCM and DAI driver */
> +	sst_acpi->pdev_pcm =
> +		platform_device_register_data(dev, desc->drv_name, -1,
> +					      sst_pdata, sizeof(*sst_pdata));
> +	if (IS_ERR(sst_acpi->pdev_pcm)) {
> +		dev_err(dev, "Cannot register device %s. Error %d\n",
> +			desc->drv_name, (int)PTR_ERR(sst_acpi->pdev_pcm));
> +	}
> +
> +	return;
> +}
> +
>  static int sst_acpi_probe(struct platform_device *pdev)
>  {
>  	const struct acpi_device_id *id;
> @@ -65,7 +92,6 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	struct sst_acpi_mach *mach;
>  	struct sst_acpi_desc *desc;
>  	struct resource *mmio;
> -	int ret = 0;
>  
>  	sst_acpi = devm_kzalloc(dev, sizeof(*sst_acpi), GFP_KERNEL);
>  	if (sst_acpi == NULL)
> @@ -79,7 +105,6 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	desc = mach->res_desc;
>  	sst_pdata = &sst_acpi->sst_pdata;
>  	sst_pdata->id = desc->sst_id;
> -	sst_pdata->fw_filename = desc->fw_filename;
>  	sst_acpi->desc = desc;
>  
>  	if (desc->resindex_dma_base >= 0) {
> @@ -118,37 +143,28 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/* register PCM and DAI driver */
> -	sst_acpi->pdev_pcm =
> -		platform_device_register_data(dev, desc->drv_name, -1,
> -					      sst_pdata, sizeof(*sst_pdata));
> -	if (IS_ERR(sst_acpi->pdev_pcm))
> -		return PTR_ERR(sst_acpi->pdev_pcm);
> -
> -	/* register machine driver */
>  	platform_set_drvdata(pdev, sst_acpi);
>  
> +	/* register machine driver */
>  	sst_acpi->pdev_mach =
>  		platform_device_register_data(dev, mach->drv_name, -1,
>  					      sst_pdata, sizeof(*sst_pdata));
> -	if (IS_ERR(sst_acpi->pdev_mach)) {
> -		ret = PTR_ERR(sst_acpi->pdev_mach);
> -		goto sst_err;
> -	}
> -
> -	return ret;
> +	if (IS_ERR(sst_acpi->pdev_mach))
> +		return PTR_ERR(sst_acpi->pdev_mach);
>  
> -sst_err:
> -	platform_device_unregister(sst_acpi->pdev_pcm);
> -	return ret;
> +	/* continue SST probing after firmware is loaded */
> +	return request_firmware_nowait(THIS_MODULE, true, desc->fw_filename,
> +				       dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);

sst_acpi->pdev_mach still should be unregistered when
request_firmware_nowait() returns an error.


>  }
>  
>  static int sst_acpi_remove(struct platform_device *pdev)
>  {
>  	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
> +	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
>  
>  	platform_device_unregister(sst_acpi->pdev_mach);
>  	platform_device_unregister(sst_acpi->pdev_pcm);

With your patch, pdev_pcm isn't always a valid pointer.  You can't
pass it unconditionally any longer.


Takashi


> +	release_firmware(sst_pdata->fw);
>  
>  	return 0;
>  }
> diff --git a/sound/soc/intel/sst-dsp.h b/sound/soc/intel/sst-dsp.h
> index d134359fecac..3730fd324455 100644
> --- a/sound/soc/intel/sst-dsp.h
> +++ b/sound/soc/intel/sst-dsp.h
> @@ -152,7 +152,7 @@ struct sst_pdata {
>  	int irq;
>  
>  	/* Firmware */
> -	const char *fw_filename;
> +	const struct firmware *fw;
>  
>  	/* DMA */
>  	u32 dma_base;
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Jarkko Nikula Feb. 19, 2014, 6:57 a.m. UTC | #2

Hi

On 02/18/2014 04:58 PM, Takashi Iwai wrote:
> At Tue, 18 Feb 2014 16:42:03 +0200,
> Jarkko Nikula wrote:
>>   
>> -sst_err:
>> -	platform_device_unregister(sst_acpi->pdev_pcm);
>> -	return ret;
>> +	/* continue SST probing after firmware is loaded */
>> +	return request_firmware_nowait(THIS_MODULE, true, desc->fw_filename,
>> +				       dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);
> sst_acpi->pdev_mach still should be unregistered when
> request_firmware_nowait() returns an error.
I was thinking to leave that for sst_acpi_remove but you are right, it 
doesn't make sense to leave it registered for instance if 
request_firmware_nowait fails because of -ENOMEM or some other fatal issue.

>>   }
>>   
>>   static int sst_acpi_remove(struct platform_device *pdev)
>>   {
>>   	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
>> +	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
>>   
>>   	platform_device_unregister(sst_acpi->pdev_mach);
>>   	platform_device_unregister(sst_acpi->pdev_pcm);
> With your patch, pdev_pcm isn't always a valid pointer.  You can't
> pass it unconditionally any longer.
>
I felt it was needless to test NULL pointers here since release_firmware 
checks it directly and platform_device_unregister indirectly. Not in 
platform_device_unregister but when calling platform_device_del and 
platform_device_put there.

Jarkko Nikula Feb. 19, 2014, 7:09 a.m. UTC | #3

On 02/19/2014 08:57 AM, Jarkko Nikula wrote:
> Hi
>
> On 02/18/2014 04:58 PM, Takashi Iwai wrote:
>> At Tue, 18 Feb 2014 16:42:03 +0200,
>> Jarkko Nikula wrote:
>>>   -sst_err:
>>> -    platform_device_unregister(sst_acpi->pdev_pcm);
>>> -    return ret;
>>> +    /* continue SST probing after firmware is loaded */
>>> +    return request_firmware_nowait(THIS_MODULE, true, 
>>> desc->fw_filename,
>>> +                       dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);
>> sst_acpi->pdev_mach still should be unregistered when
>> request_firmware_nowait() returns an error.
> I was thinking to leave that for sst_acpi_remove but you are right, it 
> doesn't make sense to leave it registered for instance if 
> request_firmware_nowait fails because of -ENOMEM or some other fatal 
> issue.
>
Answering myself after cup of coffee... Obviously probe function must 
free whatever it allocated when it returns with an error. In other words 
module loading fails and we won't ever call the remove function.

Takashi Iwai Feb. 19, 2014, 7:15 a.m. UTC | #4

At Wed, 19 Feb 2014 08:57:53 +0200,
Jarkko Nikula wrote:
> 
> Hi
> 
> On 02/18/2014 04:58 PM, Takashi Iwai wrote:
> > At Tue, 18 Feb 2014 16:42:03 +0200,
> > Jarkko Nikula wrote:
> >>   
> >> -sst_err:
> >> -	platform_device_unregister(sst_acpi->pdev_pcm);
> >> -	return ret;
> >> +	/* continue SST probing after firmware is loaded */
> >> +	return request_firmware_nowait(THIS_MODULE, true, desc->fw_filename,
> >> +				       dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);
> > sst_acpi->pdev_mach still should be unregistered when
> > request_firmware_nowait() returns an error.
> I was thinking to leave that for sst_acpi_remove but you are right, it 
> doesn't make sense to leave it registered for instance if 
> request_firmware_nowait fails because of -ENOMEM or some other fatal issue.
> 
> >>   }
> >>   
> >>   static int sst_acpi_remove(struct platform_device *pdev)
> >>   {
> >>   	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
> >> +	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
> >>   
> >>   	platform_device_unregister(sst_acpi->pdev_mach);
> >>   	platform_device_unregister(sst_acpi->pdev_pcm);
> > With your patch, pdev_pcm isn't always a valid pointer.  You can't
> > pass it unconditionally any longer.
> >
> I felt it was needless to test NULL pointers here since release_firmware 
> checks it directly and platform_device_unregister indirectly. Not in 
> platform_device_unregister but when calling platform_device_del and 
> platform_device_put there.

The problem is that it may contain ERR_PTR(xxx).  You have to either
clear to NULL in

	if (IS_ERR(sst_acpi->pdev_pcm)) {
		dev_err(dev, "Cannot register device %s. Error %d\n",
			desc->drv_name, (int)PTR_ERR(sst_acpi->pdev_pcm));
		sst_acpi->pdev_pcm = NULL;
	}

or check conditionally like

	if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm))
	 	platform_device_unregister(sst_acpi->pdev_pcm);
	
Maybe the former is better.


Takashi

Jarkko Nikula Feb. 19, 2014, 7:28 a.m. UTC | #5

Hi

On 02/19/2014 09:15 AM, Takashi Iwai wrote:
> At Wed, 19 Feb 2014 08:57:53 +0200,
> Jarkko Nikula wrote:
>>
>> I felt it was needless to test NULL pointers here since release_firmware
>> checks it directly and platform_device_unregister indirectly. Not in
>> platform_device_unregister but when calling platform_device_del and
>> platform_device_put there.
> The problem is that it may contain ERR_PTR(xxx).  You have to either
> clear to NULL in
>
> 	if (IS_ERR(sst_acpi->pdev_pcm)) {
> 		dev_err(dev, "Cannot register device %s. Error %d\n",
> 			desc->drv_name, (int)PTR_ERR(sst_acpi->pdev_pcm));
> 		sst_acpi->pdev_pcm = NULL;
> 	}
>
> or check conditionally like
>
> 	if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm))
> 	 	platform_device_unregister(sst_acpi->pdev_pcm);
> 	
> Maybe the former is better.
>
Oh yes and I'm even testing it with the IS_ERR and PTR_ERR. Why that 
knowledge did not carry to sst_acpi_remove... will fix.

Patch

diff --git a/sound/soc/intel/sst-acpi.c b/sound/soc/intel/sst-acpi.c
index aba73ca8a923..b76d9d9f15c0 100644
--- a/sound/soc/intel/sst-acpi.c
+++ b/sound/soc/intel/sst-acpi.c
@@ -16,6 +16,7 @@ 
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/firmware.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
@@ -56,6 +57,32 @@  struct sst_acpi_priv {
 	struct sst_acpi_desc *desc;
 };
 
+static void sst_acpi_fw_cb(const struct firmware *fw, void *context)
+{
+	struct platform_device *pdev = context;
+	struct device *dev = &pdev->dev;
+	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
+	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
+	struct sst_acpi_desc *desc = sst_acpi->desc;
+
+	sst_pdata->fw = fw;
+	if (!fw) {
+		dev_err(dev, "Cannot load firmware %s\n", desc->fw_filename);
+		return;
+	}
+
+	/* register PCM and DAI driver */
+	sst_acpi->pdev_pcm =
+		platform_device_register_data(dev, desc->drv_name, -1,
+					      sst_pdata, sizeof(*sst_pdata));
+	if (IS_ERR(sst_acpi->pdev_pcm)) {
+		dev_err(dev, "Cannot register device %s. Error %d\n",
+			desc->drv_name, (int)PTR_ERR(sst_acpi->pdev_pcm));
+	}
+
+	return;
+}
+
 static int sst_acpi_probe(struct platform_device *pdev)
 {
 	const struct acpi_device_id *id;
@@ -65,7 +92,6 @@  static int sst_acpi_probe(struct platform_device *pdev)
 	struct sst_acpi_mach *mach;
 	struct sst_acpi_desc *desc;
 	struct resource *mmio;
-	int ret = 0;
 
 	sst_acpi = devm_kzalloc(dev, sizeof(*sst_acpi), GFP_KERNEL);
 	if (sst_acpi == NULL)
@@ -79,7 +105,6 @@  static int sst_acpi_probe(struct platform_device *pdev)
 	desc = mach->res_desc;
 	sst_pdata = &sst_acpi->sst_pdata;
 	sst_pdata->id = desc->sst_id;
-	sst_pdata->fw_filename = desc->fw_filename;
 	sst_acpi->desc = desc;
 
 	if (desc->resindex_dma_base >= 0) {
@@ -118,37 +143,28 @@  static int sst_acpi_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* register PCM and DAI driver */
-	sst_acpi->pdev_pcm =
-		platform_device_register_data(dev, desc->drv_name, -1,
-					      sst_pdata, sizeof(*sst_pdata));
-	if (IS_ERR(sst_acpi->pdev_pcm))
-		return PTR_ERR(sst_acpi->pdev_pcm);
-
-	/* register machine driver */
 	platform_set_drvdata(pdev, sst_acpi);
 
+	/* register machine driver */
 	sst_acpi->pdev_mach =
 		platform_device_register_data(dev, mach->drv_name, -1,
 					      sst_pdata, sizeof(*sst_pdata));
-	if (IS_ERR(sst_acpi->pdev_mach)) {
-		ret = PTR_ERR(sst_acpi->pdev_mach);
-		goto sst_err;
-	}
-
-	return ret;
+	if (IS_ERR(sst_acpi->pdev_mach))
+		return PTR_ERR(sst_acpi->pdev_mach);
 
-sst_err:
-	platform_device_unregister(sst_acpi->pdev_pcm);
-	return ret;
+	/* continue SST probing after firmware is loaded */
+	return request_firmware_nowait(THIS_MODULE, true, desc->fw_filename,
+				       dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);
 }
 
 static int sst_acpi_remove(struct platform_device *pdev)
 {
 	struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);
+	struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;
 
 	platform_device_unregister(sst_acpi->pdev_mach);
 	platform_device_unregister(sst_acpi->pdev_pcm);
+	release_firmware(sst_pdata->fw);
 
 	return 0;
 }
diff --git a/sound/soc/intel/sst-dsp.h b/sound/soc/intel/sst-dsp.h
index d134359fecac..3730fd324455 100644
--- a/sound/soc/intel/sst-dsp.h
+++ b/sound/soc/intel/sst-dsp.h
@@ -152,7 +152,7 @@  struct sst_pdata {
 	int irq;
 
 	/* Firmware */
-	const char *fw_filename;
+	const struct firmware *fw;
 
 	/* DMA */
 	u32 dma_base;