| Message ID | 20180326120251.13054-1-aniruddhab@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [RESEND,RFC] irqchip: arm-gic: take gic_lock when updating irq type | expand |
On 26/03/18 13:02, Aniruddha Banerjee wrote: [an encrypted email, again] Can you *please* stop sending encrypted emails, specially as you target linux-tegra@vger.kernel.org? Thanks, M.
Hi Marc, On 26/03/18 13:14, Marc Zyngier wrote: > On 26/03/18 13:02, Aniruddha Banerjee wrote: > > [an encrypted email, again] > > Can you *please* stop sending encrypted emails, specially as you target > linux-tegra@vger.kernel.org? That's odd, this does not appear to be encrypted to me. I have also checked from my gmail as well. I did not see anything in the header? Cheers Jon
Hi Jon, On 26/03/18 13:49, Jon Hunter wrote: > Hi Marc, > > On 26/03/18 13:14, Marc Zyngier wrote: >> On 26/03/18 13:02, Aniruddha Banerjee wrote: >> >> [an encrypted email, again] >> >> Can you *please* stop sending encrypted emails, specially as you target >> linux-tegra@vger.kernel.org? > > That's odd, this does not appear to be encrypted to me. I have also > checked from my gmail as well. I did not see anything in the header? The email I'm getting from you is encrypted as well: -----BEGIN PGP MESSAGE----- Version: PGP Universal 3.3.2 (Build 21495) Charset: utf-8 qANQR1DBwUwDtofwhD5N/iMBD/0XcZF6OYqXx9NG3lG+01IwThAqc5C2SlT/K29d ilYqmUWLbklnCBZv7lu1zpeBMxqAw5og74lbCrBrnWmTgrehw9Gs8FDQnylsO07n yoANBe/9DXykf5P3keX9O8cWdyklMdr1+zbC2xfp2/pn30qt25bc+VHV2QAsOf5j +tRysiRnGrVlRtt5b8VHryAf1PGsSC3I06MaC0muEmcqO5IWfo82MdJUYSqvvypY Ry7bzka2Tq7ooQZVGLfjPj7VJIO1V6dQcJW0zfzaVKp3BPgcTgdEoBUl+fDKY7J2 Co//0LxK0/G7oAvO0dtAzXIIP+TVvFcnlZdEoMoF3Uej7y8KvPTSFVWAqtADIY9p 6SRtHHEPaGjggIzfe9uopRJs6tye6oba0ZjpMEcEYiRgyha8hbpWwdB4FA6tyfeh H5rBtgK/RpauUYsiDZYUMgs0lYT6U6zkfAAb/J+yV95fV3lhrrQTM4UrsFPfnBNL kv0UyVUsgIH+WGvk2l+hirpwcS/pQNegy34V/pXzMARkzzWhrOauuosbTnYPJ8pO l36tXl2megqsCuc/FDoPYf/+l5mZ1gX4yVC1Xbm+xBCIc+h+Zu15VBw9nZ2iPMlg HbUFPxbKfUcgKxpDJPWV+NlcB24GZOoJ0W+IHd6TMeWtdzPevw53mEnBLUskxvpn Lx/OW8HATAM8ookUzrlK2AEH/i8yp8yZQfnnqeXh6Px3pZLe0eubvqbXHYzcgibG KWy53H/FiTcVWkzPBml9ZuvQ5ZPL3PZMkJ36DKmXwzd3HMBVGOK3Rgb1cYFX92zk 5q1tGDUwqKcPLX4cXbvownx6GNpCgCgghlBsxM/oaxCWfdoEnzQDMGsQ24za+bzm VTfipRs/Hiqj0vFvq8No/jAt4KS6YAFIwbIk9RJ8/fLyE6pYrAzvvT0Fu78R9PgW xIJ2wPh8+ixdFgAacHzbDkYrsHbDBRZCaZtdQmtNGWvc255ePKTUja5pdBY+5bGp 3vFPK3DeTQ14ObbmtJBMPNJbCECRM84BBIXW4RUISIRaQ/jSwo4B3iG245Z4pF3l XQJElNNX0PksLW9XIgAfvi9W0LTgdpv2mrSr6VG5qAvs+Pyn4GL6DrDKafCBZbvS yQOukICfIhR0WPHBJVXdmyTrcQ5RyQdgrxkThwtEivG9AwTtEV/Usnd/COuxFPKf +Lb/tDK8W86cbrSUXZHT8deA3ivfQQRLvSifN8T/+rAFkqKLrOGBGnXpI5ZlH4tV 2eVaGVbDLZaoLvagJiFj3izpmILLTeSe19VfgNWtB1GNRBZexQqJmjP/KKZrJ13f mITNVbIQDcyoJnkxImE1LWB0ujPL1MXaUA2om9G6DCPdiCnkVHC3/zthR0SR4XFe zFIWxb4fevxnrEs91lqKlYFBCpc7RrJEvyvlUYw61AObgp6iuUD8gN3WhEYhLCHT qXDhJH2U4rAvq3BCkZpGa4n8e2KYQvP/dPooILcfQQE8u21/7vdKBIQ0sz2rHteY ANZ4DiB3e/bxLMttk7wtCb37VLnIvlKtORXtGFbMdyCm/SVhbT5lQ3QjwXjVpp72 B2pzU5839QDU+b511o4MEuONzr/jD7owZu+vDySJ76DDePDa8MDPD+18dMAiVKvb X4h5dok8JAlrnKrtjKH2HzfDKLeDcRqSJ7cFJKRpg2jiKD5c4XdA3x0m94XJb5FJ LwEcSMBjin6qa+Z9wKfozacv7VbuaYqLMwNTAHtjdKqBmEldGtv62oQzo9/pCo83 B7rjEoBmrkRB7IJi049m44JLlUXxsna9FOjke1Rcwx68QeGOrqZZ8b9+cG2uVogH 5CNMl+/Ja17mNPrN3vZNwaJIH8/aw52+ufQ5mBNrBpRAwjh/C6gQHjSiBK2zQdeM E3mtZn387O3N1gErAtCp0uBglm7fcF13Pnvi6GQdHuqQ+nryjxR0GRWIN0lTsnEs FNfn0SK5Xb2fisrVjfKgYbGdyFg+Wo2zhOWdEC0XWg/DA4ypji6revV2rlrfUDnk s1CoOxBAsa+0vS7Ujjb+t71KxL15ucRFLjdT3SGOfDe3uLYGcFn7DW1RxfeRHhSX TcNe68y9j29Rn88+qSQe1xrid/wtDi4AilZa/LTd8IKNrXw1KXQ0GOdFVTjxJ7bu ezd5CAYpxEfnCO/j8nKqu9M2fcI= =WVon -----END PGP MESSAGE----- Is that an Nvidia thing? Anyway, it looks like things get encrypted without people asking for it. I'll reply to Aniruddha's email in clear text, since it has made it to the list unencrypted. Thanks, M.
Hi Marc, On 26/03/18 14:10, Marc Zyngier wrote: > On 26/03/18 13:49, Jon Hunter wrote: >> Hi Marc, >> >> On 26/03/18 13:14, Marc Zyngier wrote: >>> On 26/03/18 13:02, Aniruddha Banerjee wrote: >>> >>> [an encrypted email, again] >>> >>> Can you *please* stop sending encrypted emails, specially as you target >>> linux-tegra@vger.kernel.org? >> >> That's odd, this does not appear to be encrypted to me. I have also >> checked from my gmail as well. I did not see anything in the header? > > The email I'm getting from you is encrypted as well: > > -----BEGIN PGP MESSAGE----- > Version: PGP Universal 3.3.2 (Build 21495) > Charset: utf-8 > > qANQR1DBwUwDtofwhD5N/iMBD/0XcZF6OYqXx9NG3lG+01IwThAqc5C2SlT/K29d > ilYqmUWLbklnCBZv7lu1zpeBMxqAw5og74lbCrBrnWmTgrehw9Gs8FDQnylsO07n > yoANBe/9DXykf5P3keX9O8cWdyklMdr1+zbC2xfp2/pn30qt25bc+VHV2QAsOf5j > +tRysiRnGrVlRtt5b8VHryAf1PGsSC3I06MaC0muEmcqO5IWfo82MdJUYSqvvypY > Ry7bzka2Tq7ooQZVGLfjPj7VJIO1V6dQcJW0zfzaVKp3BPgcTgdEoBUl+fDKY7J2 > Co//0LxK0/G7oAvO0dtAzXIIP+TVvFcnlZdEoMoF3Uej7y8KvPTSFVWAqtADIY9p > 6SRtHHEPaGjggIzfe9uopRJs6tye6oba0ZjpMEcEYiRgyha8hbpWwdB4FA6tyfeh > H5rBtgK/RpauUYsiDZYUMgs0lYT6U6zkfAAb/J+yV95fV3lhrrQTM4UrsFPfnBNL > kv0UyVUsgIH+WGvk2l+hirpwcS/pQNegy34V/pXzMARkzzWhrOauuosbTnYPJ8pO > l36tXl2megqsCuc/FDoPYf/+l5mZ1gX4yVC1Xbm+xBCIc+h+Zu15VBw9nZ2iPMlg > HbUFPxbKfUcgKxpDJPWV+NlcB24GZOoJ0W+IHd6TMeWtdzPevw53mEnBLUskxvpn > Lx/OW8HATAM8ookUzrlK2AEH/i8yp8yZQfnnqeXh6Px3pZLe0eubvqbXHYzcgibG > KWy53H/FiTcVWkzPBml9ZuvQ5ZPL3PZMkJ36DKmXwzd3HMBVGOK3Rgb1cYFX92zk > 5q1tGDUwqKcPLX4cXbvownx6GNpCgCgghlBsxM/oaxCWfdoEnzQDMGsQ24za+bzm > VTfipRs/Hiqj0vFvq8No/jAt4KS6YAFIwbIk9RJ8/fLyE6pYrAzvvT0Fu78R9PgW > xIJ2wPh8+ixdFgAacHzbDkYrsHbDBRZCaZtdQmtNGWvc255ePKTUja5pdBY+5bGp > 3vFPK3DeTQ14ObbmtJBMPNJbCECRM84BBIXW4RUISIRaQ/jSwo4B3iG245Z4pF3l > XQJElNNX0PksLW9XIgAfvi9W0LTgdpv2mrSr6VG5qAvs+Pyn4GL6DrDKafCBZbvS > yQOukICfIhR0WPHBJVXdmyTrcQ5RyQdgrxkThwtEivG9AwTtEV/Usnd/COuxFPKf > +Lb/tDK8W86cbrSUXZHT8deA3ivfQQRLvSifN8T/+rAFkqKLrOGBGnXpI5ZlH4tV > 2eVaGVbDLZaoLvagJiFj3izpmILLTeSe19VfgNWtB1GNRBZexQqJmjP/KKZrJ13f > mITNVbIQDcyoJnkxImE1LWB0ujPL1MXaUA2om9G6DCPdiCnkVHC3/zthR0SR4XFe > zFIWxb4fevxnrEs91lqKlYFBCpc7RrJEvyvlUYw61AObgp6iuUD8gN3WhEYhLCHT > qXDhJH2U4rAvq3BCkZpGa4n8e2KYQvP/dPooILcfQQE8u21/7vdKBIQ0sz2rHteY > ANZ4DiB3e/bxLMttk7wtCb37VLnIvlKtORXtGFbMdyCm/SVhbT5lQ3QjwXjVpp72 > B2pzU5839QDU+b511o4MEuONzr/jD7owZu+vDySJ76DDePDa8MDPD+18dMAiVKvb > X4h5dok8JAlrnKrtjKH2HzfDKLeDcRqSJ7cFJKRpg2jiKD5c4XdA3x0m94XJb5FJ > LwEcSMBjin6qa+Z9wKfozacv7VbuaYqLMwNTAHtjdKqBmEldGtv62oQzo9/pCo83 > B7rjEoBmrkRB7IJi049m44JLlUXxsna9FOjke1Rcwx68QeGOrqZZ8b9+cG2uVogH > 5CNMl+/Ja17mNPrN3vZNwaJIH8/aw52+ufQ5mBNrBpRAwjh/C6gQHjSiBK2zQdeM > E3mtZn387O3N1gErAtCp0uBglm7fcF13Pnvi6GQdHuqQ+nryjxR0GRWIN0lTsnEs > FNfn0SK5Xb2fisrVjfKgYbGdyFg+Wo2zhOWdEC0XWg/DA4ypji6revV2rlrfUDnk > s1CoOxBAsa+0vS7Ujjb+t71KxL15ucRFLjdT3SGOfDe3uLYGcFn7DW1RxfeRHhSX > TcNe68y9j29Rn88+qSQe1xrid/wtDi4AilZa/LTd8IKNrXw1KXQ0GOdFVTjxJ7bu > ezd5CAYpxEfnCO/j8nKqu9M2fcI= > =WVon > -----END PGP MESSAGE----- > > Is that an Nvidia thing? Anyway, it looks like things get encrypted > without people asking for it. I'll reply to Aniruddha's email in clear > text, since it has made it to the list unencrypted. I wonder if something has changed, as I have not had such problems before. Thanks for reporting, we will look into it. Cheers Jon
Hi Aniruddha, On 26/03/18 13:02, Aniruddha Banerjee wrote: > The kernel documentation states that the locking of the irq-chip > registers should be handled by the irq-chip driver. In the irq-gic, > the accesses to the irqchip are seemingly not protected and multiple > writes to SPIs from different irq descriptors do RMW requests without > taking the irq-chip lock. When multiple irqs call the request_irq at > the same time, there can be a simultaneous write at the gic > distributor, leading to a race. Acquire the gic_lock when the > irq_type is updated. > > Signed-off-by: Aniruddha Banerjee <aniruddhab@nvidia.com> > --- > drivers/irqchip/irq-gic.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4c797b43614d..61380f5a2254 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -67,6 +67,8 @@ static void gic_check_cpu_features(void) > #define gic_check_cpu_features() do { } while(0) > #endif > > +static DEFINE_RAW_SPINLOCK(irq_controller_lock); > + > union gic_base { > void __iomem *common_base; > void __percpu * __iomem *percpu_base; > @@ -529,6 +531,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > { > void __iomem *base = gic_dist_base(d); > unsigned int gicirq = gic_irq(d); > + int ret; > > /* Interrupt configuration for SGIs can't be changed */ > if (gicirq < 16) > @@ -539,7 +542,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > - return gic_configure_irq(gicirq, type, base, NULL); > + raw_spin_lock(&irq_controller_lock); > + ret = gic_configure_irq(gicirq, type, base, NULL); > + raw_spin_unlock(&irq_controller_lock); > + > + return ret; > } > > static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) > You do have a good point here, but it'd be better to put it directly in the gic_configure_irq() function, fixing both GICv1/2 and v3 in one go. That would allow the sync_access method to be run outside of the critical section. Please post it to LKML and add a Cc stable for it. Thanks, M.
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4c797b43614d..61380f5a2254 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -67,6 +67,8 @@ static void gic_check_cpu_features(void) #define gic_check_cpu_features() do { } while(0) #endif +static DEFINE_RAW_SPINLOCK(irq_controller_lock); + union gic_base { void __iomem *common_base; void __percpu * __iomem *percpu_base; @@ -529,6 +531,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) { void __iomem *base = gic_dist_base(d); unsigned int gicirq = gic_irq(d); + int ret; /* Interrupt configuration for SGIs can't be changed */ if (gicirq < 16) @@ -539,7 +542,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) type != IRQ_TYPE_EDGE_RISING) return -EINVAL; - return gic_configure_irq(gicirq, type, base, NULL); + raw_spin_lock(&irq_controller_lock); + ret = gic_configure_irq(gicirq, type, base, NULL); + raw_spin_unlock(&irq_controller_lock); + + return ret; } static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
The kernel documentation states that the locking of the irq-chip registers should be handled by the irq-chip driver. In the irq-gic, the accesses to the irqchip are seemingly not protected and multiple writes to SPIs from different irq descriptors do RMW requests without taking the irq-chip lock. When multiple irqs call the request_irq at the same time, there can be a simultaneous write at the gic distributor, leading to a race. Acquire the gic_lock when the irq_type is updated. Signed-off-by: Aniruddha Banerjee <aniruddhab@nvidia.com> --- drivers/irqchip/irq-gic.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)