| Message ID | 20180821095303.27664-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 04b2d03a75652bda989de1595048f0501dc0c0a0 |
| Headers | show |
| Series | [-next] spi: Fix double IDR allocation with DT aliases | expand |
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 <geert+renesas@glider.be> > --- > 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 <greg@kroah.com> 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 <geert+renesas@glider.be> > > --- > > 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
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 <geert+renesas@glider.be> > --- > 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 <geert+renesas@glider.be> 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 <geert+renesas@glider.be> This fixes SPI on imx51-babbage, thanks. Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
> 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 <kirill.kapranov@compulab.co.il> --- 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++;
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);
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 <geert+renesas@glider.be> --- Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF. --- drivers/spi/spi.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)