[PATCH 0/4] Add smp2p retrigger support

[PATCH 0/4] Add smp2p retrigger support

[Chris Lew]

The remoteproc framework keeps interrupts disabled until it powers on a
remote proc to prevent spurious interrupts. There is a case where the
remote proc can finish booting before remoteproc enables the interrupt.
If this happens, the remoteproc framework will miss the notification
and eventually timeout waiting for the remoteproc to finish "booting".

Add support into smp2p to retrigger an interrupt if it was missed while
the interrupt was disabled. The interrupt should retrigger once the
interrupt is enabled. This will make the bootup sequence for remoteproc
less racy.

Chris Lew (3):
  soc: qcom: smp2p: Introduce pending state for virtual irq
  soc: qcom: smp2p: Add proper retrigger detection
  soc: qcom: smp2p: Add memory barrier for irq_pending

Tao Zhang (1):
  soc: qcom: smp2p: Add remote_id into irq name

 drivers/soc/qcom/smp2p.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/4] soc: qcom: smp2p: Introduce pending state for virtual irq

[Chris Lew]

If a smp2p change occurs while a virtual interrupt is disabled, smp2p
should be able to resend that interrupt on enablement.

This functionality requires the CONFIG_HARDIRQS_SW_RESEND to be enabled
to reschedule the interrupts. To ensure the mask and unmask functions
are called during enabled and disable, set the flag to disable lazy
IRQ state handling (IRQ_DISABLE_UNLAZY).

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/soc/qcom/smp2p.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 59dbf4b61e6c..1c3259fe98be 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -101,6 +101,7 @@ struct smp2p_entry {
 
 	struct irq_domain *domain;
 	DECLARE_BITMAP(irq_enabled, 32);
+	DECLARE_BITMAP(irq_pending, 32);
 	DECLARE_BITMAP(irq_rising, 32);
 	DECLARE_BITMAP(irq_falling, 32);
 
@@ -146,6 +147,7 @@ struct qcom_smp2p {
 	unsigned local_pid;
 	unsigned remote_pid;
 
+	int irq;
 	struct regmap *ipc_regmap;
 	int ipc_offset;
 	int ipc_bit;
@@ -217,8 +219,8 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 {
 	struct smp2p_smem_item *in;
 	struct smp2p_entry *entry;
+	unsigned long status;
 	int irq_pin;
-	u32 status;
 	char buf[SMP2P_MAX_ENTRY_NAME];
 	u32 val;
 	int i;
@@ -247,19 +249,22 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 
 		status = val ^ entry->last_value;
 		entry->last_value = val;
+		status |= *entry->irq_pending;
 
 		/* No changes of this entry? */
 		if (!status)
 			continue;
 
-		for_each_set_bit(i, entry->irq_enabled, 32) {
-			if (!(status & BIT(i)))
-				continue;
-
+		for_each_set_bit(i, &status, 32) {
 			if ((val & BIT(i) && test_bit(i, entry->irq_rising)) ||
 			    (!(val & BIT(i)) && test_bit(i, entry->irq_falling))) {
 				irq_pin = irq_find_mapping(entry->domain, i);
 				handle_nested_irq(irq_pin);
+
+				if (test_bit(i, entry->irq_enabled))
+					clear_bit(i, entry->irq_pending);
+				else
+					set_bit(i, entry->irq_pending);
 			}
 		}
 	}
@@ -365,6 +370,8 @@ static int smp2p_irq_map(struct irq_domain *d,
 	irq_set_chip_data(irq, entry);
 	irq_set_nested_thread(irq, 1);
 	irq_set_noprobe(irq);
+	irq_set_parent(irq, entry->smp2p->irq);
+	irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY);
 
 	return 0;
 }
@@ -609,6 +616,7 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 	/* Kick the outgoing edge after allocating entries */
 	qcom_smp2p_kick(smp2p);
 
+	smp2p->irq = irq;
 	ret = devm_request_threaded_irq(&pdev->dev, irq,
 					NULL, qcom_smp2p_intr,
 					IRQF_ONESHOT,
-- 
2.7.4

[PATCH 2/4] soc: qcom: smp2p: Add proper retrigger detection

[Chris Lew]

Currently, smp2p relies on the hwirq resend feature to retrigger irqs
that were missed because the irq was disabled at the time of receiving
it. The hwirq resend feature will retrigger the parent smp2p interrupt.
In order to keep track of what children needed to be retriggered, the
pending bitmap was added.

After calling handle_nested_irq, smp2p checks if the interrupt is
enabled and sets the pending bit if the interrupt is not enabled. There
is a small window where a client can enable the interrupt between
calling handle_nested_irq and checking if the interrupt is enabled.
If this happens, the interrupt is never called when the parent smp2p
interrupt is retriggered.

Add the irq_retrigger callback so smp2p can know which child interrupts
need to be retriggered. Set the pending bits accordingly.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/soc/qcom/smp2p.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 1c3259fe98be..a94cddcb0298 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -260,11 +260,7 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 			    (!(val & BIT(i)) && test_bit(i, entry->irq_falling))) {
 				irq_pin = irq_find_mapping(entry->domain, i);
 				handle_nested_irq(irq_pin);
-
-				if (test_bit(i, entry->irq_enabled))
-					clear_bit(i, entry->irq_pending);
-				else
-					set_bit(i, entry->irq_pending);
+				clear_bit(i, entry->irq_pending);
 			}
 		}
 	}
