If the SPI bus number is provided by a DT alias, idr_alloc() is called
twice, leading to:
WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
couldn't get idr
Fix this by moving the handling of fixed SPI bus numbers up, before the
DT handling code fills in ctlr->bus_num.
Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
---
drivers/spi/spi.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a00d006d4c3a1c5a..9da0bc5a036cfff6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2143,8 +2143,17 @@ int spi_register_controller(struct spi_controller *ctlr)
*/
if (ctlr->num_chipselect == 0)
return -EINVAL;
- /* allocate dynamic bus number using Linux idr */
- if ((ctlr->bus_num < 0) && ctlr->dev.of_node) {
+ if (ctlr->bus_num >= 0) {
+ /* devices with a fixed bus num must check-in with the num */
+ mutex_lock(&board_lock);
+ id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
+ ctlr->bus_num + 1, GFP_KERNEL);
+ mutex_unlock(&board_lock);
+ if (WARN(id < 0, "couldn't get idr"))
+ return id == -ENOSPC ? -EBUSY : id;
+ ctlr->bus_num = id;
+ } else if (ctlr->dev.of_node) {
+ /* allocate dynamic bus number using Linux idr */
id = of_alias_get_id(ctlr->dev.of_node, "spi");
if (id >= 0) {
ctlr->bus_num = id;
@@ -2170,15 +2179,6 @@ int spi_register_controller(struct spi_controller *ctlr)
if (WARN(id < 0, "couldn't get idr"))
return id;
ctlr->bus_num = id;
- } else {
- /* devices with a fixed bus num must check-in with the num */
- mutex_lock(&board_lock);
- id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
- ctlr->bus_num + 1, GFP_KERNEL);
- mutex_unlock(&board_lock);
- if (WARN(id < 0, "couldn't get idr"))
- return id == -ENOSPC ? -EBUSY : id;
- ctlr->bus_num = id;
}
INIT_LIST_HEAD(&ctlr->queue);
spin_lock_init(&ctlr->queue_lock);
--
2.17.1
On Tue, Aug 21, 2018 at 11:53:03AM +0200, Geert Uytterhoeven wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
>
> WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
> couldn't get idr
>
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
>
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> ---
> drivers/spi/spi.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
Hi Greg,
On Tue, Aug 21, 2018 at 3:40 PM Greg KH <[email protected]> wrote:
> On Tue, Aug 21, 2018 at 11:53:03AM +0200, Geert Uytterhoeven wrote:
> > If the SPI bus number is provided by a DT alias, idr_alloc() is called
> > twice, leading to:
> >
> > WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
> > couldn't get idr
> >
> > Fix this by moving the handling of fixed SPI bus numbers up, before the
> > DT handling code fills in ctlr->bus_num.
> >
> > Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > ---
> > Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> > ---
> > drivers/spi/spi.c | 22 +++++++++++-----------
> > 1 file changed, 11 insertions(+), 11 deletions(-)
> >
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
I know.
I only CCed stable because the acceptance email for the original patch was
CCed to stable, and I wanted to prevent that one from being backported early.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert
Thank you for keeping me informed.
I have to point at the following threat: a dynamically allocated ID may
'squat' a bus ID that intended for a device with statically allocated
ID. This scenario is possible since module loading order is uncertain.
This threat seems to be inevitable...
--
Best regards,
Kirill.
On 08/21/2018 12:53 PM, Geert Uytterhoeven wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
>
> WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
> couldn't get idr
>
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
>
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> ---
> drivers/spi/spi.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index a00d006d4c3a1c5a..9da0bc5a036cfff6 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2143,8 +2143,17 @@ int spi_register_controller(struct spi_controller *ctlr)
> */
> if (ctlr->num_chipselect == 0)
> return -EINVAL;
> - /* allocate dynamic bus number using Linux idr */
> - if ((ctlr->bus_num < 0) && ctlr->dev.of_node) {
> + if (ctlr->bus_num >= 0) {
> + /* devices with a fixed bus num must check-in with the num */
> + mutex_lock(&board_lock);
> + id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
> + ctlr->bus_num + 1, GFP_KERNEL);
> + mutex_unlock(&board_lock);
> + if (WARN(id < 0, "couldn't get idr"))
> + return id == -ENOSPC ? -EBUSY : id;
> + ctlr->bus_num = id;
> + } else if (ctlr->dev.of_node) {
> + /* allocate dynamic bus number using Linux idr */
> id = of_alias_get_id(ctlr->dev.of_node, "spi");
> if (id >= 0) {
> ctlr->bus_num = id;
> @@ -2170,15 +2179,6 @@ int spi_register_controller(struct spi_controller *ctlr)
> if (WARN(id < 0, "couldn't get idr"))
> return id;
> ctlr->bus_num = id;
> - } else {
> - /* devices with a fixed bus num must check-in with the num */
> - mutex_lock(&board_lock);
> - id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
> - ctlr->bus_num + 1, GFP_KERNEL);
> - mutex_unlock(&board_lock);
> - if (WARN(id < 0, "couldn't get idr"))
> - return id == -ENOSPC ? -EBUSY : id;
> - ctlr->bus_num = id;
> }
> INIT_LIST_HEAD(&ctlr->queue);
> spin_lock_init(&ctlr->queue_lock);
>
On Wed, Aug 22, 2018 at 08:51:40PM +0300, Kirill Kapranov wrote:
Please don't top post, reply in line with needed context. This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.
> I have to point at the following threat: a dynamically allocated ID may
> 'squat' a bus ID that intended for a device with statically allocated ID.
> This scenario is possible since module loading order is uncertain.
> This threat seems to be inevitable...
For DT systems the dynamically allocated IDs start at the maximum
positive ID and work down so in practice it is vanishingly unlikely that
there will be a collision as idiomatic static DT IDs would be low
integers.
> For DT systems the dynamically allocated IDs start at the maximum
> positive ID and work down so in practice it is vanishingly unlikely that
> there will be a collision as idiomatic static DT IDs would be low
> integers.
Yes, this algorithm seems really bullet-proof. However, it isn't
actually used now. The ID allocation algorithm using atomic_dec_return
call had been introduced 2006-01-08 as [1]. It was being used in the
mainline kernel (with some improvements) up to 2017-08-16, when it has
been replaced with the newer algorithm using Linux idr, accordingly [2].
Since idr_alloc call works incrementally, the situation of a 'fixed' ID
squatting by a driver with 'dynamic ID' seems more than possible.
Therefore it would be justified to use a hardcoded constant
SPI_DYN_FIRST_BUS_NUM (that was introduced in [2] and eliminated in
[3]), but with a sufficiently greater value of the constant.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=8ae12a0d85987dc138f8c944cb78a92bf466cea0
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=9b61e302210eba55768962f2f11e96bb508c2408
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=42bdd7061a6e24d7b21d3d21973615fecef544ef
On Sat, Aug 25, 2018 at 08:54:53PM +0300, Kirill Kapranov wrote:
> > For DT systems the dynamically allocated IDs start at the maximum
> > positive ID and work down so in practice it is vanishingly unlikely that
> > there will be a collision as idiomatic static DT IDs would be low
> > integers.
>
> Yes, this algorithm seems really bullet-proof. However, it isn't actually
> used now. The ID allocation algorithm using atomic_dec_return call had been
> introduced 2006-01-08 as [1]. It was being used in the mainline kernel (with
> some improvements) up to 2017-08-16, when it has been replaced with the
> newer algorithm using Linux idr, accordingly [2].
Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
> Since idr_alloc call works incrementally, the situation of a 'fixed' ID
> squatting by a driver with 'dynamic ID' seems more than possible.
> Therefore it would be justified to use a hardcoded constant
> SPI_DYN_FIRST_BUS_NUM (that was introduced in [2] and eliminated in [3]),
> but with a sufficiently greater value of the constant.
Right, that clearly wasn't an intended effect, though - should be using
the max of the big constant and the maximum static ID.
Hi Geert,
On Tue, Aug 21, 2018 at 6:53 AM, Geert Uytterhoeven
<[email protected]> wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
>
> WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
> couldn't get idr
>
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
>
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
This fixes SPI on imx51-babbage, thanks.
Tested-by: Fabio Estevam <[email protected]>
> Right, that clearly wasn't an intended effect, though - should be using
> the max of the big constant and the maximum static ID.
Due to difficulties of enumeration of all device in a system, I propose
to assign safe sub-range for dynamic IDs in the upper-half of ID range the following way.
NOTE-0: This patch has to be applied after Geert's patch,
which fixes double call of idr_alloc. (above in the thread). Many thanks, Geert!
NOTE-1: The best solution, IMHO, would be migration to property_get_*
functions, declared in linux/property.h
[PATCH] Eliminate possibility of conflict between static and dynamic IDs
For systems without DT support allocate dynamical bus ID in a safe sub-range
(if given), otherwise in upper half of the range so as to avoid possible
conflict between statically and dynamically allocated IDs.
Signed-off-by: Kirill Kapranov <[email protected]>
---
drivers/spi/Kconfig | 8 ++++++++
drivers/spi/spi.c | 22 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 671d078349cc..7498fae0113b 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -47,6 +47,14 @@ config SPI_MASTER
if SPI_MASTER
+config SPI_MASTER_DYN_BUS_NUM_FIRST
+ int "First dynamically assigned SPI bus ID" if EXPERT
+ default 0
+ help
+ This value can be used as the beginning of sub-range for dynamic
+ allocation of SPI bus ID in case of absence of DT. If -1 chosen, the
+ sub-range will be allocated in upper half of the SPI bus ID range.
+
config SPI_MEM
bool "SPI memory extension"
help
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9da0bc5a036c..3ac0cf0ab49c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -49,6 +49,18 @@
#include "internals.h"
+#ifndef CHAR_BIT
+#define CHAR_BIT 8 /* Normally in <limits.h> */
+#endif
+#if !defined(CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST) || \
+ CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST < 0
+//Half of the biggest signed int of this size
+#define SPI_DYN_FIRST_NUM (((1 << \
+ (sizeof(((struct spi_controller*)0)->bus_num) * CHAR_BIT - 1)) - 1) / 2)
+#else
+#define SPI_DYN_FIRST_NUM CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST
+#endif
+
static DEFINE_IDR(spi_master_idr);
static void spidev_release(struct device *dev)
@@ -2167,7 +2179,15 @@ int spi_register_controller(struct spi_controller *ctlr)
}
if (ctlr->bus_num < 0) {
first_dynamic = of_alias_get_highest_id("spi");
- if (first_dynamic < 0)
+
+ if (first_dynamic == -ENOSYS)
+ /*
+ * In case of system without DT support, allocate
+ * dynamic bus ID in safe range, higher than the bound,
+ * to avoid conflict between static and dynamic ID
+ */
+ first_dynamic = SPI_DYN_FIRST_NUM;
+ else if (first_dynamic < 0)
first_dynamic = 0;
else
first_dynamic++;
--
2.11.0
The patch
spi: Fix double IDR allocation with DT aliases
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 04b2d03a75652bda989de1595048f0501dc0c0a0 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <[email protected]>
Date: Tue, 21 Aug 2018 11:53:03 +0200
Subject: [PATCH] spi: Fix double IDR allocation with DT aliases
If the SPI bus number is provided by a DT alias, idr_alloc() is called
twice, leading to:
WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
couldn't get idr
Fix this by moving the handling of fixed SPI bus numbers up, before the
DT handling code fills in ctlr->bus_num.
Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
Signed-off-by: Geert Uytterhoeven <[email protected]>
Tested-by: Fabio Estevam <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a00d006d4c3a..9da0bc5a036c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2143,8 +2143,17 @@ int spi_register_controller(struct spi_controller *ctlr)
*/
if (ctlr->num_chipselect == 0)
return -EINVAL;
- /* allocate dynamic bus number using Linux idr */
- if ((ctlr->bus_num < 0) && ctlr->dev.of_node) {
+ if (ctlr->bus_num >= 0) {
+ /* devices with a fixed bus num must check-in with the num */
+ mutex_lock(&board_lock);
+ id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
+ ctlr->bus_num + 1, GFP_KERNEL);
+ mutex_unlock(&board_lock);
+ if (WARN(id < 0, "couldn't get idr"))
+ return id == -ENOSPC ? -EBUSY : id;
+ ctlr->bus_num = id;
+ } else if (ctlr->dev.of_node) {
+ /* allocate dynamic bus number using Linux idr */
id = of_alias_get_id(ctlr->dev.of_node, "spi");
if (id >= 0) {
ctlr->bus_num = id;
@@ -2170,15 +2179,6 @@ int spi_register_controller(struct spi_controller *ctlr)
if (WARN(id < 0, "couldn't get idr"))
return id;
ctlr->bus_num = id;
- } else {
- /* devices with a fixed bus num must check-in with the num */
- mutex_lock(&board_lock);
- id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
- ctlr->bus_num + 1, GFP_KERNEL);
- mutex_unlock(&board_lock);
- if (WARN(id < 0, "couldn't get idr"))
- return id == -ENOSPC ? -EBUSY : id;
- ctlr->bus_num = id;
}
INIT_LIST_HEAD(&ctlr->queue);
spin_lock_init(&ctlr->queue_lock);
--
2.18.0