[PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Jonathan Cameron]

By bypassing the standard macros for setting up the kfifo we can
take advantage of the fixed record size implementation without
having to have a type to pass in (from which the size of an element
is normally established).

In IIO we have variable 'scans' as our records in which any element
can be present or not.  They do not however vary when we are
actually filling or reading from the buffer.  Thus we have a fixed
record size whenever we are actually running.  As setup and tear
down are not in the fast path we can take the overhead of reinitializing
the kfifo every time.

This is an RFC as

a) I'm far from sure I got it right. (I didn't first time round)
b) There is probably a better way of doing it!

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/iio/kfifo_buf.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index 6bf9d05..6ec763f 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
 		return -EINVAL;
 
 	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
-	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
+	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
+			     bytes_per_datum, GFP_KERNEL);
 }
 
 static int iio_request_update_kfifo(struct iio_buffer *r)
@@ -94,9 +95,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 {
 	int ret;
 	struct iio_kfifo *kf = iio_to_kfifo(r);
-	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
-	if (ret != r->bytes_per_datum)
+	ret = kfifo_in(&kf->kf, data, 1);
+	if (ret != 1)
 		return -EBUSY;
+
 	return 0;
 }
 
@@ -106,11 +108,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
 	int ret, copied;
 	struct iio_kfifo *kf = iio_to_kfifo(r);
 
-	if (n < r->bytes_per_datum)
+	if (n < r->bytes_per_datum || r->bytes_per_datum == 0)
 		return -EINVAL;
 
-	n = rounddown(n, r->bytes_per_datum);
 	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
+	if (ret < 0)
+		return ret;
 
 	return copied;
 }
-- 
1.7.10.2

Re: [PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Jonathan Cameron]

On 05/22/2012 06:56 AM, Lars-Peter Clausen wrote:
> On 05/19/2012 01:14 PM, Jonathan Cameron wrote:
>> By bypassing the standard macros for setting up the kfifo we can
>> take advantage of the fixed record size implementation without
>> having to have a type to pass in (from which the size of an element
>> is normally established).
>>
>> In IIO we have variable 'scans' as our records in which any element
>> can be present or not.  They do not however vary when we are
>> actually filling or reading from the buffer.  Thus we have a fixed
>> record size whenever we are actually running.  As setup and tear
>> down are not in the fast path we can take the overhead of reinitializing
>> the kfifo every time.
>>
>> This is an RFC as
>>
>> a) I'm far from sure I got it right.
>> b) There is probably a better way of doing it!
>>
>> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>> Note this is against current staging-next.
>>
>>
>>  drivers/iio/kfifo_buf.c |    7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
>> index 6bf9d05..74b1cb8 100644
>> --- a/drivers/iio/kfifo_buf.c
>> +++ b/drivers/iio/kfifo_buf.c
>> @@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>>  		return -EINVAL;
>>  
>>  	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
>> -	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
>> +	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
>> +			     bytes_per_datum, GFP_KERNEL);
>>  }
>>  
>>  static int iio_request_update_kfifo(struct iio_buffer *r)
>> @@ -94,7 +95,7 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>  {
>>  	int ret;
>>  	struct iio_kfifo *kf = iio_to_kfifo(r);
>> -	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>> +	ret = __kfifo_in((struct __kfifo *)&kf->kf, data, r->bytes_per_datum);
> 
> The last parameter has to be 1 now, since we want to store one record. And I
> think we can still use kfifo_in(...), the macro magic take care of doing the
> right thing.
> 
>>  	if (ret != r->bytes_per_datum)
>>  		return -EBUSY;
>>  	return 0;
>> @@ -110,7 +111,7 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>>  		return -EINVAL;
>>  
>>  	n = rounddown(n, r->bytes_per_datum);
>> -	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
> 
> Same here. n needs to be n / r->bytes_per_datum
Actually nope. That's the the one case where it is in bytes.  It's
saying how long the destination buffer is rather than how many records
to copy.  Couldn't for a minute work out why I was getting hardly any
data!  Anyhow, new version comming up shortly...
> 
>> +	ret = __kfifo_to_user((struct __kfifo *)&kf->kf, buf, n, &copied);
>>  
>>  	return copied;
>>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Jonathan Cameron]

