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

Pierre Morel
2021-02-01T12:16:28Z

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