linux-kernel - Re: [PATCH 17/19] soundwire: amd: add pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9edd31f6-1208-64fd-bdde-afb72699a1f5@linux.intel.com>
Date:   Thu, 12 Jan 2023 08:50:13 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     "Mukunda,Vijendar" <vijendar.mukunda@....com>, broonie@...nel.org,
        vkoul@...nel.org, alsa-devel@...a-project.org
Cc:     Basavaraj.Hiregoudar@....com, Sunil-kumar.Dommati@....com,
        Mario.Limonciello@....com, Mastan.Katragadda@....com,
        arungopal.kondaveeti@....com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 17/19] soundwire: amd: add pm_prepare callback and pm ops
 support


>>> +static int __maybe_unused amd_pm_prepare(struct device *dev)
>>> +{
>>> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>>> +	struct sdw_bus *bus = &ctrl->bus;
>>> +	int ret;
>>> +
>>> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
>>> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>>> +			bus->link_id);
>>> +		return 0;
>>> +	}
>>> +	ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device);
>>> +	if (ret < 0)
>>> +		dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret);
>>> +	if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>>> +		ret = pm_request_resume(dev);
>>> +		if (ret < 0) {
>>> +			dev_err(bus->dev, "pm_request_resume failed: %d\n", ret);
>>> +			return 0;
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>> This seems to be inspired by the Intel code, but is this necessary here?
> No It's not inspired by intel code. Initially, we haven't included
> pm_prepare callback. We have observed issues without
> pm_prepare callback.
>> For Intel, we saw cases where we had to pm_resume before doing a system
>> suspend, otherwise the hardware was in a bad state.
>>
>> Do you actually need to do so, or is is possible to do a system suspend
>> when the clock is stopped.
>>
>> And in the case where the bus is in 'power-off' mode, do you actually
>> need to resume at all?
> Our platform supports different power modes. To support all
> combinations, we have included pm_prepare callback.

>> do you actually need to stop the clock before powering-off? This seems
>> counter intuitive and not so useful?
> Yes, as per our design, we need to stop the clock
> before powering off.

It'd be good to add comments capturing these points, that would be
useful for new contributors and reviewers to know this is intentional
and required by the hardware programming sequences.