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

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

On 2/2/21 12:23 PM, Cornelia Huck wrote:
> On Fri, 29 Jan 2021 15:34:26 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
...snip...

>> +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;
>> +	}
>> +	return true;
>> +}
>>   /*
>>    * css_enable: enable the subchannel with the specified ISC
>>    * @schid: Subchannel Identifier
>> @@ -167,18 +192,8 @@ retry:
>>   	/*
>>   	 * Read the SCHIB again to verify the enablement
>>   	 */
>> -	cc = stsch(schid, &schib);
>> -	if (cc) {
>> -		report_info("stsch: updating sch %08x failed with cc=%d",
>> -			    schid, cc);
>> -		return cc;
>> -	}
>> -
>> -	if ((pmcw->flags & flags) == flags) {
>> -		report_info("stsch: sch %08x successfully modified after %d retries",
>> -			    schid, retry_count);
>> +	if (css_enabled(schid))
> 
> This is a slightly different test now. Previously, you also checked
> whether the ISC matched the requested one. Not sure how valuable that
> test was.

Yes, I do not think it can be anything else when the CSS accept to 
enable the subchannel.

...
>>   
>> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
>> -	if (ret) {
>> -		report(0, "Could not enable the subchannel: %08x",
>> -		       test_device_sid);
>> -		return;
>> +	if (!css_enabled(test_device_sid)) {
>> +		report(0, "enabled subchannel: %08x", test_device_sid);
> 
> Isn't that a _not_ enabled subchannel?

:) yes, may be:

s/enabled/enabling/


...snip...

>>   
>>   static void css_init(void)
>> @@ -146,8 +142,17 @@ static void css_init(void)
>>   	int ret;
>>   
>>   	ret = get_chsc_scsc();
>> -	if (!ret)
>> -		report(1, " ");
>> +	if (ret)
>> +		return;
> 
> Shouldn't you report a failure here?

Clearly yes.


Thanks for the comments,
regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen