Skip to content

ASoC: sof: add check of NULL in suspend/resume #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
libinyang wants to merge 1 commit into from
Closed

ASoC: sof: add check of NULL in suspend/resume #510

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ static int sof_resume(struct device *dev, bool runtime_resume)
struct snd_sof_dev *sdev = dev_get_drvdata(&priv->pdev_pcm->dev);
int ret = 0;

if (!sdev)
return 0;

@plbossart plbossart Jan 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this really happen? the sof_pci_probe function starts with

static int sof_pci_probe(struct pci_dev *pci,
			 const struct pci_device_id *pci_id)
{
[...]
	dev_dbg(&pci->dev, "PCI DSP detected");

	/* get ops for platform */
	ops = desc->ops;
	if (!ops) {
		dev_err(dev, "error: no matching PCI descriptor ops\n");
		return -ENODEV;
	}

	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
	if (!priv)
		return -ENOMEM;

	pci_set_drvdata(pci, priv);

so if "priv" was ever NULL we would not even be in the PM stuff.

@libinyang libinyang Jan 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart
Yes, you are right. This should never happen logically. I add this code just for sanity check in case our code is wrong...
If you don't like it, I will remove this check to make the code simple.

@plbossart plbossart Jan 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise when is this going to happen? if we ever go through sof_probe() in core.c, then sdev will never be NULL.

@libinyang libinyang Jan 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will happen in our case. If fw fails to load, the platform_device (not pci_device) driver probe will fail. Sequentially, the device-driver architecture will set dev->driver_data to NULL automatically.
If you are interested in the details, you can refer to ./driver/base/dd.c file.

@plbossart plbossart Jan 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if the platform_device probe fails, you will not even invoke pm related functions. Please detail the flow, thanks.

@libinyang libinyang Jan 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PM functions belongs to pci device, not platform device. PCI device driver is registered successfully.

@plbossart plbossart Jan 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see sof-pci-dev.c:

	/* register sof-audio platform driver */
	ret = sof_create_platform_device(priv);
	if (ret) {
		dev_err(dev, "error: failed to create platform device!\n");
		goto release_regions;
	}

>>> SEE HERE, runtime_pm is not enabled !!!

	/* allow runtime_pm */
	pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
	pm_runtime_use_autosuspend(dev);

	/*
	 * runtime pm for pci device is "forbidden" by default.
	 * so call pm_runtime_allow() to enable it.
	 */
	pm_runtime_allow(dev);

	/* follow recommendation in pci-driver.c to decrement usage counter */
	pm_runtime_put_noidle(dev);

	return ret;

release_regions:
	pci_release_regions(pci);

	return ret;

@libinyang libinyang Jan 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure is not sof_create_platform_device(), but the platform device driver sof_probe() in sof/core.c. They are different things. sof_create_platform_device() is successful.

@plbossart plbossart Jan 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's play:

int sof_create_platform_device(struct sof_platform_priv *priv)
{
	struct snd_sof_pdata *sof_pdata = priv->sof_pdata;
	struct device *dev = sof_pdata->dev;

	priv->pdev_pcm =
		platform_device_register_data(dev, "sof-audio",
					      PLATFORM_DEVID_NONE,
					      sof_pdata, sizeof(*sof_pdata));
	if (IS_ERR(priv->pdev_pcm)) {
		dev_err(dev, "error: cannot register device sof-audio. Error %ld\n",
			PTR_ERR(priv->pdev_pcm));
		return PTR_ERR(priv->pdev_pcm);
	}

	return 0;
}

this will result in sof_probe being called, so how can sof_probe() fail without the error being bubbled all the way to the pci-device?

@libinyang libinyang Jan 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled by device-driver binding methods in the lower level.

@plbossart plbossart Jan 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry man, I follow the code and don't see any evidence that your claims hold water. If you want to make your case you've got to be more convincing or prove me wrong.

@libinyang libinyang Jan 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you register a device, a corresponding driver probe will be called and vice versa. They are async. device is device, driver is driver : )
Image there is a HW platform device, we will register the device, and register the device driver. Even device driver probe failed, the device is also registered successfully. No reason that driver failure will cause device registering failure, right?


/* do nothing if dsp resume callbacks are not set */
if (!sof_ops(sdev)->resume || !sof_ops(sdev)->runtime_resume)
return 0;
Expand Down Expand Up @@ -302,6 +305,16 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
struct snd_sof_dev *sdev = dev_get_drvdata(&priv->pdev_pcm->dev);
int ret = 0;

/*
* If sdev is NULL, it meams the sof audio driver fails in
* initialization. Let's return 0 to let suspend/resume
* be successful. The audio system should already be powered down
* if the probe step if initialization fails. The same with resume()
* and prepare() callback.
*/
if (!sdev)
return 0;

/* do nothing if dsp suspend callback is not set */
if (!sof_ops(sdev)->suspend)
return 0;
Expand Down Expand Up @@ -379,6 +392,9 @@ int snd_sof_prepare(struct device *dev)
struct sof_platform_priv *priv = dev_get_drvdata(dev);
struct snd_sof_dev *sdev = dev_get_drvdata(&priv->pdev_pcm->dev);

if (!sdev)
return 0;

/*
* PCI devices are brought back to full power before system suspend.
* Setting this flag will prevent restoring kcontrols
Expand Down