Fix the following coccicheck warnings:
./arch/s390/include/asm/scsw.h:695:47-49: WARNING
!A || A && B is equivalent to !A || B
Signed-off-by: Haowen Bai <[email protected]>
---
arch/s390/include/asm/scsw.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
index a7c3ccf..f2baac8 100644
--- a/arch/s390/include/asm/scsw.h
+++ b/arch/s390/include/asm/scsw.h
@@ -692,8 +692,7 @@ static inline int scsw_tm_is_valid_pno(union scsw *scsw)
return (scsw->tm.fctl != 0) &&
(scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
(!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
- ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
- (scsw->tm.actl & SCSW_ACTL_SUSPENDED)));
+ (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
}
/**
--
2.7.4
Hi Haowen,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on s390/features]
[also build test ERROR on v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Haowen-Bai/s390-Simplify-the-calculation-of-variables/20220328-103807
base: https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-randconfig-r044-20220327 (https://download.01.org/0day-ci/archive/20220328/[email protected]/config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7356660d5a4a5dc70f5e18b63125c43d218885f3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Haowen-Bai/s390-Simplify-the-calculation-of-variables/20220328-103807
git checkout 7356660d5a4a5dc70f5e18b63125c43d218885f3
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=s390 SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from arch/s390/include/asm/cio.h:18,
from arch/s390/include/asm/chpid.h:10,
from arch/s390/include/asm/sclp.h:20,
from arch/s390/include/asm/pci.h:12,
from include/linux/pci.h:1901,
from arch/s390/include/asm/hw_irq.h:6,
from include/linux/irq.h:592,
from include/linux/iio/trigger.h:6,
from drivers/iio/accel/adxl355_core.c:14:
arch/s390/include/asm/scsw.h: In function 'scsw_tm_is_valid_pno':
>> arch/s390/include/asm/scsw.h:695:54: error: expected ';' before '}' token
695 | (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
| ^
| ;
696 | }
| ~
--
In file included from arch/s390/include/asm/cio.h:18,
from arch/s390/include/asm/chpid.h:10,
from arch/s390/include/asm/sclp.h:20,
from arch/s390/include/asm/pci.h:12,
from include/linux/pci.h:1901,
from arch/s390/include/asm/hw_irq.h:6,
from include/linux/irq.h:592,
from include/linux/iio/trigger.h:6,
from drivers/iio/accel/kxcjk-1013.c:21:
arch/s390/include/asm/scsw.h: In function 'scsw_tm_is_valid_pno':
>> arch/s390/include/asm/scsw.h:695:54: error: expected ';' before '}' token
695 | (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
| ^
| ;
696 | }
| ~
At top level:
drivers/iio/accel/kxcjk-1013.c:1709:36: warning: 'kx_acpi_match' defined but not used [-Wunused-const-variable=]
1709 | static const struct acpi_device_id kx_acpi_match[] = {
| ^~~~~~~~~~~~~
--
In file included from arch/s390/include/asm/cio.h:18,
from arch/s390/include/asm/chpid.h:10,
from arch/s390/include/asm/sclp.h:20,
from arch/s390/include/asm/pci.h:12,
from include/linux/pci.h:1901,
from arch/s390/include/asm/hw_irq.h:6,
from include/linux/irq.h:592,
from include/linux/iio/trigger.h:6,
from drivers/iio/accel/mxc4005.c:14:
arch/s390/include/asm/scsw.h: In function 'scsw_tm_is_valid_pno':
>> arch/s390/include/asm/scsw.h:695:54: error: expected ';' before '}' token
695 | (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
| ^
| ;
696 | }
| ~
At top level:
drivers/iio/accel/mxc4005.c:473:36: warning: 'mxc4005_acpi_match' defined but not used [-Wunused-const-variable=]
473 | static const struct acpi_device_id mxc4005_acpi_match[] = {
| ^~~~~~~~~~~~~~~~~~
--
In file included from arch/s390/include/asm/cio.h:18,
from arch/s390/include/asm/chpid.h:10,
from arch/s390/include/asm/sclp.h:20,
from arch/s390/include/asm/pci.h:12,
from include/linux/pci.h:1901,
from arch/s390/include/asm/hw_irq.h:6,
from include/linux/irq.h:592,
from include/linux/iio/trigger.h:6,
from drivers/iio/accel/stk8ba50.c:18:
arch/s390/include/asm/scsw.h: In function 'scsw_tm_is_valid_pno':
>> arch/s390/include/asm/scsw.h:695:54: error: expected ';' before '}' token
695 | (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
| ^
| ;
696 | }
| ~
At top level:
drivers/iio/accel/stk8ba50.c:539:36: warning: 'stk8ba50_acpi_id' defined but not used [-Wunused-const-variable=]
539 | static const struct acpi_device_id stk8ba50_acpi_id[] = {
| ^~~~~~~~~~~~~~~~
vim +695 arch/s390/include/asm/scsw.h
682
683 /**
684 * scsw_tm_is_valid_pno - check pno field validity
685 * @scsw: pointer to scsw
686 *
687 * Return non-zero if the pno field of the specified transport mode scsw is
688 * valid, zero otherwise.
689 */
690 static inline int scsw_tm_is_valid_pno(union scsw *scsw)
691 {
692 return (scsw->tm.fctl != 0) &&
693 (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
694 (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
> 695 (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
696 }
697
--
0-DAY CI Kernel Test Service
https://01.org/lkp
From: Haowen Bai
> Sent: 28 March 2022 03:36
>
> Fix the following coccicheck warnings:
> ./arch/s390/include/asm/scsw.h:695:47-49: WARNING
> !A || A && B is equivalent to !A || B
>
> Signed-off-by: Haowen Bai <[email protected]>
> ---
> arch/s390/include/asm/scsw.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
> index a7c3ccf..f2baac8 100644
> --- a/arch/s390/include/asm/scsw.h
> +++ b/arch/s390/include/asm/scsw.h
> @@ -692,8 +692,7 @@ static inline int scsw_tm_is_valid_pno(union scsw *scsw)
> return (scsw->tm.fctl != 0) &&
> (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
> (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
> - ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
> - (scsw->tm.actl & SCSW_ACTL_SUSPENDED)));
> + (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
> }
I'd split that impenetrable boolean expression up.
I think this is equivalent:
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;
The generated code could even be the same.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Mar 28, 2022 at 12:15:49PM +0000, David Laight wrote:
> From: Haowen Bai
> > Sent: 28 March 2022 03:36
> >
> > Fix the following coccicheck warnings:
> > ./arch/s390/include/asm/scsw.h:695:47-49: WARNING
> > !A || A && B is equivalent to !A || B
> >
> > Signed-off-by: Haowen Bai <[email protected]>
> > ---
> > arch/s390/include/asm/scsw.h | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
> > index a7c3ccf..f2baac8 100644
> > --- a/arch/s390/include/asm/scsw.h
> > +++ b/arch/s390/include/asm/scsw.h
> > @@ -692,8 +692,7 @@ static inline int scsw_tm_is_valid_pno(union scsw *scsw)
> > return (scsw->tm.fctl != 0) &&
> > (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
> > (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
> > - ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
> > - (scsw->tm.actl & SCSW_ACTL_SUSPENDED)));
> > + (scsw->tm.actl & SCSW_ACTL_SUSPENDED))
> > }
>
> I'd split that impenetrable boolean expression up.
>
> I think this is equivalent:
> 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;
>
> The generated code could even be the same.
Yes, we had the very same discussion here:
https://lore.kernel.org/linux-s390/[email protected]/
Where the outcome also was that it doesn't make sense to replace one
unreadable version with another unreadable version just to get rid of
a warning.
Haowen, could you please resend with a proper readable version, or
alternatively, Vineeth, could you address this please, so this doesn't
come up again?
Thanks!
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 <[email protected]>
---
V1->V2: apply a readable and simple version as suggestion.
arch/s390/include/asm/scsw.h | 47 ++++++++++++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 15 deletions(-)
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;
}
/**
@@ -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;
}
/**
@@ -675,9 +684,13 @@ static inline int scsw_tm_is_valid_q(union scsw *scsw)
*/
static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
{
- return (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
- !(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
- (scsw->tm.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;
}
/**
@@ -689,11 +702,15 @@ static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
*/
static inline int scsw_tm_is_valid_pno(union scsw *scsw)
{
- return (scsw->tm.fctl != 0) &&
- (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
- (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
- ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
- (scsw->tm.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;
}
/**
--
2.7.4
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 <[email protected]>
> ---
> 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?
Thank you!
> 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;
> }
>
> /**
> @@ -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;
> }
>
> /**
> @@ -675,9 +684,13 @@ static inline int scsw_tm_is_valid_q(union scsw *scsw)
> */
> static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
> {
> - return (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
> - !(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
> - (scsw->tm.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;
> }
>
> /**
> @@ -689,11 +702,15 @@ static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
> */
> static inline int scsw_tm_is_valid_pno(union scsw *scsw)
> {
> - return (scsw->tm.fctl != 0) &&
> - (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
> - (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
> - ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
> - (scsw->tm.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;
> }
>
> /**
> --
> 2.7.4
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
---
V1->V2: apply a readable and simple version as suggestion.
V2->V3: scsw->tm instead of scsw->cmd; fix logic flow as suggestion.
arch/s390/include/asm/scsw.h | 79 +++++++++++++++++++++++++++++++++++---------
1 file changed, 64 insertions(+), 15 deletions(-)
diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
index a7c3ccf681da..5cb893706d66 100644
--- a/arch/s390/include/asm/scsw.h
+++ b/arch/s390/include/asm/scsw.h
@@ -508,9 +508,21 @@ 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);
+ /* 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 +533,25 @@ 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));
+ /* 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;
}
/**
@@ -675,9 +700,21 @@ static inline int scsw_tm_is_valid_q(union scsw *scsw)
*/
static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
{
- return (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
- !(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
- (scsw->tm.stctl & SCSW_STCTL_ALERT_STATUS);
+ /* Must be status pending. */
+ if (!(scsw->tm.stctl & SCSW_STCTL_STATUS_PEND))
+ return 0;
+
+ /* Must have alert status. */
+ if (!(scsw->tm.stctl & SCSW_STCTL_ALERT_STATUS))
+ return 0;
+
+ /* Must be alone or together with primary, secondary or both,
+ * => no intermediate status.
+ */
+ if (scsw->tm.stctl & SCSW_STCTL_INTER_STATUS)
+ return 0;
+
+ return 1;
}
/**
@@ -689,11 +725,25 @@ static inline int scsw_tm_is_valid_ectl(union scsw *scsw)
*/
static inline int scsw_tm_is_valid_pno(union scsw *scsw)
{
- return (scsw->tm.fctl != 0) &&
- (scsw->tm.stctl & SCSW_STCTL_STATUS_PEND) &&
- (!(scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) ||
- ((scsw->tm.stctl & SCSW_STCTL_INTER_STATUS) &&
- (scsw->tm.actl & SCSW_ACTL_SUSPENDED)));
+ /* Must indicate at least one I/O function. */
+ if (!scsw->tm.fctl)
+ return 0;
+
+ /* Must be status pending. */
+ if (!(scsw->tm.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->tm.stctl & SCSW_STCTL_INTER_STATUS))
+ return 1;
+
+ /* If intermediate, must be suspended. */
+ if (scsw->tm.actl & SCSW_ACTL_SUSPENDED)
+ return 1;
+
+ return 0;
}
/**
--
2.7.4
在 4/7/22 12:44 AM, Peter Oberparleiter 写道:
> 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 <[email protected]>
>>> ---
>>> 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
>
Dear Peter Oberparleiter
Well done. Thank you for your kindly and professional explain what we need to do best.
--
Haowen Bai
On Fri, Apr 08, 2022 at 02:53:02PM +0200, Peter Oberparleiter wrote:
> On 07.04.2022 04:16, 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 <[email protected]>
>
> A definitive improvement over the current version. Thanks!
>
> Reviewed-by: Peter Oberparleiter <[email protected]>
Applied, thanks!
On 07.04.2022 04:16, 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 <[email protected]>
A definitive improvement over the current version. Thanks!
Reviewed-by: Peter Oberparleiter <[email protected]>
--
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D