Re: [kvm-unit-tests PATCH v1 2/5] s390x: css: simplifications of the tests

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/1/21 11:11 AM, Janosch Frank wrote:
> On 1/29/21 3:34 PM, Pierre Morel wrote:

...snip...

>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index fe05021..f300969 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -118,6 +118,31 @@ out:
>>   	return schid;
>>   }
>>   
>> +/*
>> + * css_enable: enable the subchannel with the specified ISC
> 
> enabled or enable?

Forgot to change the cut and paste :(

/*
  * css_enabled: report if the sub channel is enabled

> 
> I.e. do you test if it is enabled or do you want to enable it.
> 
>> + * @schid: Subchannel Identifier
>> + * Return value:
>> + *   true if the subchannel is enabled
>> + *   false otherwise
>> + */
>> +bool css_enabled(int schid)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int cc;
>> +
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>> +			    schid, cc);
>> +		return false;
>> +	}
>> +
>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>> +		report_info("stsch: sch %08x not enabled", schid);
>> +		return 0;
> 
> Please stay with true/false or change the return type to int and use ints.

yes

> 
>> +	}
>> +	return true;
>> +}

...snip...

>> @@ -129,16 +120,21 @@ static void test_sense(void)
>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>   		    senseid->dev_type, senseid->dev_model);
>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>> +		    senseid->cu_type);
>>   
>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>> -	       (uint16_t)cu_type, senseid->cu_type);
>> +	retval = senseid->cu_type == cu_type;
>>   
>>   error:
>>   	free_io_mem(ccw, sizeof(*ccw));
>>   error_ccw:
>>   	free_io_mem(senseid, sizeof(*senseid));
>> -error_senseid:
>> -	unregister_io_int_func(css_irq_io);
>> +	return retval;
> 
> Could you return senseid->cu_type == cu_type here?

It would work for the current code and DEFAULT_CU_TYPE.
But I do not think it is a good idea due to the goto we have in error 
case before I find that it makes the code less understandable.

Other opinion?

Thanks for the comments,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen