Currently spidev allows callers to set the default speed by overriding the
max_speed_hz in the underlying device. This achieves the immediate goal but
is not what devices expect and can easily lead to userspace trying to set
unsupported speeds and succeeding, apart from anything else drivers can't
set a limit on the speed using max_speed_hz as they'd expect and any other
devices on the bus will be affected.
Instead store the default speed in the spidev struct and fill this in on
each transfer.
Signed-off-by: Mark Brown <[email protected]>
---
I've not tested this at all yet.
drivers/spi/spidev.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index e50039f..42239fd 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -87,6 +87,7 @@ struct spidev_data {
unsigned users;
u8 *tx_buffer;
u8 *rx_buffer;
+ u32 speed_hz;
};
static LIST_HEAD(device_list);
@@ -274,6 +275,8 @@ static int spidev_message(struct spidev_data *spidev,
k_tmp->bits_per_word = u_tmp->bits_per_word;
k_tmp->delay_usecs = u_tmp->delay_usecs;
k_tmp->speed_hz = u_tmp->speed_hz;
+ if (!k_tmp->speed_hz)
+ k_tmp->speed_hz = spidev->speed_hz;
#ifdef VERBOSE
dev_dbg(&spidev->spi->dev,
" xfer len %zd %s%s%s%dbits %u usec %uHz\n",
@@ -377,7 +380,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
retval = __put_user(spi->bits_per_word, (__u8 __user *)arg);
break;
case SPI_IOC_RD_MAX_SPEED_HZ:
- retval = __put_user(spi->max_speed_hz, (__u32 __user *)arg);
+ retval = __put_user(spidev->speed_hz, (__u32 __user *)arg);
break;
/* write requests */
@@ -442,9 +445,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
spi->max_speed_hz = tmp;
retval = spi_setup(spi);
if (retval < 0)
- spi->max_speed_hz = save;
+ spidev->speed_hz = tmp;
else
dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
+ spi->max_speed_hz = save;
}
break;
@@ -570,6 +574,8 @@ static int spidev_release(struct inode *inode, struct file *filp)
kfree(spidev->rx_buffer);
spidev->rx_buffer = NULL;
+ spidev->speed_hz = spidev->spi->max_speed_hz;
+
/* ... after we unbound from the underlying device? */
spin_lock_irq(&spidev->spi_lock);
dofree = (spidev->spi == NULL);
@@ -650,6 +656,8 @@ static int spidev_probe(struct spi_device *spi)
}
mutex_unlock(&device_list_lock);
+ spidev->speed_hz = spi->max_speed_hz;
+
if (status == 0)
spi_set_drvdata(spi, spidev);
else
--
2.1.1
Hi Mark,
On 11/08/2014 04:29 AM, Mark Brown wrote:
> Currently spidev allows callers to set the default speed by overriding the
> max_speed_hz in the underlying device. This achieves the immediate goal but
> is not what devices expect and can easily lead to userspace trying to set
> unsupported speeds and succeeding, apart from anything else drivers can't
> set a limit on the speed using max_speed_hz as they'd expect and any other
> devices on the bus will be affected.
>
> Instead store the default speed in the spidev struct and fill this in on
> each transfer.
>
> Signed-off-by: Mark Brown<[email protected]>
> ---
>
> I've not tested this at all yet.
>
> drivers/spi/spidev.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index e50039f..42239fd 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -87,6 +87,7 @@ struct spidev_data {
> unsigned users;
> u8 *tx_buffer;
> u8 *rx_buffer;
> + u32 speed_hz;
> };
>
> static LIST_HEAD(device_list);
> @@ -274,6 +275,8 @@ static int spidev_message(struct spidev_data *spidev,
> k_tmp->bits_per_word = u_tmp->bits_per_word;
> k_tmp->delay_usecs = u_tmp->delay_usecs;
> k_tmp->speed_hz = u_tmp->speed_hz;
> + if (!k_tmp->speed_hz)
> + k_tmp->speed_hz = spidev->speed_hz;
> #ifdef VERBOSE
> dev_dbg(&spidev->spi->dev,
> " xfer len %zd %s%s%s%dbits %u usec %uHz\n",
> @@ -377,7 +380,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> retval = __put_user(spi->bits_per_word, (__u8 __user *)arg);
> break;
> case SPI_IOC_RD_MAX_SPEED_HZ:
> - retval = __put_user(spi->max_speed_hz, (__u32 __user *)arg);
> + retval = __put_user(spidev->speed_hz, (__u32 __user *)arg);
> break;
>
> /* write requests */
> @@ -442,9 +445,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> spi->max_speed_hz = tmp;
> retval = spi_setup(spi);
> if (retval < 0)
> - spi->max_speed_hz = save;
> + spidev->speed_hz = tmp;
> else
> dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
> + spi->max_speed_hz = save;
I think the test should be if (retval >= 0) otherwise the value is only
updated on an error. With this change, I was able to successfully change
the SPI speed without affecting the max speed. This was tested using
spidev_test.c on a Altera Cyclone V which includes the DesignWare SPI IP
(spi-dw.c).
> }
> break;
>
> @@ -570,6 +574,8 @@ static int spidev_release(struct inode *inode, struct file *filp)
> kfree(spidev->rx_buffer);
> spidev->rx_buffer = NULL;
>
> + spidev->speed_hz = spidev->spi->max_speed_hz;
> +
> /* ... after we unbound from the underlying device? */
> spin_lock_irq(&spidev->spi_lock);
> dofree = (spidev->spi == NULL);
> @@ -650,6 +656,8 @@ static int spidev_probe(struct spi_device *spi)
> }
> mutex_unlock(&device_list_lock);
>
> + spidev->speed_hz = spi->max_speed_hz;
> +
> if (status == 0)
> spi_set_drvdata(spi, spidev);
> else
The echo command calls spidev_write() directly. Although the speed can't
be specified in the echo command, it did prompt me to look at that path.
In that case I think we'd need to add the .speed_hz element in
spidev_sync_write() and spidev_sync_read().
struct spi_transfer t = {
.rx_buf = spidev->rx_buffer,
.len = len,
.speed_hz = spidev->speed_hz,
};
On Tue, Nov 11, 2014 at 07:53:59AM -0600, Thor Thayer wrote:
> Hi Mark,
Please delete irrelevant context from mails, it makes it much easier to
find any new content you've added.
> On 11/08/2014 04:29 AM, Mark Brown wrote:
> > if (retval < 0)
> >- spi->max_speed_hz = save;
> >+ spidev->speed_hz = tmp;
> > else
> > dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
> >+ spi->max_speed_hz = save;
> I think the test should be if (retval >= 0) otherwise the value is only
> updated on an error. With this change, I was able to successfully change the
> SPI speed without affecting the max speed. This was tested using
> spidev_test.c on a Altera Cyclone V which includes the DesignWare SPI IP
> (spi-dw.c).
Yup, misleading code.
> > if (status == 0)
> > spi_set_drvdata(spi, spidev);
> > else
> The echo command calls spidev_write() directly. Although the speed can't be
> specified in the echo command, it did prompt me to look at that path.
> In that case I think we'd need to add the .speed_hz element in
> spidev_sync_write() and spidev_sync_read().
> struct spi_transfer t = {
> .rx_buf = spidev->rx_buffer,
> .len = len,
> .speed_hz = spidev->speed_hz,
> };
Yup, looks that way.