[greybus-dev] [PATCH 1/2] staging: greybus: loopback: use gb_loopback_async_wait_all don't spin

[Bryan O'Donoghue]

On 20/12/16 10:28, Johan Hovold wrote:
> On Tue, Dec 20, 2016 at 01:12:08AM +0000, Bryan O'Donoghue wrote:
>> Currently the greybus-loopback thread logic spins around waiting for
>> send_count == iteration_max which on real hardware doesn't make a
>> difference to us but in simulation is excruciatingly slow, anti-social and
>> bad manners. Use the existing gb_loopback_async_wait_all() function to gate
>> continuing when the send_count == iteration_max and go to sleep until
>> there's something worthwhile to-do.
>>
>> Signed-off-by: Bryan O'Donoghue <pure.logic at nexus-software.ie>
> 
> You forgot to CC me and Alex.
> 
>> ---
>>  drivers/staging/greybus/loopback.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
>> index 7882306..d6302ef 100644
>> --- a/drivers/staging/greybus/loopback.c
>> +++ b/drivers/staging/greybus/loopback.c
>> @@ -1008,11 +1008,23 @@ static int gb_loopback_fn(void *data)
>>  
>>  		/* Optionally terminate */
>>  		if (gb->send_count == gb->iteration_max) {
>> +			mutex_unlock(&gb->mutex);
>> +
>> +			/* Wait for synchronous and asynchronus completion */
>> +			gb_loopback_async_wait_all(gb);
> 
> This introduces a non-interruptible wait here, which should be ok given
> that all outstanding operations will be cancelled by core as part of
> connection disable before stopping the kthread during disconnect.
> 
> But this driver is full of such subtleties and really needs a clean up
> (or rewrite).

A rewrite is on the bucket list at some point.

>> +
>> +			/* Mark complete unless user-space has poked us */
>> +			mutex_lock(&gb->mutex);
>>  			if (gb->iteration_count == gb->iteration_max) {
>>  				gb->type = 0;
>>  				gb->send_count = 0;
>>  				sysfs_notify(&gb->dev->kobj,  NULL,
>>  						"iteration_count");
>> +				dev_dbg(&gb->connection->bundle->dev,
> 
> You have a pointer to the bundle you should use here and below.

Will do.

> 
>> +					"load test complete\n");
>> +			} else {
>> +				dev_dbg(&gb->connection->bundle->dev,
>> +					"continuing on with new test set\n");
> 
> The fact that user-space can reset test counters and change parameters
> while a test is running is really another bug.

Agreed will be done on the rewrite.

---
bod