linux-kernel - Re: [PATCH] crypto: qat - remove redundant condition accel_dev->is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 25 Oct 2019 11:33:59 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Yunfeng Ye <yeyunfeng@...wei.com>
Cc:     Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>, qat-linux@...el.com,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "hushiyuan@...wei.com" <hushiyuan@...wei.com>,
        "linfeilong@...wei.com" <linfeilong@...wei.com>
Subject: Re: [PATCH] crypto: qat - remove redundant condition accel_dev->is_vf

On Fri, 25 Oct 2019 at 09:24, Yunfeng Ye <yeyunfeng@...wei.com> wrote:
>
> Warning is found by the code analysis tool:
>   "Redundant condition: accel_dev->is_vf"
>
> So remove the redundant condition accel_dev->is_vf.
>
> Signed-off-by: Yunfeng Ye <yeyunfeng@...wei.com>
> ---
>  drivers/crypto/qat/qat_common/adf_dev_mgr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/qat/qat_common/adf_dev_mgr.c b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
> index 2d06409bd3c4..b54b8850fe20 100644
> --- a/drivers/crypto/qat/qat_common/adf_dev_mgr.c
> +++ b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
> @@ -196,7 +196,7 @@ int adf_devmgr_add_dev(struct adf_accel_dev *accel_dev,
>         atomic_set(&accel_dev->ref_count, 0);
>
>         /* PF on host or VF on guest */
> -       if (!accel_dev->is_vf || (accel_dev->is_vf && !pf)) {
> +       if (!accel_dev->is_vf || !pf) {

I disagree with this change. There is no bug here, and the way the
condition is formulated self-documents the code, i.e.,

IF NOT is_vf
OR (is_vf BUT NOT pf)

Using an automated tool to reduce every boolean expression to its
minimal representation doesn't seem that useful to me, since the
compiler is perfectly capable of doing that when generating the object
code.




>                 struct vf_id_map *map;
>
>                 list_for_each(itr, &accel_table) {
> @@ -292,7 +292,7 @@ void adf_devmgr_rm_dev(struct adf_accel_dev *accel_dev,
>                        struct adf_accel_dev *pf)
>  {
>         mutex_lock(&table_lock);
> -       if (!accel_dev->is_vf || (accel_dev->is_vf && !pf)) {
> +       if (!accel_dev->is_vf || !pf) {
>                 id_map[accel_dev->accel_id] = 0;
>                 num_devices--;
>         } else if (accel_dev->is_vf && pf) {
> --
> 2.7.4
>