linux-kernel - Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ca35058d-1f40-3f85-9e2d-bfb29c8625cb@xsightlabs.com>
Date:   Fri, 4 Nov 2022 11:39:52 -0400
From:   Robbie King <robbiek@...ghtlabs.com>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     "lihuisong (C)" <lihuisong@...wei.com>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, rafael@...nel.org,
        rafael.j.wysocki@...el.com, wanghuiqiang@...wei.com,
        huangdaode@...wei.com, tanxiaofei@...wei.com
Subject: Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces

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.

> 
>> I think the RDN2 may provide an example of a write only interrupt
>> acknowledge mechanism mentioned by Sudeep.
>>
> 
> Yes.
> 
>> The RDN2 reference design uses the MHUv2 IP for the doorbell mechanism.  If
>> my implementation is correct (and it quite possibly is not), acknowledging
>> the DB interrupt from the platform is accomplished by writing a 1 to the
>> appropriate bit in the receiver channel window CH_CLR register, which is
>> documented as:
>>
>>    Channel flag clear.
>>    Write 0b1 to a bit clears the corresponding bit in the CH_ST and CH_ST_MSK.
>>    Writing 0b0 has no effect.
>>    Each bit always reads as 0b0.
>>
> 
> Correct, on this MHUv[1-2], it is write only register and it reads zero.
> So basically you will ignore the interrupt if we apply the logic Huisong
> proposed initially.
> 
>> in the "Arm Corstone SSE-700 Subsystem Technical Reference Manual".
>>
>> Apologies if I am off in the weeds here as I have only been working with
>> PCC/SCMI for a very short period of time.
> 
> Good to know info :).
> 

It helps that your linux / firmware code is easy to follow! :)

One other minor issue I encountered was that a NULL GAS (all zeros) doesn't
seem to be supported by pcc_chan_reg_init, may be a good opportunity for me
to submit my first RFC...

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index ed18936b8ce6..3fa7335d15b0 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -100,6 +100,7 @@ struct pcc_chan_info {
         struct pcc_chan_reg cmd_update;
         struct pcc_chan_reg error;
         int plat_irq;
+       unsigned int plat_irq_flags;
  };

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index ed18936b8ce6..3fa7335d15b0 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -100,6 +100,7 @@ struct pcc_chan_info {
         struct pcc_chan_reg cmd_update;
         struct pcc_chan_reg error;
         int plat_irq;
+       unsigned int plat_irq_flags;
  };

  #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -221,6 +222,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;
+}
+
  /**
   * pcc_mbox_irq - PCC mailbox interrupt handler
   * @irq:       interrupt number
@@ -310,9 +317,12 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)

         if (pchan->plat_irq > 0) {
                 int rc;
+               unsigned long irqflags;

-               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);
@@ -458,6 +468,8 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
                 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;