Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1cb4e02f-f040-23bd-44d0-0675429332bd@linaro.org>
Date:   Tue, 22 Feb 2022 22:52:25 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 3/3] soundwire: qcom: add wake up interrupt support
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        robh+dt@kernel.org, vkoul@kernel.org,
        yung-chuan.liao@linux.intel.com
Cc:     devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, quic_srivasam@quicinc.com
References: <20220221104127.15670-1-srinivas.kandagatla@linaro.org>
 <20220221104127.15670-4-srinivas.kandagatla@linaro.org>
 <5e050d4c-e3d2-35fb-ca49-7be53579bc31@linux.intel.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <5e050d4c-e3d2-35fb-ca49-7be53579bc31@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 22/02/2022 19:26, Pierre-Louis Bossart wrote:
> 
> 
> 
>> +static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct qcom_swrm_ctrl *swrm = dev_id;
>> +	int ret = IRQ_HANDLED;
>> +	struct sdw_slave *slave;
>> +
>> +	clk_prepare_enable(swrm->hclk);
>> +
>> +	if (swrm->wake_irq > 0) {
>> +		if (!irqd_irq_disabled(irq_get_irq_data(swrm->wake_irq)))
>> +			disable_irq_nosync(swrm->wake_irq);
>> +	}
>> +
>> +	/*
>> +	 * resume all the slaves which must have potentially generated this
>> +	 * interrupt, this should also wake the controller at the same time.
>> +	 * this is much safer than waking controller directly that will deadlock!
>> +	 */
> There should be no difference if you first resume the controller and
> then attached peripherals, or do a loop where you rely on the pm_runtime
> framework.
> 
> The notion that there might be a dead-lock is surprising, you would need
> to elaborate here.Issue is, if wakeup interrupt resumes the controller first which can 
trigger an slave pending interrupt (ex: Button press event) in the 
middle of resume that will try to wake the slave device which in turn 
will try to wake parent in the middle of resume resulting in a dead lock.

This was the best way to avoid dead lock.

> 
>> +	list_for_each_entry(slave, &swrm->bus.slaves, node) {
>> +		ret = pm_runtime_get_sync(&slave->dev);
> 
> In my experience, you don't want to blur layers and take references on
> the child devices from the parent device. I don't know how many times we
> end-up with weird behavior.
> 
> we've done something similar on the Intel side but implemented in a less
> directive manner.
thanks, I can take some inspiration from that.


--srini
> 
> ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);
> 
> static int intel_resume_child_device(struct device *dev, void *data)
> {
> [...]	
> 	ret = pm_request_resume(dev);
> 	if (ret < 0)
> 		dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
> 
> 	return ret;
> }
> 
> 
>> +		if (ret < 0 && ret != -EACCES) {
>> +			dev_err_ratelimited(swrm->dev,
>> +					    "pm_runtime_get_sync failed in %s, ret %d\n",
>> +					    __func__, ret);
>> +			pm_runtime_put_noidle(&slave->dev);
>> +			ret = IRQ_NONE;
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	list_for_each_entry(slave, &swrm->bus.slaves, node) {
>> +		pm_runtime_mark_last_busy(&slave->dev);
>> +		pm_runtime_put_autosuspend(&slave->dev);
>> +	}
>> +err:
>> +	clk_disable_unprepare(swrm->hclk);
>> +	return IRQ_HANDLED;
>> +}
>> +
>