| Message ID | 20190619095254.19559-1-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | b5e3cf410b486a2415ff09b12f3ef18aba9f53ff |
| Headers | show |
| Series | spi/acpi: fix incorrect ACPI parent check | expand |
On Wed, Jun 19, 2019 at 11:52:54AM +0200, Ard Biesheuvel wrote: > The ACPI device object parsing code for SPI slaves enumerates the > entire ACPI namespace to look for devices that refer to the master > in question via the 'resource_source' field in the 'SPISerialBus' > resource. If that field does not refer to a valid ACPI device or > if it refers to the wrong SPI master, we should disregard the > device. > > Current, the valid device check is wrong, since it gets the > polarity of 'status' wrong. This could cause issues if the > 'resource_source' field is bogus but parent_handle happens to > refer to the correct master (which is not entirely imaginary > since this code runs in a loop) > > So test for ACPI_FAILURE() instead, to make the code more > self explanatory. > > Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") > Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On 6/19/19 1:16 PM, Mika Westerberg wrote: > On Wed, Jun 19, 2019 at 11:52:54AM +0200, Ard Biesheuvel wrote: >> The ACPI device object parsing code for SPI slaves enumerates the >> entire ACPI namespace to look for devices that refer to the master >> in question via the 'resource_source' field in the 'SPISerialBus' >> resource. If that field does not refer to a valid ACPI device or >> if it refers to the wrong SPI master, we should disregard the >> device. >> >> Current, the valid device check is wrong, since it gets the >> polarity of 'status' wrong. This could cause issues if the >> 'resource_source' field is bogus but parent_handle happens to >> refer to the correct master (which is not entirely imaginary >> since this code runs in a loop) >> >> So test for ACPI_FAILURE() instead, to make the code more >> self explanatory. >> >> Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") >> Reported-by: kbuild test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > I hit yesterday a regression caused by 4c3c59544f33. I've a custom ACPI tables defining I2C gpio expanders (gpio-pca953x.c as INT3491) and a spidev test device (SPT0001). Both stopped enumerating after 4c3c59544f33. With this fix spidev device enumerates but still get confused with I2C GPIO expanders (INT3491): [ 5.629874][ T1] pxa2xx-spi pxa2xx-spi.3: registered master spi3 [ 5.644447][ T1] pxa2xx-spi pxa2xx-spi.5: registered master spi5 [ 5.653930][ T1] spi spi-SPT0001:00: in setup: DMA burst size set to 8 [ 5.661300][ T1] spi spi-SPT0001:00: setup mode 0, 8 bits/w, 1000000 Hz max --> 0 [ 5.671360][ T1] spidev spi-SPT0001:00: do not use this driver in production systems! [ 5.682325][ T1] pxa2xx-spi pxa2xx-spi.5: registered child spi-SPT0001:00 [ 5.690240][ T1] spi spi-PRP0001:00: in setup: DMA burst size set to 8 [ 5.697492][ T1] spi spi-PRP0001:00: setup mode 0, 8 bits/w, 20000000 Hz max --> 0 [ 5.706928][ T1] pxa2xx-spi pxa2xx-spi.5: registered child spi-PRP0001:00 [ 5.715754][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 [ 5.721688][ T1] spi_master spi5: failed to add SPI device INT3491:00 from ACPI [ 5.730648][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 [ 5.736657][ T1] spi_master spi5: failed to add SPI device INT3491:01 from ACPI [ 5.745617][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 [ 5.751546][ T1] spi_master spi5: failed to add SPI device INT3491:02 from ACPI [ 5.760628][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 [ 5.766549][ T1] spi_master spi5: failed to add SPI device INT3491:03 from ACPI [ 5.777160][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 [ 5.783087][ T1] spi_master spi5: failed to add SPI device BCM2E95:00 from ACPI [ 5.797008][ T1] pxa2xx-spi pxa2xx-spi.6: registered master spi6 Ok log with commit 4c3c59544f33 reverted: [ 5.633116][ T1] pxa2xx-spi pxa2xx-spi.3: registered master spi3 [ 5.647701][ T1] pxa2xx-spi pxa2xx-spi.5: registered master spi5 [ 5.655668][ T1] spi spi-SPT0001:00: in setup: DMA burst size set to 8 [ 5.663066][ T1] spi spi-SPT0001:00: setup mode 0, 8 bits/w, 1000000 Hz max --> 0 [ 5.672758][ T1] pxa2xx-spi pxa2xx-spi.5: registered child spi-SPT0001:00 [ 5.680602][ T1] spi spi-PRP0001:00: in setup: DMA burst size set to 8 [ 5.687820][ T1] spi spi-PRP0001:00: setup mode 0, 8 bits/w, 20000000 Hz max --> 0 [ 5.697366][ T1] pxa2xx-spi pxa2xx-spi.5: registered child spi-PRP0001:00 [ 5.709064][ T1] pxa2xx-spi pxa2xx-spi.6: registered master spi6 [ 11.021760][ T84] spidev spi-SPT0001:00: do not use this driver in production systems! -- Jarkko
On Wed, 19 Jun 2019 at 13:58, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > > On 6/19/19 1:16 PM, Mika Westerberg wrote: > > On Wed, Jun 19, 2019 at 11:52:54AM +0200, Ard Biesheuvel wrote: > >> The ACPI device object parsing code for SPI slaves enumerates the > >> entire ACPI namespace to look for devices that refer to the master > >> in question via the 'resource_source' field in the 'SPISerialBus' > >> resource. If that field does not refer to a valid ACPI device or > >> if it refers to the wrong SPI master, we should disregard the > >> device. > >> > >> Current, the valid device check is wrong, since it gets the > >> polarity of 'status' wrong. This could cause issues if the > >> 'resource_source' field is bogus but parent_handle happens to > >> refer to the correct master (which is not entirely imaginary > >> since this code runs in a loop) > >> > >> So test for ACPI_FAILURE() instead, to make the code more > >> self explanatory. > >> > >> Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") > >> Reported-by: kbuild test robot <lkp@intel.com> > >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > I hit yesterday a regression caused by 4c3c59544f33. I've a custom ACPI > tables defining I2C gpio expanders (gpio-pca953x.c as INT3491) and a > spidev test device (SPT0001). > > Both stopped enumerating after 4c3c59544f33. With this fix spidev device > enumerates but still get confused with I2C GPIO expanders (INT3491): > Could you share the decomplied D/SSDT please? > [ 5.629874][ T1] pxa2xx-spi pxa2xx-spi.3: registered master spi3 > [ 5.644447][ T1] pxa2xx-spi pxa2xx-spi.5: registered master spi5 > [ 5.653930][ T1] spi spi-SPT0001:00: in setup: DMA burst size set to 8 > [ 5.661300][ T1] spi spi-SPT0001:00: setup mode 0, 8 bits/w, > 1000000 Hz max --> 0 > [ 5.671360][ T1] spidev spi-SPT0001:00: do not use this driver in > production systems! > [ 5.682325][ T1] pxa2xx-spi pxa2xx-spi.5: registered child > spi-SPT0001:00 > [ 5.690240][ T1] spi spi-PRP0001:00: in setup: DMA burst size set to 8 > [ 5.697492][ T1] spi spi-PRP0001:00: setup mode 0, 8 bits/w, > 20000000 Hz max --> 0 > [ 5.706928][ T1] pxa2xx-spi pxa2xx-spi.5: registered child > spi-PRP0001:00 > [ 5.715754][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 > [ 5.721688][ T1] spi_master spi5: failed to add SPI device > INT3491:00 from ACPI > [ 5.730648][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 > [ 5.736657][ T1] spi_master spi5: failed to add SPI device > INT3491:01 from ACPI > [ 5.745617][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 > [ 5.751546][ T1] spi_master spi5: failed to add SPI device > INT3491:02 from ACPI > [ 5.760628][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 > [ 5.766549][ T1] spi_master spi5: failed to add SPI device > INT3491:03 from ACPI > [ 5.777160][ T1] pxa2xx-spi pxa2xx-spi.5: cs104 >= max 4 > [ 5.783087][ T1] spi_master spi5: failed to add SPI device > BCM2E95:00 from ACPI > [ 5.797008][ T1] pxa2xx-spi pxa2xx-spi.6: registered master spi6 > > Ok log with commit 4c3c59544f33 reverted: > > [ 5.633116][ T1] pxa2xx-spi pxa2xx-spi.3: registered master spi3 > [ 5.647701][ T1] pxa2xx-spi pxa2xx-spi.5: registered master spi5 > [ 5.655668][ T1] spi spi-SPT0001:00: in setup: DMA burst size set to 8 > [ 5.663066][ T1] spi spi-SPT0001:00: setup mode 0, 8 bits/w, > 1000000 Hz max --> 0 > [ 5.672758][ T1] pxa2xx-spi pxa2xx-spi.5: registered child > spi-SPT0001:00 > [ 5.680602][ T1] spi spi-PRP0001:00: in setup: DMA burst size set to 8 > [ 5.687820][ T1] spi spi-PRP0001:00: setup mode 0, 8 bits/w, > 20000000 Hz max --> 0 > [ 5.697366][ T1] pxa2xx-spi pxa2xx-spi.5: registered child > spi-PRP0001:00 > [ 5.709064][ T1] pxa2xx-spi pxa2xx-spi.6: registered master spi6 > [ 11.021760][ T84] spidev spi-SPT0001:00: do not use this driver in > production systems! > > -- > Jarkko
On 6/19/19 2:59 PM, Ard Biesheuvel wrote: > On Wed, 19 Jun 2019 at 13:58, Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: >> >> On 6/19/19 1:16 PM, Mika Westerberg wrote: >>> On Wed, Jun 19, 2019 at 11:52:54AM +0200, Ard Biesheuvel wrote: >>>> The ACPI device object parsing code for SPI slaves enumerates the >>>> entire ACPI namespace to look for devices that refer to the master >>>> in question via the 'resource_source' field in the 'SPISerialBus' >>>> resource. If that field does not refer to a valid ACPI device or >>>> if it refers to the wrong SPI master, we should disregard the >>>> device. >>>> >>>> Current, the valid device check is wrong, since it gets the >>>> polarity of 'status' wrong. This could cause issues if the >>>> 'resource_source' field is bogus but parent_handle happens to >>>> refer to the correct master (which is not entirely imaginary >>>> since this code runs in a loop) >>>> >>>> So test for ACPI_FAILURE() instead, to make the code more >>>> self explanatory. >>>> >>>> Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") >>>> Reported-by: kbuild test robot <lkp@intel.com> >>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >>> >>> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> >>> >> I hit yesterday a regression caused by 4c3c59544f33. I've a custom ACPI >> tables defining I2C gpio expanders (gpio-pca953x.c as INT3491) and a >> spidev test device (SPT0001). >> >> Both stopped enumerating after 4c3c59544f33. With this fix spidev device >> enumerates but still get confused with I2C GPIO expanders (INT3491): >> > > Could you share the decomplied D/SSDT please? > It's Intel Edison with tables from Mika's sample ACPI tables. The interesting parts here are these two: https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/spidev.asl https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/gpioexp.asli The full tables are of course larger but I think those two above are relevant here. I build SSDT from arduino-all.asl below which includes bunch of other files and with above spidev.asl. https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/arduino-all.asl Let me know if you need full dump. -- Jarkko
On Wed, 19 Jun 2019 at 15:21, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > > On 6/19/19 2:59 PM, Ard Biesheuvel wrote: > > On Wed, 19 Jun 2019 at 13:58, Jarkko Nikula > > <jarkko.nikula@linux.intel.com> wrote: > >> > >> On 6/19/19 1:16 PM, Mika Westerberg wrote: > >>> On Wed, Jun 19, 2019 at 11:52:54AM +0200, Ard Biesheuvel wrote: > >>>> The ACPI device object parsing code for SPI slaves enumerates the > >>>> entire ACPI namespace to look for devices that refer to the master > >>>> in question via the 'resource_source' field in the 'SPISerialBus' > >>>> resource. If that field does not refer to a valid ACPI device or > >>>> if it refers to the wrong SPI master, we should disregard the > >>>> device. > >>>> > >>>> Current, the valid device check is wrong, since it gets the > >>>> polarity of 'status' wrong. This could cause issues if the > >>>> 'resource_source' field is bogus but parent_handle happens to > >>>> refer to the correct master (which is not entirely imaginary > >>>> since this code runs in a loop) > >>>> > >>>> So test for ACPI_FAILURE() instead, to make the code more > >>>> self explanatory. > >>>> > >>>> Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") > >>>> Reported-by: kbuild test robot <lkp@intel.com> > >>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > >>> > >>> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >>> > >> I hit yesterday a regression caused by 4c3c59544f33. I've a custom ACPI > >> tables defining I2C gpio expanders (gpio-pca953x.c as INT3491) and a > >> spidev test device (SPT0001). > >> > >> Both stopped enumerating after 4c3c59544f33. With this fix spidev device > >> enumerates but still get confused with I2C GPIO expanders (INT3491): > >> > > > > Could you share the decomplied D/SSDT please? > > > It's Intel Edison with tables from Mika's sample ACPI tables. The > interesting parts here are these two: > > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/spidev.asl > > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/gpioexp.asli > > The full tables are of course larger but I think those two above are > relevant here. I build SSDT from arduino-all.asl below which includes > bunch of other files and with above spidev.asl. > > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/arduino-all.asl > > Let me know if you need full dump. > So can you explain how exactly the I2C GPIO expander is failing? I struggle to understand how the SPI slave probing could be related to that.
On Wed, Jun 19, 2019 at 05:17:59PM +0300, Jarkko Nikula wrote: > Hi > > On 6/19/19 4:58 PM, Ard Biesheuvel wrote: > > So can you explain how exactly the I2C GPIO expander is failing? I > > struggle to understand how the SPI slave probing could be related to > > that. > > > They don't show up in /sys/kernel/debug/gpio, are not present in > /sys/bus/i2c/devices/ but SPI core instead tries add them with a bogus Chip > Select number: > > [ 5.727699][ T1] pxa2xx-spi pxa2xx-spi.5: cs56 >= max 4 > [ 5.733545][ T1] spi_master spi5: failed to add SPI device INT3491:00 > from ACPI Just a guess but looking at the 4c3c59544f33 acpi_register_spi_device() does not seem to zero fill the whole struct acpi_spi_lookup structure so when it is supposed to bail out when SPI slave was not found: if (!lookup.max_speed_hz) return AE_OK it instead continues to register SPI slave because lookup.max_speed_hz may contain whatever garbage there is in the stack at that address.
On Wed, 19 Jun 2019 at 16:43, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Wed, Jun 19, 2019 at 05:17:59PM +0300, Jarkko Nikula wrote: > > Hi > > > > On 6/19/19 4:58 PM, Ard Biesheuvel wrote: > > > So can you explain how exactly the I2C GPIO expander is failing? I > > > struggle to understand how the SPI slave probing could be related to > > > that. > > > > > They don't show up in /sys/kernel/debug/gpio, are not present in > > /sys/bus/i2c/devices/ but SPI core instead tries add them with a bogus Chip > > Select number: > > > > [ 5.727699][ T1] pxa2xx-spi pxa2xx-spi.5: cs56 >= max 4 > > [ 5.733545][ T1] spi_master spi5: failed to add SPI device INT3491:00 > > from ACPI > > Just a guess but looking at the 4c3c59544f33 acpi_register_spi_device() > does not seem to zero fill the whole struct acpi_spi_lookup structure so > when it is supposed to bail out when SPI slave was not found: > > if (!lookup.max_speed_hz) > return AE_OK > > it instead continues to register SPI slave because lookup.max_speed_hz > may contain whatever garbage there is in the stack at that address. Good point. Jarkko, does this help? diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 50d230b33c42..d072efdd65ba 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1914,6 +1914,7 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr, return AE_OK; lookup.ctlr = ctlr; + lookup.max_speed_hz = 0; lookup.mode = 0; lookup.bits_per_word = 0; lookup.irq = -1;
On Thu, Jun 20, 2019 at 12:33:41PM +0200, Ard Biesheuvel wrote: > Jarkko, does this help? > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 50d230b33c42..d072efdd65ba 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1914,6 +1914,7 @@ static acpi_status > acpi_register_spi_device(struct spi_controller *ctlr, > return AE_OK; > > lookup.ctlr = ctlr; > + lookup.max_speed_hz = 0; > lookup.mode = 0; > lookup.bits_per_word = 0; > lookup.irq = -1; IMHO it's better to do: memset(&lookup, 0, sizeof(lookup)); lookup.ctlr = ctlr; lookup.irq = -1; this also initializes chip_select and possibly other fields that get added to the lookup structure later.
On Thu, Jun 20, 2019 at 01:41:28PM +0300, Mika Westerberg wrote: > On Thu, Jun 20, 2019 at 12:33:41PM +0200, Ard Biesheuvel wrote: > IMHO it's better to do: > memset(&lookup, 0, sizeof(lookup)); > lookup.ctlr = ctlr; > lookup.irq = -1; > this also initializes chip_select and possibly other fields that get > added to the lookup structure later. I agree.
On Thu, 20 Jun 2019 at 13:19, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jun 20, 2019 at 01:41:28PM +0300, Mika Westerberg wrote: > > On Thu, Jun 20, 2019 at 12:33:41PM +0200, Ard Biesheuvel wrote: > > > IMHO it's better to do: > > > memset(&lookup, 0, sizeof(lookup)); > > lookup.ctlr = ctlr; > > lookup.irq = -1; > > > this also initializes chip_select and possibly other fields that get > > added to the lookup structure later. > > I agree. Sure, I will spin it like that once Jarkko confirms that this fixes his problem.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c8adcc97f3ef..50d230b33c42 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1859,7 +1859,7 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data) sb->resource_source.string_ptr, &parent_handle); - if (!status || + if (ACPI_FAILURE(status) || ACPI_HANDLE(ctlr->dev.parent) != parent_handle) return -ENODEV;
The ACPI device object parsing code for SPI slaves enumerates the entire ACPI namespace to look for devices that refer to the master in question via the 'resource_source' field in the 'SPISerialBus' resource. If that field does not refer to a valid ACPI device or if it refers to the wrong SPI master, we should disregard the device. Current, the valid device check is wrong, since it gets the polarity of 'status' wrong. This could cause issues if the 'resource_source' field is bogus but parent_handle happens to refer to the correct master (which is not entirely imaginary since this code runs in a loop) So test for ACPI_FAILURE() instead, to make the code more self explanatory. Fixes: 4c3c59544f33 ("spi/acpi: enumerate all SPI slaves in the namespace") Reported-by: kbuild test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: andy.shevchenko@gmail.com Cc: masahisa.kojima@linaro.org Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> Cc: linux-acpi@vger.kernel.org Cc: Lukas Wunner <lukas@wunner.de> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/spi/spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.1