@@ -353,11 +349,22 @@ static int smp2p_set_irq_type(struct irq_data *irqd, unsigned int type)
 	return 0;
 }
 
+static int smp2p_retrigger_irq(struct irq_data *irqd)
+{
+	struct smp2p_entry *entry = irq_data_get_irq_chip_data(irqd);
+	irq_hw_number_t irq = irqd_to_hwirq(irqd);
+
+	set_bit(irq, entry->irq_pending);
+
+	return 0;
+}
+
 static struct irq_chip smp2p_irq_chip = {
 	.name           = "smp2p",
 	.irq_mask       = smp2p_mask_irq,
 	.irq_unmask     = smp2p_unmask_irq,
 	.irq_set_type	= smp2p_set_irq_type,
+	.irq_retrigger	= smp2p_retrigger_irq,
 };
 
 static int smp2p_irq_map(struct irq_domain *d,
-- 
2.7.4

[PATCH 3/4] soc: qcom: smp2p: Add memory barrier for irq_pending

[Chris Lew]

There is a very tight race where the irq_retrigger function is run
on one cpu and the actual retrigger softirq is running on a second
cpu. When this happens, there may be a chance that the second cpu
will not see the updated irq_pending value from first cpu.

Add a memory barrier to ensure that irq_pending is read correctly.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/soc/qcom/smp2p.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a94cddcb0298..a1ea5f55c228 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -249,6 +249,9 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 
 		status = val ^ entry->last_value;
 		entry->last_value = val;
+
+		/* Ensure irq_pending is read correctly */
+		mb();
 		status |= *entry->irq_pending;
 
 		/* No changes of this entry? */
@@ -356,6 +359,11 @@ static int smp2p_retrigger_irq(struct irq_data *irqd)
 
 	set_bit(irq, entry->irq_pending);
 
+	/* Ensure irq_pending is visible to all cpus that retried interrupt
+	 * can run on
+	 */
+	mb();
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH 4/4] soc: qcom: smp2p: Add remote_id into irq name

[Chris Lew]

From: Tao Zhang <quic_taozhan@quicinc.com>

Changed smp2p irq devname from "smp2p" to "smp2p_<remote_id>", which
makes the wakeup source distinguishable in irq wakeup prints.

Signed-off-by: Tao Zhang <quic_taozhan@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/soc/qcom/smp2p.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a1ea5f55c228..21a0e84b16f4 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -541,6 +541,7 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 	struct device_node *node;
 	struct qcom_smp2p *smp2p;
 	const char *key;
+	char *name;
 	int irq;
 	int ret;
 
