From: Robbie King <robbiek@xsightlabs.com>
To: "lihuisong (C)" <lihuisong@huawei.com>,
Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
rafael@kernel.org, rafael.j.wysocki@intel.com,
wanghuiqiang@huawei.com, huangdaode@huawei.com,
tanxiaofei@huawei.com
Subject: Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces
Date: Mon, 21 Nov 2022 11:59:17 -0500 [thread overview]
Message-ID: <a7119a55-9e7a-b27f-4969-2c6bef764011@xsightlabs.com> (raw)
In-Reply-To: <1156ef89-20a4-7e7e-6205-c68e21a9bb36@huawei.com>
On 11/19/2022 2:32 AM, lihuisong (C) wrote:
>
> 在 2022/11/18 2:09, Robbie King 写道:
>> On 11/7/2022 1:24 AM, lihuisong (C) wrote:
>>>
>>> 在 2022/11/4 23:39, Robbie King 写道:
>>>> On 11/4/2022 11:15 AM, Sudeep Holla wrote:
>>>>> On Fri, Nov 04, 2022 at 11:04:22AM -0400, Robbie King wrote:
>>>>>> Hello Huisong, your raising of the shared interrupt issue is very timely, I
>>>>>> am working to implement "Extended PCC subspaces (types 3 and 4)" using PCC
>>>>>> on the ARM RDN2 reference platform as a proof of concept, and encountered
>>>>>> this issue as well. FWIW, I am currently testing using Sudeep's patch with
>>>>>> the "chan_in_use" flag removed, and so far have not encountered any issues.
>>>>>>
>>>>>
>>>>> Interesting, do you mean the patch I post in this thread but without the
>>>>> whole chan_in_use flag ?
>>>>
>>>> That's right, diff I'm running with is attached to end of message.
>>> Hello Robbie, In multiple subspaces scenario, there is a problem
>>> that OS doesn't know which channel should respond to the interrupt
>>> if no this chan_in_use flag. If you have not not encountered any
>>> issues in this case, it may be related to your register settings.
>>>
>>
>> Hi Huisong, apologies, I see your point now concerning multiple subspaces.
>>
>> I have started stress testing where I continuously generate both requests
>> and notifications as quickly as possible, and unfortunately found an issue
>> even with the original chan_in_use patch. I first had to modify the patch
>> to get the type 4 channel notifications to function at all, essentially
>> ignoring the chan_in_use flag for that channel. With that change, I still
>> hit my original stress issue, where the pcc_mbox_irq function did not
>> correctly ignore an interrupt for the type 3 channel.
>>
>> The issue occurs when a request from AP to SCP over the type 3 channel is
>> outstanding, and simultaneously the SCP initiates a notification over the
>> type 4 channel. Since the two channels share an interrupt, both handlers
>> are invoked.
>>
>> I've tried to draw out the state of the channel status "free" bits along
>> with the AP and SCP function calls involved.
>>
>> type 3
>> ------
>>
>> (1)pcc.c:pcc_send_data()
>> | (5) mailbox.c:mbox_chan_receive_data()
>> _______v (4)pcc.c:pcc_mbox_irq()
>> free \_________________________________________
>>
>> ^
>> type 4 ^
>> ------ ^
>> _____________________
>> free \_____________________________
>> ^ ^
>> | |
>> (2)mod_smt.c:smt_transmit() |
>> |
>> (3)mod_mhu2.c:raise_interrupt()
>>
>> The sequence of events are:
>>
>> 1) OS initiates request to SCP by clearing FREE in status and ringing SCP doorbell
>> 2) SCP initiates notification by filling shared memory and clearing FREE in status
>> 3) SCP notifies OS by ringing OS doorbell
>> 4) OS first invokes interrupt handler for type 3 channel
>>
>> At this step, the issue is that "val" from reading status (i.e. CommandCompleteCheck)
>> is zero (SCP has not responded yet) so the code below falls through and continues
>> to processes the interrupt as if the request has been acknowledged by the SCP.
>>
>> if (val) { /* Ensure GAS exists and value is non-zero */
>> val &= pchan->cmd_complete.status_mask;
>> if (!val)
>> return IRQ_NONE;
>> }
>>
>> The chan_in_use flag does not address this because the channel is indeed in use.
>>
>> 5) ACPI:PCC client kernel module is incorrectly notified that response data is
>> available
> Indeed, chan_in_use flag is invalid for type4.
Thinking about this some more, I believe there is a need for the chan_in_use flag
for the type 4 channels. If there are multiple SCP to AP channels sharing an
interrupt, and the PCC client code chooses to acknowledge the transfer from
process level (i.e. call mbox_send outside of the mbox_chan_received_data callback),
then I believe a window exists where the callback could be invoked twice for the
same SCP to AP channel. I've attached a diff.
>> I added the following fix (applied on top of Sudeep's original patch for clarity)
>> for the issue above which solved the stress test issue. I've changed the interrupt
>> handler to explicitly verify that the status value matches the mask for type 3
>> interrupts before acknowledging them. Conversely, a type 4 channel verifies that
>> the status value does *not* match the mask, since in this case we are functioning
>> as the recipient, not the initiator.
>>
>> One concern is that since this fundamentally changes handling of the channel status,
>> that existing platforms could be impacted.
> [snip]
>>
>> + /*
>> + * When we control data flow on the channel, we expect
>> + * to see the mask bit(s) set by the remote to indicate
>> + * the presence of a valid response. When we do not
>> + * control the flow (i.e. type 4) the opposite is true.
>> + */
>> + if (pchan->is_controller)
>> + cmp = pchan->cmd_complete.status_mask;
>> + else
>> + cmp = 0;
>> +
>> + val &= pchan->cmd_complete.status_mask;
>> + if (cmp != val)
>> + return IRQ_NONE;
>>
> We don't have to use the pchan->cmd_complete.status_mask as above.
>
> For the communication from AP to SCP, if this channel is in use, command
> complete bit is 1 indicates that the command being executed has been
> completed.
> For the communication from SCP to AP, if this channel is in use, command
> complete bit is 0 indicates that the bit has been cleared and OS should
> response the interrupt.
>
> So you concern should be gone if we do as following approach:
> "
> val &= pchan->cmd_complete.status_mask;
> need_rsp_irq = pchan->is_controller ? val != 0 : val == 0;
> if (!need_rsp_irq)
> return IRQ_NONE
> "
>
> This may depend on the default value of the command complete register
> for each channel(must be 1, means that the channel is free for use).
> It is ok for type3 because of chan_in_use flag, while something needs
> to do in platform or OS for type4.
>> ret = pcc_chan_reg_read(&pchan->error, &val);
>> if (ret)
>> @@ -704,6 +717,9 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>> pcc_mbox_channels[i].con_priv = pchan;
>> pchan->chan.mchan = &pcc_mbox_channels[i];
>>
>> + pchan->is_controller =
>> + (pcct_entry->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE);
>> +
> This definition does not apply to all types because type1 and type2
> support bidirectional communication.
>
>> if (pcct_entry->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
>> !pcc_mbox_ctrl->txdone_irq) {
>> pr_err("Plaform Interrupt flag must be set to 1");
>>
>
> I put all points we discussed into the following modifcation.
> Robbie, can you try it again for type 4 and see what else needs to be
> done?
>
> Regards,
> Huisong
>
Thanks Huisong, I ran my current stress test scenario against your diff
with no issues (I did have to manually merge due to a tabs to spaces issue
which may be totally on my end, still investigating).
Here is the proposed change to support chan_in_use for type 4 (which I've
also successfully tested with). I think I have solved the tabs to spaces
issue for my sent messages, apologies if that's not the case.
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 057e00ee83c8..d4fcc913a9a8 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -292,7 +292,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
- if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
+ if (!pchan->chan_in_use)
return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
@@ -320,8 +320,16 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
goto out;
}
+ /*
+ * Clearing in_use before RX callback allows calls to mbox_send
+ * (which sets in_use) within the callback so SCP to AP channels
+ * can acknowledge transfer within IRQ context
+ */
+ if (pchan->cmd_complete.gas)
+ pchan->chan_in_use = false;
+
mbox_chan_received_data(chan, NULL);
- rc = IRQ_HANDLED;
+ return IRQ_HANDLED;
out:
if (pchan->cmd_complete.gas)
@@ -772,6 +780,8 @@ static int pcc_mbox_probe(struct platform_device *pdev)
goto err;
}
pcc_set_chan_mesg_dir(pchan, pcct_entry->type);
+ if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
+ pchan->chan_in_use = true;
if (pcc_mbox_ctrl->txdone_irq) {
rc = pcc_parse_subspace_irq(pchan, pcct_entry);
next prev parent reply other threads:[~2022-11-21 16:59 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-16 3:40 [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces Huisong Li
2022-10-26 6:10 ` lihuisong (C)
2022-10-27 11:10 ` Sudeep Holla
2022-10-27 12:48 ` lihuisong (C)
2022-10-27 15:53 ` Sudeep Holla
2022-10-28 7:55 ` lihuisong (C)
2022-10-31 10:40 ` Sudeep Holla
2022-11-01 2:49 ` lihuisong (C)
2022-11-04 15:04 ` Robbie King
2022-11-04 15:15 ` Sudeep Holla
2022-11-04 15:39 ` Robbie King
2022-11-07 6:24 ` lihuisong (C)
2022-11-17 18:09 ` Robbie King
2022-11-19 7:32 ` lihuisong (C)
2022-11-21 16:59 ` Robbie King [this message]
2022-11-22 3:42 ` lihuisong (C)
2022-11-22 3:30 ` [RFC V2] " Huisong Li
2022-11-22 13:46 ` Sudeep Holla
2022-11-23 3:36 ` lihuisong (C)
2022-12-03 9:51 ` [RFC-V3 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt Huisong Li
2022-12-03 9:51 ` [RFC-V3 1/2] mailbox: pcc: Add processing platform notification for slave subspaces Huisong Li
2023-02-06 15:39 ` Sudeep Holla
2023-02-07 2:27 ` lihuisong (C)
2023-02-13 21:18 ` Robbie King
2023-02-14 1:24 ` lihuisong (C)
2022-12-03 9:51 ` [RFC-V3 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2022-12-14 2:57 ` [RFC-V3 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt lihuisong (C)
2022-12-30 1:07 ` lihuisong (C)
2023-01-04 11:06 ` Sudeep Holla
2023-01-05 1:14 ` lihuisong (C)
2023-01-28 1:49 ` lihuisong (C)
2023-02-16 6:36 ` [PATCH " Huisong Li
2023-02-16 6:36 ` [PATCH 1/2] mailbox: pcc: Add processing platform notification for slave subspaces Huisong Li
2023-03-01 13:24 ` Sudeep Holla
2023-03-02 1:57 ` lihuisong (C)
2023-03-02 13:52 ` Sudeep Holla
2023-03-03 1:50 ` lihuisong (C)
2023-03-03 11:07 ` Sudeep Holla
2023-03-04 9:49 ` lihuisong (C)
2023-02-16 6:36 ` [PATCH 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2023-03-01 13:36 ` Sudeep Holla
2023-03-02 2:17 ` lihuisong (C)
2023-03-02 14:02 ` Sudeep Holla
[not found] ` <020cc964-9938-7ebe-7514-125cd041bfcb@huawei.com>
2023-03-03 11:14 ` Sudeep Holla
2023-03-04 9:47 ` lihuisong (C)
2023-03-10 20:14 ` Sudeep Holla
2023-03-14 1:05 ` lihuisong (C)
2023-02-27 13:09 ` [PATCH 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt lihuisong (C)
2023-03-14 11:11 ` [PATCH v2 " Huisong Li
2023-03-14 11:11 ` [PATCH v2 1/2] mailbox: pcc: Add support for platform notification handling Huisong Li
2023-03-27 11:30 ` Sudeep Holla
2023-03-27 12:25 ` lihuisong (C)
2023-03-27 13:27 ` Sudeep Holla
2023-03-14 11:11 ` [PATCH v2 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2023-03-24 2:30 ` [PATCH v2 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt lihuisong (C)
2023-03-27 11:33 ` Sudeep Holla
2023-03-27 12:31 ` lihuisong (C)
2023-04-10 1:26 ` lihuisong (C)
2023-04-11 14:47 ` Robbie King
2023-04-14 1:05 ` lihuisong (C)
2023-04-14 13:48 ` Robbie King
2023-04-18 2:21 ` lihuisong (C)
2023-04-21 10:55 ` Sudeep Holla
2023-04-23 3:58 ` lihuisong (C)
2023-04-25 13:22 ` lihuisong (C)
2023-04-23 11:03 ` [PATCH v3 " Huisong Li
2023-04-23 11:03 ` [PATCH v3 1/2] mailbox: pcc: Add support for platform notification handling Huisong Li
2023-04-23 11:03 ` [PATCH v3 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2023-04-25 14:41 ` [PATCH v3 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt Sudeep Holla
2023-05-25 12:25 ` lihuisong (C)
2023-05-04 1:30 ` lihuisong (C)
2023-05-09 9:06 ` Sudeep Holla
2023-06-13 12:57 ` [PATCH v4 " Huisong Li
2023-06-13 12:57 ` [PATCH v4 1/2] mailbox: pcc: Add support for platform notification handling Huisong Li
2023-06-13 12:57 ` [PATCH v4 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2023-06-14 15:58 ` [PATCH v4 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt Sudeep Holla
2023-07-14 6:39 ` lihuisong (C)
2023-07-21 12:31 ` lihuisong (C)
2023-06-15 1:58 ` Hanjun Guo
2023-08-01 6:38 ` [PATCH RESEND " Huisong Li
2023-08-01 6:38 ` [PATCH RESEND v4 1/2] mailbox: pcc: Add support for platform notification handling Huisong Li
2023-08-01 6:38 ` [PATCH RESEND v4 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces Huisong Li
2023-08-01 9:38 ` [PATCH RESEND v4 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt Sudeep Holla
2023-08-09 11:44 ` lihuisong (C)
2023-08-17 6:50 ` lihuisong (C)
2023-08-26 2:40 ` lihuisong (C)
[not found] ` <AS8P193MB233566815E722D9B3B13B5EECAE2A@AS8P193MB2335.EURP193.PROD.OUTLOOK.COM>
2023-08-28 1:03 ` lihuisong (C)
2023-08-29 9:55 ` Sudeep Holla
2023-09-21 13:52 ` Sudeep Holla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a7119a55-9e7a-b27f-4969-2c6bef764011@xsightlabs.com \
--to=robbiek@xsightlabs.com \
--cc=huangdaode@huawei.com \
--cc=lihuisong@huawei.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=rafael@kernel.org \
--cc=sudeep.holla@arm.com \
--cc=tanxiaofei@huawei.com \
--cc=wanghuiqiang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).