Re: [PATCH] spi: Add option to bind spidev to all chipselects
On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: > Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is > set. Rename spidev devices to avoid sysfs conflict. To repeat my previous feedback I don't see any way in which this is sane, having two drivers simultaneously controlling the same device is an obviously terrible idea. signature.asc Description: Digital signature
Re: [PATCH] spi: Add option to bind spidev to all chipselects
On 13 May 2015 at 12:16, Maxime Ripard wrote: > Hi, > > On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: >> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW >> is >> set. Rename spidev devices to avoid sysfs conflict. >> >> This allows dynamically loading SPI device overlays or communicating >> with SPI devices configured by a kernel driver from userspace. >> >> Signed-off-by: Michal Suchanek > > Output from checkpatch: > total: 2 errors, 4 warnings, 4 checks, 157 lines checked > > ... > > I told you a few times already to run checkpatch before sending your > patches, apparently you make a point at ignoring me. Fine. That's a good idea to run, yes. Sorry about that. I also discovered an additional issue with unused variable when the config option is set. > > That being said, I'm not sure this is the right approach, or at least, > it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still > have the same issue with addition of new devices on previously unused > chip selects, and where we have an spidev device now. > > What I think we should do is, when a new device is created, we just > lookup the modalias of the spi_device associated to it. > > If that modalias is "spidev", then unregister the spidev device, > register the new device, you're done. If not, return an error. Yes, that's what I intend to look into eventually. However, this patch is still useful and allows both accessing unused bus with spidev and dynamically loading overlays that would use the bus. > > On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good > idea. There's a good chance it will break the driver by doing stuff > behind its back, possibly in a way that will harm the whole kernel, > and it's something we usually try to avoid. What is the possibility to harm the whole kernel? If the kernel crashes because the device misses a message this is somewhat worrying. You could see it as a developer option similar to SCSI error injection and others that can introduce states that would normally occur only rarely. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: Add option to bind spidev to all chipselects
Hi, On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: > Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is > set. Rename spidev devices to avoid sysfs conflict. > > This allows dynamically loading SPI device overlays or communicating > with SPI devices configured by a kernel driver from userspace. > > Signed-off-by: Michal Suchanek Output from checkpatch: total: 2 errors, 4 warnings, 4 checks, 157 lines checked ... I told you a few times already to run checkpatch before sending your patches, apparently you make a point at ignoring me. Fine. That being said, I'm not sure this is the right approach, or at least, it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still have the same issue with addition of new devices on previously unused chip selects, and where we have an spidev device now. What I think we should do is, when a new device is created, we just lookup the modalias of the spi_device associated to it. If that modalias is "spidev", then unregister the spidev device, register the new device, you're done. If not, return an error. On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good idea. There's a good chance it will break the driver by doing stuff behind its back, possibly in a way that will harm the whole kernel, and it's something we usually try to avoid. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
[PATCH] spi: Add option to bind spidev to all chipselects
Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
set. Rename spidev devices to avoid sysfs conflict.
This allows dynamically loading SPI device overlays or communicating
with SPI devices configured by a kernel driver from userspace.
Signed-off-by: Michal Suchanek
---
drivers/spi/Kconfig | 13 +
drivers/spi/spi.c | 74 ++---
include/linux/spi/spi.h | 1 +
3 files changed, 65 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..b477828 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -651,6 +651,19 @@ config SPI_SPIDEV
Note that this application programming interface is EXPERIMENTAL
and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
+config SPIDEV_SHADOW
+ depends on SPI_SPIDEV
+ bool "Allow spidev access to configured SPI devices (DANGEROUS)"
+ help
+ This creates a spidev device node for every chipselect.
+
+ It is possible to access even SPI devices which are in use by a
+ kernel driver. This allows invoking features not in use by the kernel
+ or checking device state from userspace when the kernel driver fails.
+
+ Sending out-of-order messages to the device or reconfiguring the
+ device might cause driver failure. DANGEROUS
+
config SPI_TLE62X0
tristate "Infineon TLE62X0 (for power switching)"
depends on SYSFS
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e6ca46e..b48a0dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -254,16 +254,23 @@ struct spi_device *spi_alloc_device(struct spi_master
*master)
}
EXPORT_SYMBOL_GPL(spi_alloc_device);
-static void spi_dev_set_name(struct spi_device *spi)
+static void spi_dev_set_name(struct spi_device *spi, const char * alias)
{
struct acpi_device *adev = ACPI_COMPANION(>dev);
if (adev) {
- dev_set_name(>dev, "spi-%s", acpi_dev_name(adev));
+ if (alias)
+ dev_set_name(>dev, "%s-%s", alias,
acpi_dev_name(adev));
+ else
+ dev_set_name(>dev, "spi-%s", acpi_dev_name(adev));
return;
}
- dev_set_name(>dev, "%s.%u", dev_name(>master->dev),
+ if (alias)
+ dev_set_name(>dev, "%s%u.%u", alias, spi->master->bus_num,
+spi->chip_select);
+ else
+ dev_set_name(>dev, "%s.%u", dev_name(>master->dev),
spi->chip_select);
}
@@ -272,22 +279,25 @@ 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)
+ if (spi->master == new_spi->master
+ && spi->chip_select == new_spi->chip_select
+#ifdef CONFIG_SPIDEV_SHADOW
+ && !spi->shadow && !new_spi->shadow
+#endif
+ )
return -EBUSY;
return 0;
}
/**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * spi_add_device_alias - Add spi_device allocated with spi_alloc_device
+ * possibly even when device for the CS exists.
* @spi: spi_device to register
+ * @alias: string used as device name prefix or NULL
*
- * Companion function to spi_alloc_device. Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Returns 0 on success; negative errno on failure
+ * See spi_add_device
*/
-int spi_add_device(struct spi_device *spi)
+static inline int spi_add_device_alias(struct spi_device *spi, const char *
alias)
{
static DEFINE_MUTEX(spi_add_lock);
struct spi_master *master = spi->master;
@@ -303,7 +313,8 @@ int spi_add_device(struct spi_device *spi)
}
/* Set the bus ID string */
- spi_dev_set_name(spi);
+ spi_dev_set_name(spi, alias);
+ spi->shadow = !!alias;
/* We need to make sure there's no other device with this
* chipselect **BEFORE** we call setup(), else we'll trash
@@ -321,15 +332,17 @@ int spi_add_device(struct spi_device *spi)
if (master->cs_gpios)
spi->cs_gpio = master->cs_gpios[spi->chip_select];
- /* Drivers may modify this initial i/o setup, but will
-* normally rely on the device being setup. Devices
-* using SPI_CS_HIGH can't coexist well otherwise...
-*/
- status = spi_setup(spi);
- if (status < 0) {
- dev_err(dev, "can't setup %s, status %d\n",
- dev_name(>dev), status);
- goto done;
+ if (!alias) {
+ /* Drivers may modify this initial i/o setup, but will
+* normally rely on the device being setup. Devices
+* using
Re: [PATCH] spi: Add option to bind spidev to all chipselects
On 13 May 2015 at 12:16, Maxime Ripard maxime.rip...@free-electrons.com wrote: Hi, On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is set. Rename spidev devices to avoid sysfs conflict. This allows dynamically loading SPI device overlays or communicating with SPI devices configured by a kernel driver from userspace. Signed-off-by: Michal Suchanek hramr...@gmail.com Output from checkpatch: total: 2 errors, 4 warnings, 4 checks, 157 lines checked ... I told you a few times already to run checkpatch before sending your patches, apparently you make a point at ignoring me. Fine. That's a good idea to run, yes. Sorry about that. I also discovered an additional issue with unused variable when the config option is set. That being said, I'm not sure this is the right approach, or at least, it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still have the same issue with addition of new devices on previously unused chip selects, and where we have an spidev device now. What I think we should do is, when a new device is created, we just lookup the modalias of the spi_device associated to it. If that modalias is spidev, then unregister the spidev device, register the new device, you're done. If not, return an error. Yes, that's what I intend to look into eventually. However, this patch is still useful and allows both accessing unused bus with spidev and dynamically loading overlays that would use the bus. On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good idea. There's a good chance it will break the driver by doing stuff behind its back, possibly in a way that will harm the whole kernel, and it's something we usually try to avoid. What is the possibility to harm the whole kernel? If the kernel crashes because the device misses a message this is somewhat worrying. You could see it as a developer option similar to SCSI error injection and others that can introduce states that would normally occur only rarely. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: Add option to bind spidev to all chipselects
Hi, On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is set. Rename spidev devices to avoid sysfs conflict. This allows dynamically loading SPI device overlays or communicating with SPI devices configured by a kernel driver from userspace. Signed-off-by: Michal Suchanek hramr...@gmail.com Output from checkpatch: total: 2 errors, 4 warnings, 4 checks, 157 lines checked ... I told you a few times already to run checkpatch before sending your patches, apparently you make a point at ignoring me. Fine. That being said, I'm not sure this is the right approach, or at least, it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still have the same issue with addition of new devices on previously unused chip selects, and where we have an spidev device now. What I think we should do is, when a new device is created, we just lookup the modalias of the spi_device associated to it. If that modalias is spidev, then unregister the spidev device, register the new device, you're done. If not, return an error. On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good idea. There's a good chance it will break the driver by doing stuff behind its back, possibly in a way that will harm the whole kernel, and it's something we usually try to avoid. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
[PATCH] spi: Add option to bind spidev to all chipselects
Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
set. Rename spidev devices to avoid sysfs conflict.
This allows dynamically loading SPI device overlays or communicating
with SPI devices configured by a kernel driver from userspace.
Signed-off-by: Michal Suchanek hramr...@gmail.com
---
drivers/spi/Kconfig | 13 +
drivers/spi/spi.c | 74 ++---
include/linux/spi/spi.h | 1 +
3 files changed, 65 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..b477828 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -651,6 +651,19 @@ config SPI_SPIDEV
Note that this application programming interface is EXPERIMENTAL
and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
+config SPIDEV_SHADOW
+ depends on SPI_SPIDEV
+ bool Allow spidev access to configured SPI devices (DANGEROUS)
+ help
+ This creates a spidev device node for every chipselect.
+
+ It is possible to access even SPI devices which are in use by a
+ kernel driver. This allows invoking features not in use by the kernel
+ or checking device state from userspace when the kernel driver fails.
+
+ Sending out-of-order messages to the device or reconfiguring the
+ device might cause driver failure. DANGEROUS
+
config SPI_TLE62X0
tristate Infineon TLE62X0 (for power switching)
depends on SYSFS
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e6ca46e..b48a0dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -254,16 +254,23 @@ struct spi_device *spi_alloc_device(struct spi_master
*master)
}
EXPORT_SYMBOL_GPL(spi_alloc_device);
-static void spi_dev_set_name(struct spi_device *spi)
+static void spi_dev_set_name(struct spi_device *spi, const char * alias)
{
struct acpi_device *adev = ACPI_COMPANION(spi-dev);
if (adev) {
- dev_set_name(spi-dev, spi-%s, acpi_dev_name(adev));
+ if (alias)
+ dev_set_name(spi-dev, %s-%s, alias,
acpi_dev_name(adev));
+ else
+ dev_set_name(spi-dev, spi-%s, acpi_dev_name(adev));
return;
}
- dev_set_name(spi-dev, %s.%u, dev_name(spi-master-dev),
+ if (alias)
+ dev_set_name(spi-dev, %s%u.%u, alias, spi-master-bus_num,
+spi-chip_select);
+ else
+ dev_set_name(spi-dev, %s.%u, dev_name(spi-master-dev),
spi-chip_select);
}
@@ -272,22 +279,25 @@ 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)
+ if (spi-master == new_spi-master
+spi-chip_select == new_spi-chip_select
+#ifdef CONFIG_SPIDEV_SHADOW
+!spi-shadow !new_spi-shadow
+#endif
+ )
return -EBUSY;
return 0;
}
/**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * spi_add_device_alias - Add spi_device allocated with spi_alloc_device
+ * possibly even when device for the CS exists.
* @spi: spi_device to register
+ * @alias: string used as device name prefix or NULL
*
- * Companion function to spi_alloc_device. Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Returns 0 on success; negative errno on failure
+ * See spi_add_device
*/
-int spi_add_device(struct spi_device *spi)
+static inline int spi_add_device_alias(struct spi_device *spi, const char *
alias)
{
static DEFINE_MUTEX(spi_add_lock);
struct spi_master *master = spi-master;
@@ -303,7 +313,8 @@ int spi_add_device(struct spi_device *spi)
}
/* Set the bus ID string */
- spi_dev_set_name(spi);
+ spi_dev_set_name(spi, alias);
+ spi-shadow = !!alias;
/* We need to make sure there's no other device with this
* chipselect **BEFORE** we call setup(), else we'll trash
@@ -321,15 +332,17 @@ int spi_add_device(struct spi_device *spi)
if (master-cs_gpios)
spi-cs_gpio = master-cs_gpios[spi-chip_select];
- /* Drivers may modify this initial i/o setup, but will
-* normally rely on the device being setup. Devices
-* using SPI_CS_HIGH can't coexist well otherwise...
-*/
- status = spi_setup(spi);
- if (status 0) {
- dev_err(dev, can't setup %s, status %d\n,
- dev_name(spi-dev), status);
- goto done;
+ if (!alias) {
+ /* Drivers may modify this initial i/o setup, but will
+* normally rely on the device being setup. Devices
+* using
Re: [PATCH] spi: Add option to bind spidev to all chipselects
On Wed, May 13, 2015 at 09:34:41AM -, Michal Suchanek wrote: Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is set. Rename spidev devices to avoid sysfs conflict. To repeat my previous feedback I don't see any way in which this is sane, having two drivers simultaneously controlling the same device is an obviously terrible idea. signature.asc Description: Digital signature