On 05/22/2012 06:56 AM, Lars-Peter Clausen wrote:
> On 05/19/2012 01:14 PM, Jonathan Cameron wrote:
>> By bypassing the standard macros for setting up the kfifo we can
>> take advantage of the fixed record size implementation without
>> having to have a type to pass in (from which the size of an element
>> is normally established).
>>
>> In IIO we have variable 'scans' as our records in which any element
>> can be present or not.  They do not however vary when we are
>> actually filling or reading from the buffer.  Thus we have a fixed
>> record size whenever we are actually running.  As setup and tear
>> down are not in the fast path we can take the overhead of reinitializing
>> the kfifo every time.
>>
>> This is an RFC as
>>
>> a) I'm far from sure I got it right.
>> b) There is probably a better way of doing it!
>>
>> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>> Note this is against current staging-next.
>>
>>
>>  drivers/iio/kfifo_buf.c |    7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
>> index 6bf9d05..74b1cb8 100644
>> --- a/drivers/iio/kfifo_buf.c
>> +++ b/drivers/iio/kfifo_buf.c
>> @@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>>  		return -EINVAL;
>>  
>>  	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
>> -	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
>> +	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
>> +			     bytes_per_datum, GFP_KERNEL);
>>  }
>>  
>>  static int iio_request_update_kfifo(struct iio_buffer *r)
>> @@ -94,7 +95,7 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>  {
>>  	int ret;
>>  	struct iio_kfifo *kf = iio_to_kfifo(r);
>> -	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>> +	ret = __kfifo_in((struct __kfifo *)&kf->kf, data, r->bytes_per_datum);
> 
> The last parameter has to be 1 now, since we want to store one record. And I
> think we can still use kfifo_in(...), the macro magic take care of doing the
> right thing.
good point on the macro. I got lost following that the first time, but
with the alloc as above, recsize will be zero and all it will fall
through to this.
> 
>>  	if (ret != r->bytes_per_datum)
>>  		return -EBUSY;
>>  	return 0;
>> @@ -110,7 +111,7 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>>  		return -EINVAL;
>>  
>>  	n = rounddown(n, r->bytes_per_datum);
>> -	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
> 
> Same here. n needs to be n / r->bytes_per_datum
indeed it does. also looks like the kfifo_to_user macro will work I think...

Will resend.  Having one of those fun days of bisecting at the moment to
find out what broke my test board...
> 
>> +	ret = __kfifo_to_user((struct __kfifo *)&kf->kf, buf, n, &copied);
>>  
>>  	return copied;
>>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Lars-Peter Clausen]

On 05/19/2012 01:14 PM, Jonathan Cameron wrote:
> By bypassing the standard macros for setting up the kfifo we can
> take advantage of the fixed record size implementation without
> having to have a type to pass in (from which the size of an element
> is normally established).
> 
> In IIO we have variable 'scans' as our records in which any element
> can be present or not.  They do not however vary when we are
> actually filling or reading from the buffer.  Thus we have a fixed
> record size whenever we are actually running.  As setup and tear
> down are not in the fast path we can take the overhead of reinitializing
> the kfifo every time.
> 
> This is an RFC as
> 
> a) I'm far from sure I got it right.
> b) There is probably a better way of doing it!
> 
> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> ---
> Note this is against current staging-next.
> 
> 
>  drivers/iio/kfifo_buf.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index 6bf9d05..74b1cb8 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>  		return -EINVAL;
>  
>  	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
> -	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
> +	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
> +			     bytes_per_datum, GFP_KERNEL);
>  }
>  
>  static int iio_request_update_kfifo(struct iio_buffer *r)
> @@ -94,7 +95,7 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>  {
>  	int ret;
>  	struct iio_kfifo *kf = iio_to_kfifo(r);
> -	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
> +	ret = __kfifo_in((struct __kfifo *)&kf->kf, data, r->bytes_per_datum);

The last parameter has to be 1 now, since we want to store one record. And I
think we can still use kfifo_in(...), the macro magic take care of doing the
right thing.

>  	if (ret != r->bytes_per_datum)
>  		return -EBUSY;
>  	return 0;
> @@ -110,7 +111,7 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>  		return -EINVAL;
>  
>  	n = rounddown(n, r->bytes_per_datum);
> -	ret = kfifo_to_user(&kf->kf, buf, n, &copied);

Same here. n needs to be n / r->bytes_per_datum

> +	ret = __kfifo_to_user((struct __kfifo *)&kf->kf, buf, n, &copied);
>  
>  	return copied;
>  }

