| Message ID | 1389273835-8615-1-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b6fb8d3a1f156c50a35f88b9b55f404034493938 |
| Headers | show |
On 01/09/2014 03:23 PM, Mika Westerberg wrote: > Commit e13ac47bec20 (spi: Use stable dev_name for ACPI enumerated SPI > slaves) changed the SPI device naming to be based on ACPI device name > instead of carrying bus number and chip select for devices enumerated > from ACPI namespace. > > In case of a buggy BIOS that lists multiple SPI devices sharing the same > chip select (even though they should use different) the current code fails > to detect that and allows the devices to be added to the bus. > > Fix this by walking through the bus and comparing spi->chip_select instead > of device name. This should work regardless what the device name will be in > future. > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/spi/spi.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 349ebba4b199..13dcc360bde6 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > I guess this is a valid fix in that sense that before my patch those ill-defined SPI devices were not added. Is this something which should go to 3.13? Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 09, 2014 at 03:23:55PM +0200, Mika Westerberg wrote: > In case of a buggy BIOS that lists multiple SPI devices sharing the same > chip select (even though they should use different) the current code fails > to detect that and allows the devices to be added to the bus. This actually also points out another issue - if we have two instances of a device on different buses they could end up with the same dev_name() if they have the same chip select. Hrm. I'll apply this but we'll probably need to do something about such conflicts at some point.
On Thu, Jan 09, 2014 at 02:06:46PM +0000, Mark Brown wrote: > On Thu, Jan 09, 2014 at 03:23:55PM +0200, Mika Westerberg wrote: > > > In case of a buggy BIOS that lists multiple SPI devices sharing the same > > chip select (even though they should use different) the current code fails > > to detect that and allows the devices to be added to the bus. > > This actually also points out another issue - if we have two instances > of a device on different buses they could end up with the same > dev_name() if they have the same chip select. Hrm. I don't think that can happen, at least in case of ACPI, since each ACPI device has unique name, such as 'INT33C0:00' - the ':00' suffix is the "instance" number ACPI core gives for devices. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 09, 2014 at 04:27:29PM +0200, Mika Westerberg wrote: > On Thu, Jan 09, 2014 at 02:06:46PM +0000, Mark Brown wrote: > > This actually also points out another issue - if we have two instances > > of a device on different buses they could end up with the same > > dev_name() if they have the same chip select. Hrm. > I don't think that can happen, at least in case of ACPI, since each ACPI > device has unique name, such as 'INT33C0:00' - the ':00' suffix is the > "instance" number ACPI core gives for devices. That's OK then.
On Thu, Jan 09, 2014 at 03:59:38PM +0200, Jarkko Nikula wrote: > On 01/09/2014 03:23 PM, Mika Westerberg wrote: > >Commit e13ac47bec20 (spi: Use stable dev_name for ACPI enumerated SPI > >slaves) changed the SPI device naming to be based on ACPI device name > >instead of carrying bus number and chip select for devices enumerated > >from ACPI namespace. > > > >In case of a buggy BIOS that lists multiple SPI devices sharing the same > >chip select (even though they should use different) the current code fails > >to detect that and allows the devices to be added to the bus. > > > >Fix this by walking through the bus and comparing spi->chip_select instead > >of device name. This should work regardless what the device name will be in > >future. > > > >Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> > >Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >--- > > drivers/spi/spi.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > >diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > >index 349ebba4b199..13dcc360bde6 100644 > >--- a/drivers/spi/spi.c > >+++ b/drivers/spi/spi.c > > > I guess this is a valid fix in that sense that before my patch those > ill-defined SPI devices were not added. Is this something which > should go to 3.13? This is pretty rare case so I wouldn't expect any current HSW machines for example to have such bugs. I'm not even sure if the SPI bus on LPSS is used anywhere outside Intel currently. However, we happen to have few development machines here where this bug exists. Up to Mark to decide whether this is urgent enough to be included in 3.13. > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 349ebba4b199..13dcc360bde6 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -370,6 +370,17 @@ static void spi_dev_set_name(struct spi_device *spi) spi->chip_select); } +static int spi_dev_check(struct device *dev, void *data) +{ + struct spi_device *spi = to_spi_device(dev); + struct spi_device *new_spi = data; + + if (spi->master == new_spi->master && + spi->chip_select == new_spi->chip_select) + return -EBUSY; + return 0; +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -384,7 +395,6 @@ int spi_add_device(struct spi_device *spi) static DEFINE_MUTEX(spi_add_lock); struct spi_master *master = spi->master; struct device *dev = master->dev.parent; - struct device *d; int status; /* Chipselects are numbered 0..max; validate. */ @@ -404,12 +414,10 @@ int spi_add_device(struct spi_device *spi) */ mutex_lock(&spi_add_lock); - d = bus_find_device_by_name(&spi_bus_type, NULL, dev_name(&spi->dev)); - if (d != NULL) { + status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); + if (status) { dev_err(dev, "chipselect %d already in use\n", spi->chip_select); - put_device(d); - status = -EBUSY; goto done; }
Commit e13ac47bec20 (spi: Use stable dev_name for ACPI enumerated SPI slaves) changed the SPI device naming to be based on ACPI device name instead of carrying bus number and chip select for devices enumerated from ACPI namespace. In case of a buggy BIOS that lists multiple SPI devices sharing the same chip select (even though they should use different) the current code fails to detect that and allows the devices to be added to the bus. Fix this by walking through the bus and comparing spi->chip_select instead of device name. This should work regardless what the device name will be in future. Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/spi/spi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)