Message-ID: <6208840a-bb97-6b45-7b8e-80ad79849129@linux.ibm.com>
Date:   Wed, 6 Apr 2022 18:44:37 +0200
From:   Peter Oberparleiter <oberpar@...ux.ibm.com>
To:     Heiko Carstens <hca@...ux.ibm.com>,
        Haowen Bai <baihaowen@...zu.com>,
        Vineeth Vijayan <vneethv@...ux.ibm.com>
Cc:     Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] s390: Simplify the calculation of variables

On 06.04.2022 10:45, Heiko Carstens wrote:
> On Wed, Apr 06, 2022 at 10:37:31AM +0800, Haowen Bai wrote:
>> Fix the following coccicheck warnings:
>> ./arch/s390/include/asm/scsw.h:695:47-49: WARNING
>>  !A || A && B is equivalent to !A || B
>>
>> I apply a readable version just to get rid of a warning.
>>
>> Signed-off-by: Haowen Bai <baihaowen@...zu.com>
>> ---
>> V1->V2: apply a readable and simple version as suggestion.
>>
>>  arch/s390/include/asm/scsw.h | 47 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 32 insertions(+), 15 deletions(-)
> 
> [full quote below]
> 
> Vineeth, Peter, could one of you please Review and or ACK the patch
> below?

This patch changes scsw->cmd access to scsw->tm access, which is
incorrect, so I cannot ACK them as is.

Also I'm somewhat torn on the general question if these functions should
be changed:

- the current implementation is unreadable => change it
- the current implementation works => keep it
- improvement patches like this one seem to appear regularly and consume
  time in reviewing => change it

If there was a new version that really improved readability, this would
be really welcome. The problem is that the definition of readability is
special for these functions: each of these functions implement a
validity check based on text from the s390 Principles of Operations
(PoP) document [1]. "Readable" for myself would mean: I can easily
correlate the code to the text from the PoP so that I can spot errors or
adjust code to changed text.

I'm adding some examples how that could look like below. My question to
the original author would be, if this is something they could implement,
or if we'd rather do that at some point in time in the future by ourselves.

>> diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
>> index a7c3ccf681da..b7e65f96de3c 100644
>> --- a/arch/s390/include/asm/scsw.h
>> +++ b/arch/s390/include/asm/scsw.h
>> @@ -508,9 +508,13 @@ static inline int scsw_cmd_is_valid_zcc(union scsw *scsw)
>>   */
>>  static inline int scsw_cmd_is_valid_ectl(union scsw *scsw)
>>  {
>> -	return (scsw->cmd.stctl & SCSW_STCTL_STATUS_PEND) &&
>> -	       !(scsw->cmd.stctl & SCSW_STCTL_INTER_STATUS) &&
>> -	       (scsw->cmd.stctl & SCSW_STCTL_ALERT_STATUS);
>> +	if (!(scsw->tm.stctl & SCSW_STCTL_STATUS_PEND))
>> +		return 0;
>> +	if (scsw->tm.stctl & SCSW_STCTL_INTER_STATUS)
>> +		return 0;
>> +	if (scsw->tm.stctl & SCSW_STCTL_ALERT_STATUS)
>> +		return 1;
>> +	return 0;
>>  }

Here's the PoP text that is the base for this function (note ECTL=E):

"The E bit is meaningful whenever the subchannel is status pending with
alert status either alone or together with primary status, secondary
status, or both."

A readable version for me would therefore contain code that can easily
be matched against that text, e.g. something like:

/* Must be status pending. */
if (!(scsw->cmd.stctl & SCSW_STCTL_STATUS_PEND))
  return 0;

/* Must have alert status. */
if (!(scsw->cmd.stctl & SCSW_STCTL_ALERT_STATUS))
  return 0;

/* Must be alone or together with primary, secondary or both,
 * => no intermediate status. */
if (scsw->cmd.stctl & SCSW_STCTL_INTER_STATUS)
  return 0;

return 1;

>>  /**
>> @@ -522,10 +526,15 @@ static inline int scsw_cmd_is_valid_ectl(union scsw *scsw)
>>   */
>>  static inline int scsw_cmd_is_valid_pno(union scsw *scsw)
>>  {
>> -	return (scsw->cmd.fctl != 0) &&
>> -	       (scsw->cmd.stctl & SCSW_STCTL_STATUS_PEND) &&
>> -	       (!(scsw->cmd.stctl & SCSW_STCTL_INTER_STATUS) ||
>> -		  (scsw->cmd.actl & SCSW_ACTL_SUSPENDED));
>> +	if (!scsw->tm.fctl)
>> +		return 0;
>> +	if (!(scsw->tm.stctl & SCSW_STCTL_STATUS_PEND))
>> +		return 0;
>> +	if (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS))
>> +		return 1;
>> +	if (scsw->tm.actl & SCSW_ACTL_SUSPENDED)
>> +		return 1;
>> +	return 0;
>>  }

Here's the associated PoP text for this function (note: PNO=N)

"The N bit is meaningful whenever the status-control field contains any
of the indications listed below and at least one basic I/O function is
also indicated at the subchannel:
- Status pending with any combination of primary, secondary, or alert
  status
- Status pending alone
- Status pending with intermediate status when the subchannel is also
  suspended"

Again a readable version could look like:

/* Must indicate at least one I/O function. */
if (!scsw->cmd.fctl)
  return 0;

/* Must be status pending. */
if (!(scsw->cmd.stctl & SCSW_STCTL_STATUS_PEND))
  return 0;

/* Can be status pending alone, or with any combination of primary,
 * secondary and alert => no intermediate status. */
if (!(scsw->cmd.stctl & SCSW_STCTL_INTER_STATUS))
  return 1;

/* If intermediate, must be suspended. */
if (scsw->cmd.actl & SCSW_ACTL_SUSPENDED)
  return 1;

return 0;

The _tm_ functions below should be changed in the exact same way, while
accessing the corresponding data fields in scsw->tm instead of scsw->cmd.

>>  static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
>>  static inline int scsw_tm_is_valid_pno(union scsw *scsw)

[1] https://www.ibm.com/support/pages/zarchitecture-principles-operation

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Powered by blists - more mailing lists