| 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: <718da090-bc58-0762-c901-cbbfc3b78d5f@huawei.com>
Date: Sat, 4 Mar 2023 17:47:28 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Sudeep Holla <sudeep.holla@....com>
CC: <robbiek@...ghtlabs.com>, <linux-acpi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <rafael@...nel.org>,
<rafael.j.wysocki@...el.com>, <wanghuiqiang@...wei.com>,
<zhangzekun11@...wei.com>, <wangxiongfeng2@...wei.com>,
<tanxiaofei@...wei.com>, <guohanjun@...wei.com>,
<xiexiuqi@...wei.com>, <wangkefeng.wang@...wei.com>,
<huangdaode@...wei.com>
Subject: Re: [PATCH 2/2] mailbox: pcc: Support shared interrupt for multiple
subspaces
在 2023/3/3 19:14, Sudeep Holla 写道:
> On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote:
>> Sorry for my resend. Because I found that my last reply email is not in the
>> thread of this patch. I guess it may be send failed.
>>
>> 在 2023/3/2 22:02, Sudeep Holla 写道:
>>> No. I meant a comment saying it is not need since only one transfer can occur
>>> at a time and mailbox takes care of locking. So chan_in_use can be accessed
>>> without a lock.
>> Got it. Agreed.
> Thanks
already modify this comment as below.
>
>>>> For types no need this flag, it is always hard to understand and redundant
>>>> design.
>>> But does it matter ? You can even support shared interrupt for type 1&2.
>> BTW, type 1 subspaces do not support a level triggered platform interrupt as
>> no method is provided to clear the interrupt.
> Agreed but there is no harm using the flag, you can add a comment that it is
> useful only if shared interrupts are supported. That will imply it is dummy
> for type 1. I am avoiding too many type unnecessary checks especially in IRQ
> handler.
Understood.
>
>>> They support level interrupt, so we can add them too. I understand you can
>>> test only type 3, but this driver caters for all and the code must be generic
>>> as much as possible. I don't see any point in check for type 3 only. Only
>> I understand what you do.
>> But type 2 also supports the communication flow from OSPM to Platfrom.
>> In this case, this flag will get in the way of type 2.
>>
> How ?
It should be ok if all types except for type 3 do not check this flag in
interrupt handle.
Namely, these types consider it as dummy, and do not use it, anywhere,
Right?
>
>> Whether the interrupt belongs to a type2 channel is only determined by
>> the status field in Generic Communications Channel Shared Memory Region,
>> which is done in rx_callback of PCC client.
> Agreed, but do you see any issue using the flag even if it acts as dummy ?
I think it can work well if these types completely ignore this flag, like below.
what do you think?
-->8
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index ecd54f049de3..14405e99193d 100755
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -92,6 +92,13 @@ struct pcc_chan_reg {
* @error: PCC register bundle for the error status register
* @plat_irq: platform interrupt
* @type: PCC subspace type
+ * @plat_irq_flags: platform interrupt flags
+ * @chan_in_use: this flag is used just to check if the interrupt needs
+ * handling when it is shared. Since only one transfer can
occur
+ * at a time and mailbox takes care of locking, this flag
can be
+ * accessed without a lock. Note: the type only support the
+ * communication from OSPM to Platform, like type3, use it, and
+ * other types completely ignore it.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -102,6 +109,8 @@ struct pcc_chan_info {
struct pcc_chan_reg error;
int plat_irq;
u8 type;
+ unsigned int plat_irq_flags;
+ bool chan_in_use;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -225,6 +234,12 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_chan_plat_irq_can_be_shared(struct pcc_chan_info *pchan)
+{
+ return (pchan->plat_irq_flags & ACPI_PCCT_INTERRUPT_MODE) ==
+ ACPI_LEVEL_SENSITIVE;
+}
+
static bool pcc_chan_command_complete(struct pcc_chan_info *pchan,
u64 cmd_complete_reg_val)
{
@@ -277,6 +292,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
+ !pchan->chan_in_use)
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
@@ -302,9 +320,12 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
/*
* The PCC slave subspace channel needs to set the command
complete bit
* and ring doorbell after processing message.
+ *
+ * The PCC master subspace channel clears chan_in_use to free
channel.
*/
if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
pcc_send_data(chan, NULL);
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -353,10 +374,13 @@ pcc_mbox_request_channel(struct mbox_client *cl,
int subspace_id)
spin_unlock_irqrestore(&chan->lock, flags);
if (pchan->plat_irq > 0) {
+ unsigned long irqflags;
int rc;
- rc = devm_request_irq(dev, pchan->plat_irq, pcc_mbox_irq, 0,
- MBOX_IRQ_NAME, chan);
+ irqflags = pcc_chan_plat_irq_can_be_shared(pchan) ?
+ IRQF_SHARED | IRQF_ONESHOT : 0;
+ rc = devm_request_irq(dev, pchan->plat_irq, pcc_mbox_irq,
+ irqflags, MBOX_IRQ_NAME, chan);
if (unlikely(rc)) {
dev_err(dev, "failed to register PCC interrupt
%d\n",
pchan->plat_irq);
@@ -418,7 +442,11 @@ static int pcc_send_data(struct mbox_chan *chan,
void *data)
if (ret)
return ret;
- return pcc_chan_reg_read_modify_write(&pchan->db);
+ ret = pcc_chan_reg_read_modify_write(&pchan->db);
+ if (!ret && pchan->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -501,6 +529,7 @@ static int pcc_parse_subspace_irq(struct
pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
return -EINVAL;
}
+ pchan->plat_irq_flags = pcct_ss->flags;
if (pcct_ss->header.type ==
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void
*)pcct_ss;
@@ -522,6 +551,12 @@ static int pcc_parse_subspace_irq(struct
pcc_chan_info *pchan,
"PLAT IRQ ACK");
}
+ if (pcc_chan_plat_irq_can_be_shared(pchan) &&
+ !pchan->plat_irq_ack.gas) {
+ pr_err("PCC subspace has level IRQ with no ACK register\n");
+ return -EINVAL;
+ }
+
return ret;
}
Powered by blists - more mailing lists