@@ -632,10 +633,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 	qcom_smp2p_kick(smp2p);
 
 	smp2p->irq = irq;
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "smp2p_%d",
+			      smp2p->remote_pid);
+	if (!name) {
+		ret = -ENOMEM;
+		goto unwind_interfaces;
+	}
 	ret = devm_request_threaded_irq(&pdev->dev, irq,
 					NULL, qcom_smp2p_intr,
 					IRQF_ONESHOT,
-					"smp2p", (void *)smp2p);
+					name, (void *)smp2p);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request interrupt\n");
 		goto unwind_interfaces;
-- 
2.7.4

Re: [PATCH 3/4] soc: qcom: smp2p: Add memory barrier for irq_pending

[Manivannan Sadhasivam]

On Tue, Jul 05, 2022 at 11:02:10PM -0700, Chris Lew wrote:
> There is a very tight race where the irq_retrigger function is run
> on one cpu and the actual retrigger softirq is running on a second
> cpu. When this happens, there may be a chance that the second cpu
> will not see the updated irq_pending value from first cpu.
> 
> Add a memory barrier to ensure that irq_pending is read correctly.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>  drivers/soc/qcom/smp2p.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index a94cddcb0298..a1ea5f55c228 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -249,6 +249,9 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>  
>  		status = val ^ entry->last_value;
>  		entry->last_value = val;
> +
> +		/* Ensure irq_pending is read correctly */
> +		mb();

I don't quite understand why you need a barrier here. mb() makes sure all the
prior instructions gets executed before executing the later one. But why is it
needed here?

>  		status |= *entry->irq_pending;
>  
>  		/* No changes of this entry? */
> @@ -356,6 +359,11 @@ static int smp2p_retrigger_irq(struct irq_data *irqd)
>  
>  	set_bit(irq, entry->irq_pending);
>  
> +	/* Ensure irq_pending is visible to all cpus that retried interrupt
> +	 * can run on
> +	 */
> +	mb();
> +

Here it makes sense because you want the CPU to set irq_pending before exiting
from this function. But even then you can use the less strict smp_wmb() that
serves the exact purpose.

Thanks,
Mani

>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/4] soc: qcom: smp2p: Introduce pending state for virtual irq

[Doug Anderson]

Hi,

I'm jumping in half-blind here since I've never looked at this driver
before, so if I say something that sounds wrong then it probably is.
;-P At the very least I'm super thankful for the "summary" kernel doc
at the start of this file (thanks Bjorn!)...

On Tue, Jul 5, 2022 at 11:03 PM Chris Lew <quic_clew@quicinc.com> wrote:
>
> If a smp2p change occurs while a virtual interrupt is disabled, smp2p
> should be able to resend that interrupt on enablement.