Re: [PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Jonathan Cameron]

Forgot to mention that my testing on this change has been pretty
'light' with a couple of runs of my generic buffer test on a modified
max1363 driver.  Hence would be great if anyone else has a chance
to check this hasn't messed things up for them! Also forgot to
edit the patch title to say it was an rfc at this point.

> By bypassing the standard macros for setting up the kfifo we can
> take advantage of the fixed record size implementation without
> having to have a type to pass in (from which the size of an element
> is normally established).
> 
> In IIO we have variable 'scans' as our records in which any element
> can be present or not.  They do not however vary when we are
> actually filling or reading from the buffer.  Thus we have a fixed
> record size whenever we are actually running.  As setup and tear
> down are not in the fast path we can take the overhead of reinitializing
> the kfifo every time.
> 
> This is an RFC as
> 
> a) I'm far from sure I got it right.
> b) There is probably a better way of doing it!
> 
> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> ---
> Note this is against current staging-next.
> 
> 
>  drivers/iio/kfifo_buf.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index 6bf9d05..74b1cb8 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>  		return -EINVAL;
>  
>  	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
> -	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
> +	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
> +			     bytes_per_datum, GFP_KERNEL);
>  }
>  
>  static int iio_request_update_kfifo(struct iio_buffer *r)
> @@ -94,7 +95,7 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>  {
>  	int ret;
>  	struct iio_kfifo *kf = iio_to_kfifo(r);
> -	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
> +	ret = __kfifo_in((struct __kfifo *)&kf->kf, data, r->bytes_per_datum);
>  	if (ret != r->bytes_per_datum)
>  		return -EBUSY;
>  	return 0;
> @@ -110,7 +111,7 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>  		return -EINVAL;
>  
>  	n = rounddown(n, r->bytes_per_datum);
> -	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
> +	ret = __kfifo_to_user((struct __kfifo *)&kf->kf, buf, n, &copied);
>  
>  	return copied;
>  }

[PATCH] iio:kfifo_buf Take advantage of the fixed record size used in IIO

[Jonathan Cameron]

By bypassing the standard macros for setting up the kfifo we can
take advantage of the fixed record size implementation without
having to have a type to pass in (from which the size of an element
is normally established).

In IIO we have variable 'scans' as our records in which any element
can be present or not.  They do not however vary when we are
actually filling or reading from the buffer.  Thus we have a fixed
record size whenever we are actually running.  As setup and tear
down are not in the fast path we can take the overhead of reinitializing
the kfifo every time.

This is an RFC as

a) I'm far from sure I got it right.
b) There is probably a better way of doing it!

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
Note this is against current staging-next.


 drivers/iio/kfifo_buf.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index 6bf9d05..74b1cb8 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -22,7 +22,8 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
 		return -EINVAL;
 
 	__iio_update_buffer(&buf->buffer, bytes_per_datum, length);
-	return kfifo_alloc(&buf->kf, bytes_per_datum*length, GFP_KERNEL);
+	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
+			     bytes_per_datum, GFP_KERNEL);
 }
 
 static int iio_request_update_kfifo(struct iio_buffer *r)
@@ -94,7 +95,7 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 {
 	int ret;
 	struct iio_kfifo *kf = iio_to_kfifo(r);
-	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
+	ret = __kfifo_in((struct __kfifo *)&kf->kf, data, r->bytes_per_datum);
 	if (ret != r->bytes_per_datum)
 		return -EBUSY;
 	return 0;
@@ -110,7 +111,7 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
 		return -EINVAL;
 
 	n = rounddown(n, r->bytes_per_datum);
-	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
+	ret = __kfifo_to_user((struct __kfifo *)&kf->kf, buf, n, &copied);
 
 	return copied;
 }
-- 
1.7.10.2