| Message ID | 1657087331-32455-4-git-send-email-quic_clew@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add smp2p retrigger support | expand |
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 >
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
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; }
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(+)