On 2/1/21 1:15 PM, Pierre Morel wrote:
>
>
> 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?
That's why I asked, it wasn't a direct suggestion.
I'm not a big fan of the ret/retval naming but the function is short so
it should be ok.
>
> Thanks for the comments,
> Pierre
>
