[PATCH] staging: comedi: comedi_buf: introduce comedi_buf_n_bytes_ready()

[Hartley Sweeten]

On Wednesday, June 04, 2014 11:17 AM, Ian Abbott wrote:
> On 2014-05-29 18:38, H Hartley Sweeten wrote:
>> Introduce a helper function to return the number of bytes that are
>> ready to read/write from/to the comedi_async buffer. The "write to"
>> use doesn't really make much sense but is handled for completeness.
>>
>> Use the helper function in the comedi drivers that currently do the
>> calculation as part of the (*poll) operation.
>>
>> Also, use the helper in comedi_fops where the calculation is used as
>> part of the subdevice going nonbusy.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>> Cc: Ian Abbott <abbotti at mev.co.uk>
>> Cc: Greg Kroah-Hartman <gregk at linuxfoundation.org>
>> ---
>>   drivers/staging/comedi/comedi_buf.c            | 18 ++++++++++++++++++
>>   drivers/staging/comedi/comedi_fops.c           |  5 ++---
>>   drivers/staging/comedi/comedidev.h             |  2 ++
>>   drivers/staging/comedi/drivers/das16m1.c       |  2 +-
>>   drivers/staging/comedi/drivers/das1800.c       |  2 +-
>>   drivers/staging/comedi/drivers/ni_mio_common.c |  2 +-
>>   drivers/staging/comedi/drivers/ni_pcidio.c     |  2 +-
>>   drivers/staging/comedi/drivers/pcl812.c        |  2 +-
>>   drivers/staging/comedi/drivers/pcl816.c        |  2 +-
>>   9 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/comedi_buf.c b/drivers/staging/comedi/comedi_buf.c
>> index df4a9c4..52fc634 100644
>> --- a/drivers/staging/comedi/comedi_buf.c
>> +++ b/drivers/staging/comedi/comedi_buf.c
>> @@ -243,6 +243,24 @@ void comedi_buf_reset(struct comedi_subdevice *s)
>>   	async->events = 0;
>>   }
>>
>> +/**
>> + * comedi_buf_n_bytes_ready() - Returns the number of bytes ready to read/write
>> + * @s: comedi_subdevice struct
>> + */
>> +unsigned int comedi_buf_n_bytes_ready(struct comedi_subdevice *s)
>> +{
>> +	struct comedi_async *async = s->async;
>> +
>> +	if (async->buf_write_count == async->buf_read_count)
>> +		return 0;
>> +
>> +	if (async->buf_write_count > async->buf_read_count)
>> +		return async->buf_write_count - async->buf_read_count;
>> +	else
>> +		return async->buf_read_count - async->buf_write_count;
>> +}
>> +EXPORT_SYMBOL_GPL(comedi_buf_n_bytes_ready);
>> +
>
> This will actually break things.  The buf_write_count, buf_read_count, 
> buf_write_alloc_count, buf_read_alloc_count, and munge_count members of 
> struct comedi_async are all supposed to work modulo 2^32 (UINT_MAX+1). 
> Logically (with big-ints):
> 
> buf_read_count <= buf_read_alloc_count <= munge_count <= buf_write_count 
> <= buf_write_alloc_count <= (buf_read_count + prealloc_bufsz)
> 
> and logically they only increase in value, but in practice are all 
> Reduced modulo 2^32.  The upshot is that this function should always return:
>
>	return async->buf_write_count - async->buf_read_count;
>
> which could easily be inlined to avoid an external function call.

I don't see how it "breaks" anything since the two if cases will return the
same result as your proposal above. And if the logic of your "count" values
is correct, and I assume it is, then the 'else' case could never happen.

Regardless, I like your inline proposal better than the exported function.

I'll rebase this as suggested.

Thanks,
Hartley