[PATCH 1/2] ASoC: Intel: Move extended fw base and size fields in struct sst_pdata

[PATCH 1/2] ASoC: Intel: Move extended fw base and size fields in struct sst_pdata

[Jarkko Nikula]

Move fw_base and fw_size fields in struct sst_pdata under ACPI data for
clarifying that these are not related to firmware file but for platform
specific extended firmware area reserved by the BIOS.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
---
 sound/soc/intel/sst-dsp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/sst-dsp.h b/sound/soc/intel/sst-dsp.h
index 0ce5c8d91794..d134359fecac 100644
--- a/sound/soc/intel/sst-dsp.h
+++ b/sound/soc/intel/sst-dsp.h
@@ -147,12 +147,12 @@ struct sst_pdata {
 	u32 lpe_size;
 	u32 pcicfg_base;
 	u32 pcicfg_size;
+	u32 fw_base;
+	u32 fw_size;
 	int irq;
 
 	/* Firmware */
 	const char *fw_filename;
-	u32 fw_base;
-	u32 fw_size;
 
 	/* DMA */
 	u32 dma_base;
-- 
1.8.5.3

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

[Jarkko Nikula]

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 <jarkko.nikula@linux.intel.com>
Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
---
 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);
 }
 
 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;
-- 
1.8.5.3

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

[Takashi Iwai]

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 <jarkko.nikula@linux.intel.com>
> Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> ---
>  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
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 1/2] ASoC: Intel: Move extended fw base and size fields in struct sst_pdata

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 285 bytes --]

On Tue, Feb 18, 2014 at 04:42:02PM +0200, Jarkko Nikula wrote:
> Move fw_base and fw_size fields in struct sst_pdata under ACPI data for
> clarifying that these are not related to firmware file but for platform
> specific extended firmware area reserved by the BIOS.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Jarkko Nikula]

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

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

[Jarkko Nikula]

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.

-- 
Jarkko

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

[Takashi Iwai]

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

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

[Jarkko Nikula]

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.

-- 
Jarkko