* Re: [PATCH] s390: Simplify the calculation of variables
2022-03-28 2:36 [PATCH] s390: Simplify the calculation of variables Haowen Bai
@ 2022-03-28 5:37 ` kernel test robot
2022-03-28 6:08 ` kernel test robot
2022-03-28 12:15 ` David Laight
2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2022-03-28 5:37 UTC (permalink / raw)
To: Haowen Bai, hca, gor, agordeev, borntraeger, svens
Cc: llvm, kbuild-all, linux-s390, linux-kernel, Haowen Bai
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-20220328 (https://download.01.org/0day-ci/archive/20220328/202203281319.E0AKjDcq-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
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
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# 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=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/nfc/nfcmrvl/main.c:9:
In file included from include/linux/gpio.h:62:
In file included from include/asm-generic/gpio.h:11:
In file included from include/linux/gpio/driver.h:7:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/nfc/nfcmrvl/main.c:9:
In file included from include/linux/gpio.h:62:
In file included from include/asm-generic/gpio.h:11:
In file included from include/linux/gpio/driver.h:7:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/nfc/nfcmrvl/main.c:9:
In file included from include/linux/gpio.h:62:
In file included from include/asm-generic/gpio.h:11:
In file included from include/linux/gpio/driver.h:7:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from drivers/nfc/nfcmrvl/main.c:9:
In file included from include/linux/gpio.h:62:
In file included from include/asm-generic/gpio.h:11:
In file included from include/linux/gpio/driver.h:7:
In file included from include/linux/irq.h:592:
In file included from arch/s390/include/asm/hw_irq.h:6:
In file included from include/linux/pci.h:1901:
In file included from arch/s390/include/asm/pci.h:12:
In file included from arch/s390/include/asm/sclp.h:20:
In file included from arch/s390/include/asm/chpid.h:10:
In file included from arch/s390/include/asm/cio.h:18:
>> arch/s390/include/asm/scsw.h:695:47: error: expected ';' after return statement
(scsw->tm.actl & SCSW_ACTL_SUSPENDED))
^
;
12 warnings and 1 error generated.
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] s390: Simplify the calculation of variables
2022-03-28 2:36 [PATCH] s390: Simplify the calculation of variables Haowen Bai
2022-03-28 5:37 ` kernel test robot
@ 2022-03-28 6:08 ` kernel test robot
2022-03-28 12:15 ` David Laight
2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2022-03-28 6:08 UTC (permalink / raw)
To: Haowen Bai, hca, gor, agordeev, borntraeger, svens
Cc: kbuild-all, linux-s390, linux-kernel, Haowen Bai
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/202203281418.KxPHz4Ek-lkp@intel.com/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 <lkp@intel.com>
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
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH] s390: Simplify the calculation of variables
2022-03-28 2:36 [PATCH] s390: Simplify the calculation of variables Haowen Bai
2022-03-28 5:37 ` kernel test robot
2022-03-28 6:08 ` kernel test robot
@ 2022-03-28 12:15 ` David Laight
2022-04-04 9:10 ` Heiko Carstens
2 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2022-03-28 12:15 UTC (permalink / raw)
To: 'Haowen Bai', hca, gor, agordeev, borntraeger, svens
Cc: linux-s390, linux-kernel
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 <baihaowen@meizu.com>
> ---
> 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] s390: Simplify the calculation of variables
2022-03-28 12:15 ` David Laight
@ 2022-04-04 9:10 ` Heiko Carstens
2022-04-06 2:37 ` [PATCH V2] " Haowen Bai
0 siblings, 1 reply; 12+ messages in thread
From: Heiko Carstens @ 2022-04-04 9:10 UTC (permalink / raw)
To: David Laight
Cc: 'Haowen Bai',
gor, agordeev, borntraeger, svens, linux-s390, linux-kernel,
Vineeth Vijayan
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 <baihaowen@meizu.com>
> > ---
> > 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/20210820025159.11914-1-jing.yangyang@zte.com.cn/
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] s390: Simplify the calculation of variables
2022-04-04 9:10 ` Heiko Carstens
@ 2022-04-06 2:37 ` Haowen Bai
2022-04-06 8:45 ` Heiko Carstens
0 siblings, 1 reply; 12+ messages in thread
From: Haowen Bai @ 2022-04-06 2:37 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle
Cc: Haowen Bai, linux-s390, linux-kernel
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@meizu.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(-)
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V2] s390: Simplify the calculation of variables
2022-04-06 2:37 ` [PATCH V2] " Haowen Bai
@ 2022-04-06 8:45 ` Heiko Carstens
2022-04-06 16:44 ` Peter Oberparleiter
0 siblings, 1 reply; 12+ messages in thread
From: Heiko Carstens @ 2022-04-06 8:45 UTC (permalink / raw)
To: Haowen Bai, Peter Oberparleiter, Vineeth Vijayan
Cc: Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, linux-s390, linux-kernel
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@meizu.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?
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V2] s390: Simplify the calculation of variables
2022-04-06 8:45 ` Heiko Carstens
@ 2022-04-06 16:44 ` Peter Oberparleiter
2022-04-07 2:16 ` [PATCH V3] " Haowen Bai
2022-04-07 2:19 ` [PATCH V2] " baihaowen
0 siblings, 2 replies; 12+ messages in thread
From: Peter Oberparleiter @ 2022-04-06 16:44 UTC (permalink / raw)
To: Heiko Carstens, Haowen Bai, Vineeth Vijayan
Cc: Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, linux-s390, linux-kernel
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@meizu.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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH V3] s390: Simplify the calculation of variables
2022-04-06 16:44 ` Peter Oberparleiter
@ 2022-04-07 2:16 ` Haowen Bai
2022-04-08 12:53 ` Peter Oberparleiter
2022-04-07 2:19 ` [PATCH V2] " baihaowen
1 sibling, 1 reply; 12+ messages in thread
From: Haowen Bai @ 2022-04-07 2:16 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle
Cc: Haowen Bai, linux-s390, linux-kernel
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@meizu.com>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V3] s390: Simplify the calculation of variables
2022-04-07 2:16 ` [PATCH V3] " Haowen Bai
@ 2022-04-08 12:53 ` Peter Oberparleiter
2022-04-08 14:32 ` Heiko Carstens
0 siblings, 1 reply; 12+ messages in thread
From: Peter Oberparleiter @ 2022-04-08 12:53 UTC (permalink / raw)
To: Haowen Bai, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Vineeth Vijayan
Cc: linux-s390, linux-kernel
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 <baihaowen@meizu.com>
A definitive improvement over the current version. Thanks!
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
--
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3] s390: Simplify the calculation of variables
2022-04-08 12:53 ` Peter Oberparleiter
@ 2022-04-08 14:32 ` Heiko Carstens
0 siblings, 0 replies; 12+ messages in thread
From: Heiko Carstens @ 2022-04-08 14:32 UTC (permalink / raw)
To: Peter Oberparleiter
Cc: Haowen Bai, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Vineeth Vijayan,
linux-s390, linux-kernel
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 <baihaowen@meizu.com>
>
> A definitive improvement over the current version. Thanks!
>
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Applied, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] s390: Simplify the calculation of variables
2022-04-06 16:44 ` Peter Oberparleiter
2022-04-07 2:16 ` [PATCH V3] " Haowen Bai
@ 2022-04-07 2:19 ` baihaowen
1 sibling, 0 replies; 12+ messages in thread
From: baihaowen @ 2022-04-07 2:19 UTC (permalink / raw)
To: Peter Oberparleiter, Heiko Carstens, Vineeth Vijayan
Cc: Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, linux-s390, linux-kernel
在 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 <baihaowen@meizu.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
>
Dear Peter Oberparleiter
Well done. Thank you for your kindly and professional explain what we need to do best.
--
Haowen Bai
^ permalink raw reply [flat|nested] 12+ messages in thread