[PATCH] spi: fix the read path in spidev

[PATCH] spi: fix the read path in spidev

[Sebastian Siewior]

This got broken by the recent "fix rmmod $spi_driver while spidev-user is active".
I tested the rmmod & write path but didn't check the read path. I am sorry.
The read logic changed and spidev_sync_read() + spidev_sync_write() do not return
zero on success anymore but the number of bytes that has been transfered over the
bus.
This patch changes the logic and copy_to_user() gets called again.

The write path returns the number of bytes which are written to the underlying device
what may be less than the requested size. This patch makes the same change to the read
path or else we request a read of 20 bytes, get 10, don't call copy to user and
report to the user that we read 10 bytes.

Signed-off-by: Sebastian Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc:  David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/spi/spidev.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 72282cd..60ec6f7 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -167,14 +167,14 @@ spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
 
 	mutex_lock(&spidev->buf_lock);
 	status = spidev_sync_read(spidev, count);
-	if (status == 0) {
+	if (status > 0) {
 		unsigned long	missing;
 
-		missing = copy_to_user(buf, spidev->buffer, count);
-		if (count && missing == count)
+		missing = copy_to_user(buf, spidev->buffer, status);
+		if (status && missing == status)
 			status = -EFAULT;
 		else
-			status = count - missing;
+			status = status - missing;
 	}
 	mutex_unlock(&spidev->buf_lock);
 
@@ -200,8 +200,6 @@ spidev_write(struct file *filp, const char __user *buf,
 	missing = copy_from_user(spidev->buffer, buf, count);
 	if (missing == 0) {
 		status = spidev_sync_write(spidev, count);
-		if (status == 0)
-			status = count;
 	} else
 		status = -EFAULT;
 	mutex_unlock(&spidev->buf_lock);
-- 
1.5.5.2


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [PATCH] spi: fix the read path in spidev

[Andrew Morton]

On Tue, 1 Jul 2008 17:45:04 +0200
Sebastian Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:

> This got broken by the recent "fix rmmod $spi_driver while spidev-user is active".
> I tested the rmmod & write path but didn't check the read path. I am sorry.
> The read logic changed and spidev_sync_read() + spidev_sync_write() do not return
> zero on success anymore but the number of bytes that has been transfered over the
> bus.
> This patch changes the logic and copy_to_user() gets called again.
> 
> The write path returns the number of bytes which are written to the underlying device
> what may be less than the requested size. This patch makes the same change to the read
> path or else we request a read of 20 bytes, get 10, don't call copy to user and
> report to the user that we read 10 bytes.
> 

Looks OK.  I'll queue it for 2.6.26.

> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 72282cd..60ec6f7 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -167,14 +167,14 @@ spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
>  
>  	mutex_lock(&spidev->buf_lock);
>  	status = spidev_sync_read(spidev, count);
> -	if (status == 0) {
> +	if (status > 0) {

`status' is a poorly chosen identifier. But Iworked it out.


>  		unsigned long	missing;
>  
> -		missing = copy_to_user(buf, spidev->buffer, count);
> -		if (count && missing == count)
> +		missing = copy_to_user(buf, spidev->buffer, status);
> +		if (status && missing == status)

The test for status!=0 is unneeded - I'll remove it.

>  			status = -EFAULT;
>  		else
> -			status = count - missing;
> +			status = status - missing;
>  	}
>  	mutex_unlock(&spidev->buf_lock);
>  
> @@ -200,8 +200,6 @@ spidev_write(struct file *filp, const char __user *buf,
>  	missing = copy_from_user(spidev->buffer, buf, count);
>  	if (missing == 0) {
>  		status = spidev_sync_write(spidev, count);
> -		if (status == 0)
> -			status = count;
>  	} else
>  		status = -EFAULT;
>  	mutex_unlock(&spidev->buf_lock);
> -- 
> 1.5.5.2

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08

Re: [PATCH] spi: fix the read path in spidev

[David Brownell]

On Tuesday 01 July 2008, Sebastian Siewior wrote:
> This got broken by the recent "fix rmmod $spi_driver while spidev-user is active".
> I tested the rmmod & write path but didn't check the read path. I am sorry.
> The read logic changed and spidev_sync_read() + spidev_sync_write() do not return
> zero on success anymore but the number of bytes that has been transfered over the
> bus.
> This patch changes the logic and copy_to_user() gets called again.

Good catch ... did you observe this failure happening "in the wild"
or was this a code-inspection kind of thing?


> 
> The write path returns the number of bytes which are written to the underlying device
> what may be less than the requested size. This patch makes the same change to the read
> path or else we request a read of 20 bytes, get 10, don't call copy to user and
> report to the user that we read 10 bytes.
> 
> Signed-off-by: Sebastian Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

Acked-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>


> Cc:  David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/spi/spidev.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 72282cd..60ec6f7 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -167,14 +167,14 @@ spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
>  
>  	mutex_lock(&spidev->buf_lock);
>  	status = spidev_sync_read(spidev, count);
> -	if (status == 0) {
> +	if (status > 0) {
>  		unsigned long	missing;
>  
> -		missing = copy_to_user(buf, spidev->buffer, count);
> -		if (count && missing == count)
> +		missing = copy_to_user(buf, spidev->buffer, status);
> +		if (status && missing == status)
>  			status = -EFAULT;
>  		else
> -			status = count - missing;
> +			status = status - missing;
>  	}
>  	mutex_unlock(&spidev->buf_lock);
>  
> @@ -200,8 +200,6 @@ spidev_write(struct file *filp, const char __user *buf,
>  	missing = copy_from_user(spidev->buffer, buf, count);
>  	if (missing == 0) {
>  		status = spidev_sync_write(spidev, count);
> -		if (status == 0)
> -			status = count;
>  	} else
>  		status = -EFAULT;
>  	mutex_unlock(&spidev->buf_lock);
> -- 
> 1.5.5.2
> 



-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08

Re: [PATCH] spi: fix the read path in spidev

[Sebastian Siewior]

David Brownell wrote:
> On Tuesday 01 July 2008, Sebastian Siewior wrote:
>> This got broken by the recent "fix rmmod $spi_driver while spidev-user is active".
>> I tested the rmmod & write path but didn't check the read path. I am sorry.
>> The read logic changed and spidev_sync_read() + spidev_sync_write() do not return
>> zero on success anymore but the number of bytes that has been transfered over the
>> bus.
>> This patch changes the logic and copy_to_user() gets called again.
> 
> Good catch ... did you observe this failure happening "in the wild"
> or was this a code-inspection kind of thing?
> 
"in the wild". I've been testing HW testing after I rebased to -rc8 and 
somehow I got no error but the receive buffer was empty....

Sebastian

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08