Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 2 Mar 2020 11:51:03 +0000
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Zaibo Xu <xuzaibo@huawei.com>
CC:     <herbert@gondor.apana.org.au>, <davem@davemloft.net>,
        <qianweili@huawei.com>, <tanghui20@huawei.com>,
        <forest.zhouchang@huawei.com>, <linuxarm@huawei.com>,
        <zhangwei375@huawei.com>, <yekai13@huawei.com>,
        <linux-crypto@vger.kernel.org>
Subject: Re: [PATCH v2 2/5] crypto: hisilicon/sec2 - Add workqueue for SEC
 driver.
Message-ID: <20200302115103.00005d06@Huawei.com>
In-Reply-To: <1583129716-28382-3-git-send-email-xuzaibo@huawei.com>
References: <1583129716-28382-1-git-send-email-xuzaibo@huawei.com>
        <1583129716-28382-3-git-send-email-xuzaibo@huawei.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Mon, 2 Mar 2020 14:15:13 +0800
Zaibo Xu <xuzaibo@huawei.com> wrote:

> From: yekai13 <yekai13@huawei.com>
> 
> Allocate one workqueue for each QM instead of one for all QMs,
> we found the throughput of SEC engine can be increased to
> the hardware limit throughput during testing sec2 performance.
> so we added this scheme.
> 
> Signed-off-by: yekai13 <yekai13@huawei.com>
> Signed-off-by: liulongfang <liulongfang@huawei.com>

That first sign off needs fixing.  Needs to be a real name.

Also missing xuzaibo's sign offf.

A question inline that might be worth a follow up patch.

With signoffs fixed

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

> ---
>  drivers/crypto/hisilicon/sec2/sec_main.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/hisilicon/sec2/sec_main.c b/drivers/crypto/hisilicon/sec2/sec_main.c
> index 3767fdb..ebafc1c 100644
> --- a/drivers/crypto/hisilicon/sec2/sec_main.c
> +++ b/drivers/crypto/hisilicon/sec2/sec_main.c
> @@ -774,12 +774,24 @@ static void sec_qm_uninit(struct hisi_qm *qm)
>  
>  static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec)
>  {
> +	int ret;
> +
> +	qm->wq = alloc_workqueue("%s", WQ_HIGHPRI | WQ_CPU_INTENSIVE |
> +		WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus(),
> +		pci_name(qm->pdev));

I appreciate that you have the same parameters here as were originally in
qm.c, but I would like to fully understand why some of these flags are set.

Perhaps a comment for each of them?  I'm not sure I'd consider the work
to be done in this work queue CPU_INTENSIVE for example.

This could be a follow up patch though as not actually related to this
change.

> +	if (!qm->wq) {
> +		pci_err(qm->pdev, "fail to alloc workqueue\n");
> +		return -ENOMEM;
> +	}
> +
>  	if (qm->fun_type == QM_HW_PF) {
>  		qm->qp_base = SEC_PF_DEF_Q_BASE;
>  		qm->qp_num = pf_q_num;
>  		qm->debug.curr_qm_qp_num = pf_q_num;
>  
> -		return sec_pf_probe_init(sec);
> +		ret = sec_pf_probe_init(sec);
> +		if (ret)
> +			goto err_probe_uninit;
>  	} else if (qm->fun_type == QM_HW_VF) {
>  		/*
>  		 * have no way to get qm configure in VM in v1 hardware,
> @@ -792,18 +804,26 @@ static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec)
>  			qm->qp_num = SEC_QUEUE_NUM_V1 - SEC_PF_DEF_Q_NUM;
>  		} else if (qm->ver == QM_HW_V2) {
>  			/* v2 starts to support get vft by mailbox */
> -			return hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num);
> +			ret = hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num);
> +			if (ret)
> +				goto err_probe_uninit;
>  		}
>  	} else {
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto err_probe_uninit;
>  	}
>  
>  	return 0;
> +err_probe_uninit:
> +	destroy_workqueue(qm->wq);
> +	return ret;
>  }
>  
>  static void sec_probe_uninit(struct hisi_qm *qm)
>  {
>  	hisi_qm_dev_err_uninit(qm);
> +
> +	destroy_workqueue(qm->wq);
>  }
>  
>  static int sec_probe(struct pci_dev *pdev, const struct pci_device_id *id)