| 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
| ||
|
Message-ID: <a69aa5d0-613a-6ef6-9263-378b1e99770a@linux.intel.com>
Date: Tue, 30 Jun 2020 12:18:51 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Vinod Koul <vkoul@...nel.org>,
Bard Liao <yung-chuan.liao@...ux.intel.com>
Cc: alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
jank@...ence.com, srinivas.kandagatla@...aro.org,
rander.wang@...ux.intel.com, ranjani.sridharan@...ux.intel.com,
hui.wang@...onical.com, sanyog.r.kale@...el.com,
slawomir.blauciak@...el.com, mengdong.lin@...el.com,
bard.liao@...el.com
Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support
On 6/30/20 11:51 AM, Vinod Koul wrote:
> On 24-06-20, 01:35, Bard Liao wrote:
>> From: Rander Wang <rander.wang@...el.com>
>>
>> When system is suspended in clock stop mode on intel platforms, both
>> master and slave are in clock stop mode and soundwire bus is taken
>> over by a glue hardware. The bus message for jack event is processed
>> by this glue hardware, which will trigger an interrupt to resume audio
>> pci device. Then audio pci driver will resume soundwire master and slave,
>> transfer bus ownership to master, finally slave will report jack event
>> to master and codec driver is triggered to check jack status.
>>
>> if a slave has been attached to a bus, the slave->dev_num_sticky
>> should be non-zero, so we can check this value to skip the
>> ghost devices defined in ACPI table but not populated in hardware.
>>
>> Signed-off-by: Rander Wang <rander.wang@...el.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@...ux.intel.com>
>> ---
>> drivers/soundwire/intel.c | 48 +++++++++++++++++++++++++++++++++-
>> drivers/soundwire/intel.h | 1 +
>> drivers/soundwire/intel_init.c | 22 ++++++++++++++++
>> 3 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index 06c553d94890..22d9fd3e34fa 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -13,6 +13,7 @@
>> #include <linux/io.h>
>> #include <linux/platform_device.h>
>> #include <sound/pcm_params.h>
>> +#include <linux/pm_runtime.h>
>> #include <sound/soc.h>
>> #include <linux/soundwire/sdw_registers.h>
>> #include <linux/soundwire/sdw.h>
>> @@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
>> return ret;
>> }
>>
>> -static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>> +static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>
> why drop __maybe?
the __maybe was used in previous patches to avoid throwing 'defined but
not used' errors.
In this patch this function is actually used so there's no longer a
reason to keep it.
>> {
>> void __iomem *shim = sdw->link_res->shim;
>> unsigned int link_id = sdw->instance;
>> @@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +int intel_master_process_wakeen_event(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct sdw_intel *sdw;
>> + struct sdw_bus *bus;
>> + struct sdw_slave *slave;
>> + void __iomem *shim;
>> + u16 wake_sts;
>> +
>> + sdw = platform_get_drvdata(pdev);
>> + bus = &sdw->cdns.bus;
>> +
>> + if (bus->prop.hw_disabled) {
>> + dev_dbg(dev,
>> + "SoundWire master %d is disabled, ignoring\n",
>> + bus->link_id);
>
> single line pls
ok
>> + return 0;
>> + }
>> +
>> + shim = sdw->link_res->shim;
>> + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
>> +
>> + if (!(wake_sts & BIT(sdw->instance)))
>> + return 0;
>> +
>> + /* disable WAKEEN interrupt ASAP to prevent interrupt flood */
>> + intel_shim_wake(sdw, false);
>
> when & where is this enabled?
in follow-up patches where the clock-stop mode is enabled.
} else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET ||
!clock_stop_quirks) {
ret = sdw_cdns_clock_stop(cdns, true);
if (ret < 0) {
dev_err(dev, "cannot enable clock stop on suspend\n");
return ret;
}
ret = sdw_cdns_enable_interrupt(cdns, false);
if (ret < 0) {
dev_err(dev, "cannot disable interrupts on suspend\n");
return ret;
}
ret = intel_link_power_down(sdw);
if (ret) {
dev_err(dev, "Link power down failed: %d", ret);
return ret;
}
intel_shim_wake(sdw, true);
>> +
>> + /*
>> + * wake up master and slave so that slave can notify master
>> + * the wakeen event and let codec driver check codec status
>> + */
>> + list_for_each_entry(slave, &bus->slaves, node) {
>> + /*
>> + * discard devices that are defined in ACPI tables but
>> + * not physically present and devices that cannot
>> + * generate wakes
>> + */
>> + if (slave->dev_num_sticky && slave->prop.wake_capable)
>> + pm_request_resume(&slave->dev);
>
> Hmmm, shouldn't slave do this? would it not make sense to notify the
> slave thru callback and then slave decides to resume or not..?
In this mode, the bus is clock-stop mode, and events are detected with
level detector tied to PCI events. The master and slave devices are all
in pm_runtime suspended states. The codec cannot make any decisions on
its own since the bus is stopped, it needs to first resume, which
assumes that the master resumes first and the enumeration re-done before
it can access any of its registers.
By looping through the list of devices that can generate events, you
end-up first forcing the master to resume, and then each slave resumes
and can check who generated the event and what happened while suspended.
if the codec didn't generate the event it will go back to suspended mode
after the usual timeout.
We can add a callback but that callback would only be used for Intel
solutions, but internally it would only do a pm_request_resume() since
the codec cannot make any decisions before first resuming. In other
words, it would be an Intel-specific callback that is implemented using
generic resume operations. It's probably better to keep this in
Intel-specific code, no?
Powered by blists - more mailing lists