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