Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Fri, 18 Aug 2023 18:30:42 +0200,
Pierre-Louis Bossart wrote:
> 
> The code doesn't look too bad but needs a bit more work. There are quite
> a few error handling issues, pm_runtime needs to be revisited and
> ACPI/EFI as well.
> 
> > +enum calib_data {
> 
> tas2781_calib_data?

Well, as long as it's a local stuff, a suffix isn't really needed.
If it makes thing too confusing, it should be named properly, of
course, though.

> > +static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
> > +{
> > +	struct acpi_device *adev;
> > +	struct device *physdev;
> > +	LIST_HEAD(resources);
> > +	const char *sub;
> > +	int ret;
> > +
> > +	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
> > +	if (!adev) {
> > +		dev_err(p->dev,
> > +			"Failed to find an ACPI device for %s\n", hid);
> > +		return -ENODEV;
> > +	}
> 
> [1] need to take care of a resource leak here

Right, and that's rather a typo at the end of the function...

> > +err:
> > +	dev_err(p->dev, "read acpi error, ret: %d\n", ret);
> > +	put_device(physdev);

... this must be put_device(adev) instead physdev.

> > +static void tas2781_hda_playback_hook(struct device *dev, int action)
> > +{
> > +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
> > +	switch (action) {
> > +	case HDA_GEN_PCM_ACT_OPEN:
> > +		pm_runtime_get_sync(dev);
> 
> test if this actually works?

To be fair, most of driver codes don't check it, including the
HD-audio core.  (Actually, over 900 of 1300 calls have no check in the
whole tree.)

It implies that forcing the check in each place is moot; rather the
helper needs to be coded not to fail, IMO.

> > +static int tasdevice_hda_clamp(int val, int max)
> > +{
> > +	if (val > max)
> > +		val = max;
> > +
> > +	if (val < 0)
> > +		val = 0;
> > +	return val;
> > +}
> 
> I've seen that macro in the TAS2783 code as well, that sounds like a
> good helper function to share?

There is already clamp() macro, and I guess it can be replaced with
clamp(val, 0, max).

> > +
> > +	comps->dev = dev;
> > +
> > +	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
> > +
> > +	ret = tascodec_init(tas_priv, codec, tasdev_fw_ready);
> > +	if (ret)
> > +		return ret;
> 
> need to do a put_autosuspend below, this is leaking a refcount.

Right, that needs an obvious leak.  Let's fix it.

> > +static int tas2781_system_suspend(struct device *dev)
> > +{
> > +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	dev_dbg(tas_priv->dev, "System Suspend\n");
> > +
> > +	ret = pm_runtime_force_suspend(dev);
> > +	if (ret)
> > +		return ret;
> 
> that's usually the other way around, for system suspend you either want
> the device to be pm_runtime active, or if it's already suspended do nothing.
> 
> This is very odd to me.

This is a normal procedure, as stated in pm_runtime_force_suspend()
definition:

/**
 * pm_runtime_force_suspend - Force a device into suspend state if needed.
....
 * Typically this function may be invoked from a system suspend callback to make
 * sure the device is put into low power state and it should only be used during
 * system-wide PM transitions to sleep states.  It assumes that the analogous
 * pm_runtime_force_resume() will be used to resume the device.


thanks,

Takashi