That sounds right. I made some attempt to document how I thought this
worked in commit cf9d052aa600 ("pinctrl: qcom: Don't clear pending
interrupts when enabling").


> This functionality requires the CONFIG_HARDIRQS_SW_RESEND to be enabled
> to reschedule the interrupts.

Maybe mention that you don't need to select this yourself because it's
already selected by the arm64 config?


> To ensure the mask and unmask functions
> are called during enabled and disable, set the flag to disable lazy
> IRQ state handling (IRQ_DISABLE_UNLAZY).

From your description of the problem it actually feels like you want
the opposite? When an interrupt is masked lazily then the interrupt
can still fire but won't make it to the client. Then as soon as the
client unmasks the interrupt will fire right away. That's exactly what
you want, isn't it?

Are you certain you need IRQ_DISABLE_UNLAZY?


> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>  drivers/soc/qcom/smp2p.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 59dbf4b61e6c..1c3259fe98be 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -101,6 +101,7 @@ struct smp2p_entry {
>
>         struct irq_domain *domain;
>         DECLARE_BITMAP(irq_enabled, 32);
> +       DECLARE_BITMAP(irq_pending, 32);

You'll get a kernel-doc warning since the rest of the elements in this
structure are documented but your new one isn't...

...in this structure and others you're modifying in this patch...


>         DECLARE_BITMAP(irq_rising, 32);
>         DECLARE_BITMAP(irq_falling, 32);
>
> @@ -146,6 +147,7 @@ struct qcom_smp2p {
>         unsigned local_pid;
>         unsigned remote_pid;
>
> +       int irq;
>         struct regmap *ipc_regmap;
>         int ipc_offset;
>         int ipc_bit;
> @@ -217,8 +219,8 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>  {
>         struct smp2p_smem_item *in;
>         struct smp2p_entry *entry;
> +       unsigned long status;
>         int irq_pin;
> -       u32 status;
>         char buf[SMP2P_MAX_ENTRY_NAME];
>         u32 val;
>         int i;
> @@ -247,19 +249,22 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>
>                 status = val ^ entry->last_value;
>                 entry->last_value = val;
> +               status |= *entry->irq_pending;
>
>                 /* No changes of this entry? */
>                 if (!status)
>                         continue;
>
> -               for_each_set_bit(i, entry->irq_enabled, 32) {
> -                       if (!(status & BIT(i)))
> -                               continue;
> -
> +               for_each_set_bit(i, &status, 32) {
>                         if ((val & BIT(i) && test_bit(i, entry->irq_rising)) ||
>                             (!(val & BIT(i)) && test_bit(i, entry->irq_falling))) {
>                                 irq_pin = irq_find_mapping(entry->domain, i);
>                                 handle_nested_irq(irq_pin);
> +
> +                               if (test_bit(i, entry->irq_enabled))
> +                                       clear_bit(i, entry->irq_pending);
> +                               else
> +                                       set_bit(i, entry->irq_pending);

Your new logic seems a bit off to me.

For one thing, once an IRQ becomes pending it should stay pending.
With your code I believe that if an IRQ was marked as "rising" and
then it went high and low again while masked that you would lose track
of the interrupt. Normally an edge-triggered interrupt will latch the
edge and keep track of it.

I think the logic should be something like this (untested):

status = val ^ entry->last_value;
entry->last_value = val;

/* IRQs become pending regardless of whether they're masked or not */
for_each_set_bit(i, &status, 32) {
        if ((val & BIT(i) && test_bit(i, entry->irq_rising)) ||
            (!(val & BIT(i)) && test_bit(i, entry->irq_falling)))
                set_bit(i, entry->irq_pending);
}

/* Handled unmasked (AKA enabled) pending IRQs */
for_each_set_bit(i, entry->irq_pending), 32) {
        if (!test_bit(i, entry->irq_enabled))
                continue;
        clear_bit(i, entry->irq_pending);

        irq_pin = irq_find_mapping(entry->domain, i);
        handle_nested_irq(irq_pin);
}

Re: [PATCH 3/4] soc: qcom: smp2p: Add memory barrier for irq_pending

[Doug Anderson]

Hi,

On Tue, Jul 5, 2022 at 11:03 PM Chris Lew <quic_clew@quicinc.com> wrote:
>
> There is a very tight race where the irq_retrigger function is run
> on one cpu and the actual retrigger softirq is running on a second
> cpu. When this happens, there may be a chance that the second cpu
> will not see the updated irq_pending value from first cpu.
>
> Add a memory barrier to ensure that irq_pending is read correctly.
>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>  drivers/soc/qcom/smp2p.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index a94cddcb0298..a1ea5f55c228 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -249,6 +249,9 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>
>                 status = val ^ entry->last_value;
>                 entry->last_value = val;
> +
> +               /* Ensure irq_pending is read correctly */
> +               mb();
>                 status |= *entry->irq_pending;
>
>                 /* No changes of this entry? */
> @@ -356,6 +359,11 @@ static int smp2p_retrigger_irq(struct irq_data *irqd)
>
>         set_bit(irq, entry->irq_pending);
>
> +       /* Ensure irq_pending is visible to all cpus that retried interrupt
> +        * can run on
> +        */
> +       mb();
> +

For the most part memory barriers break my brain and there should be a
very high bar for using them instead of normal locking mechanisms.
It's got to be an area that's super performance critical. I don't
think this is.

...but also if you really can have two thread mucking with
irq_pending, it seems like you have a bigger problem. Both threads are
doing read-modify-write of irq_pending (clear_bit and set_bit aren't
atomic) and a memory barrier won't help you there.

Just use a normal locking mechanism.


-Doug