From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id EB591A034F for ; Wed, 13 May 2020 13:13:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D20691D53D; Wed, 13 May 2020 13:13:40 +0200 (CEST) Received: from huawei.com (szxga01-in.huawei.com [45.249.212.187]) by dpdk.org (Postfix) with ESMTP id BFCD01D532; Wed, 13 May 2020 13:13:37 +0200 (CEST) Received: from DGGEMM406-HUB.china.huawei.com (unknown [172.30.72.54]) by Forcepoint Email with ESMTP id 84DB0DD99E8B43779A80; Wed, 13 May 2020 19:13:35 +0800 (CST) Received: from DGGEMM421-HUB.china.huawei.com (10.1.198.38) by DGGEMM406-HUB.china.huawei.com (10.3.20.214) with Microsoft SMTP Server (TLS) id 14.3.487.0; Wed, 13 May 2020 19:13:35 +0800 Received: from DGGEMM533-MBX.china.huawei.com ([169.254.5.111]) by dggemm421-hub.china.huawei.com ([10.1.198.38]) with mapi id 14.03.0487.000; Wed, 13 May 2020 19:13:27 +0800 From: wangyunjian To: Gagandeep Singh , "dev@dpdk.org" CC: Hemant Agrawal , "Lilijun (Jerry)" , xudingke , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd Thread-Index: AQHWJ03ohEpJ9z9gjUGvXuoEchaPkaijS86AgAD1MjCAAKNRAIAA+n6g Date: Wed, 13 May 2020 11:13:26 +0000 Message-ID: <34EFBCA9F01B0748BEB6B629CE643AE60CFDA9CF@DGGEMM533-MBX.china.huawei.com> References: <1589171838-18076-1-git-send-email-wangyunjian@huawei.com> <34EFBCA9F01B0748BEB6B629CE643AE60CFD82D5@DGGEMM533-MBX.china.huawei.com> In-Reply-To: Accept-Language: en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.173.251.152] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-CFilter-Loop: Reflected Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" > -----Original Message----- > From: Gagandeep Singh [mailto:G.Singh@nxp.com] > Sent: Wednesday, May 13, 2020 12:11 PM > To: wangyunjian ; dev@dpdk.org > Cc: Hemant Agrawal ; Lilijun (Jerry) > ; xudingke ; > stable@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd >=20 >=20 >=20 > > -----Original Message----- > > From: wangyunjian > > Sent: Tuesday, May 12, 2020 4:28 PM > > To: Gagandeep Singh ; dev@dpdk.org > > Cc: Hemant Agrawal ; Lilijun (Jerry) > > ; xudingke ; > > stable@dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of > > fd > > > > > -----Original Message----- > > > From: Gagandeep Singh [mailto:G.Singh@nxp.com] > > > Sent: Tuesday, May 12, 2020 11:49 AM > > > To: wangyunjian ; dev@dpdk.org > > > Cc: Hemant Agrawal ; Lilijun (Jerry) > > > ; xudingke ; > > > stable@dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check > > > of fd > > > > > > > > > > > > > -----Original Message----- > > > > From: wangyunjian > > > > Sent: Monday, May 11, 2020 10:07 AM > > > > To: dev@dpdk.org > > > > Cc: Gagandeep Singh ; Hemant Agrawal > > > > ; jerry.lilijun@huawei.com; > > > > xudingke@huawei.com; Yunjian Wang ; > > > > stable@dpdk.org > > > > Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of > > > > fd > > > > > > > > From: Yunjian Wang > > > > > > > > Zero is a valid fd. It will fail to check the fd if the fd is zero. > > > > The "job_ring->uio_fd" is an fd, so define it as "int". > > > > > > > > Fixes: e7a45f3cc245 ("crypto/caam_jr: add UIO specific > > > > operations") > > > > Fixes: a5e1018d5e67 ("crypto/caam_jr: add routines to configure > > > > HW") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: Yunjian Wang > > > > --- > > > > v2: > > > > * Change "job_ring->uio_fd" type suggested by Gagandeep Singh > > > > --- > > > > drivers/crypto/caam_jr/caam_jr.c | 6 ++++-- > > > > drivers/crypto/caam_jr/caam_jr_hw_specific.h | 2 +- > > > > drivers/crypto/caam_jr/caam_jr_pvt.h | 5 +++-- > > > > drivers/crypto/caam_jr/caam_jr_uio.c | 21 > ++++++++++++++------ > > > > 4 files changed, 23 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/crypto/caam_jr/caam_jr.c > > > > b/drivers/crypto/caam_jr/caam_jr.c > > > > index 5a29dd169..f62d46546 100644 > > > > --- a/drivers/crypto/caam_jr/caam_jr.c > > > > +++ b/drivers/crypto/caam_jr/caam_jr.c > > > > @@ -2074,7 +2074,7 @@ static struct rte_security_ops > > > > caam_jr_security_ops =3D { static void close_job_ring(struct > > > > sec_job_ring_t *job_ring) { > > > > - if (job_ring->irq_fd) { > > > > + if (job_ring->irq_fd !=3D -1) { > > > > /* Producer index is frozen. If consumer index is not equal > > > > * with producer index, then we have descs to flush. > > > > */ > > > > @@ -2187,7 +2187,7 @@ caam_jr_dev_uninit(struct rte_cryptodev > *dev) > > > > * > > > > */ > > > > static void * > > > > -init_job_ring(void *reg_base_addr, uint32_t irq_id) > > > > +init_job_ring(void *reg_base_addr, int irq_id) > > > > { > > > > struct sec_job_ring_t *job_ring =3D NULL; > > > > int i, ret =3D 0; > > > > @@ -2466,6 +2466,8 @@ > > > > RTE_PMD_REGISTER_CRYPTO_DRIVER(caam_jr_crypto_drv, > > > > cryptodev_caam_jr_drv.driver, > > > > > > > > RTE_INIT(caam_jr_init_log) > > > > { > > > > + sec_init(); > > > > + > > > Why are you calling a fd initialization function in log registration > constructor? > > > This is not the right place for any initialization other than logs. > > > Also, sec_init function will not work here, as you are using > "g_uio_jr_num" > > > whose value will always be 0 during this function call. > > > > Thanks, my bad. I should use 'MAX_SEC_JOB_RINGS'. > > The default values of 'g_job_rings' and 'g_uio_job_ring' are 0. > > As a result, the validity cannot be checked. what about following: > > > > void sec_uio_job_rings_init(void) > > { > > int i; > > > > for (i =3D 0; i < MAX_SEC_JOB_RINGS; i++) > > g_uio_job_ring[i].uio_fd =3D -1; > > } > > > > void sec_job_rings_init(void) > > { > > int i; > > > > for (i =3D 0; i < MAX_SEC_JOB_RINGS; i++) > > g_job_rings[i].irq_fd =3D -1; > > } > > > > RTE_INIT(caam_jr_init) > > { > > sec_uio_job_rings_init(); > > sec_job_rings_init(); > > } > > > This looks ok to me. You can combine both init functions into single one. OK, the two variables are defined in different files, so cannot be combined= .=20 Thanks, Yunjian >=20 > > > > > > > caam_jr_logtype =3D rte_log_register("pmd.crypto.caam"); > > > > if (caam_jr_logtype >=3D 0) > > > > rte_log_set_level(caam_jr_logtype, RTE_LOG_NOTICE); diff -- > > git > > > > a/drivers/crypto/caam_jr/caam_jr_hw_specific.h > > > > b/drivers/crypto/caam_jr/caam_jr_hw_specific.h > > > > index 5f58a585d..ec4539d1b 100644 > > > > --- a/drivers/crypto/caam_jr/caam_jr_hw_specific.h > > > > +++ b/drivers/crypto/caam_jr/caam_jr_hw_specific.h > > > > @@ -360,7 +360,7 @@ struct sec_job_ring_t { > > > > * bitwise operations. > > > > */ > > > > > > > > - uint32_t irq_fd; /* The file descriptor used for polling from > > > > + int irq_fd; /* The file descriptor used for polling from > > > > * user space for interrupts notifications > > > > */ > > > > uint32_t jr_mode; /* Model used by SEC Driver to receive > > > > diff --git a/drivers/crypto/caam_jr/caam_jr_pvt.h > > > > b/drivers/crypto/caam_jr/caam_jr_pvt.h > > > > index 98cd4438a..fa9e86ea6 100644 > > > > --- a/drivers/crypto/caam_jr/caam_jr_pvt.h > > > > +++ b/drivers/crypto/caam_jr/caam_jr_pvt.h > > > > @@ -216,16 +216,17 @@ calc_chksum(void *buffer, int len) } > > > > struct uio_job_ring { > > > > uint32_t jr_id; > > > > - uint32_t uio_fd; > > > > + int uio_fd; > > > > void *register_base_addr; > > > > int map_size; > > > > int uio_minor_number; > > > > }; > > > > > > > > int sec_cleanup(void); > > > > +void sec_init(void); > > > > int sec_configure(void); > > > > struct uio_job_ring *config_job_ring(void); -void > > > > free_job_ring(uint32_t uio_fd); > > > > +void free_job_ring(int uio_fd); > > > > > > > > /* For Dma memory allocation of specified length and alignment */ > > > > static inline void * diff --git > > > > a/drivers/crypto/caam_jr/caam_jr_uio.c > > > > b/drivers/crypto/caam_jr/caam_jr_uio.c > > > > index b1bb44ca4..133b32084 100644 > > > > --- a/drivers/crypto/caam_jr/caam_jr_uio.c > > > > +++ b/drivers/crypto/caam_jr/caam_jr_uio.c > > > > @@ -145,7 +145,7 @@ file_read_first_line(const char root[], const > > > > char subdir[], > > > > "%s/%s/%s", root, subdir, filename); > > > > > > > > fd =3D open(absolute_file_name, O_RDONLY); > > > > - SEC_ASSERT(fd > 0, fd, "Error opening file %s", > > > > + SEC_ASSERT(fd >=3D 0, fd, "Error opening file %s", > > > > absolute_file_name); > > > > > > > > /* read UIO device name from first line in file */ @@ -322,7 > > > > +322,7 @@ uio_map_registers(int uio_device_fd, int uio_device_id, > > > > } > > > > > > > > void > > > > -free_job_ring(uint32_t uio_fd) > > > > +free_job_ring(int uio_fd) > > > > { > > > > struct uio_job_ring *job_ring =3D NULL; > > > > int i; > > > > > > Please update the check "if (!uio_fd)". > > > > OK, I will fix it. > > > > > > > > > @@ -347,7 +347,7 @@ free_job_ring(uint32_t uio_fd) > > > > job_ring->jr_id, job_ring->uio_fd); > > > > close(job_ring->uio_fd); > > > > g_uio_jr_num--; > > > > - job_ring->uio_fd =3D 0; > > > > + job_ring->uio_fd =3D -1; > > > > if (job_ring->register_base_addr =3D=3D NULL) > > > > return; > > > > > > > > @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void) > > > > int i; > > > > > > > > for (i =3D 0; i < MAX_SEC_JOB_RINGS; i++) { > > > > - if (g_uio_job_ring[i].uio_fd =3D=3D 0) { > > > > + if (g_uio_job_ring[i].uio_fd =3D=3D -1) { > > > > job_ring =3D &g_uio_job_ring[i]; > > > > g_uio_jr_num++; > > > > break; > > > > @@ -389,7 +389,7 @@ uio_job_ring *config_job_ring(void) > > > > > > > > /* Open device file */ > > > > job_ring->uio_fd =3D open(uio_device_file_name, O_RDWR); > > > > - SEC_ASSERT(job_ring->uio_fd > 0, NULL, > > > > + SEC_ASSERT(job_ring->uio_fd >=3D 0, NULL, > > > > "Failed to open UIO device file for job ring %d", > > > > job_ring->jr_id); > > > > > > > > @@ -488,12 +488,21 @@ sec_cleanup(void) > > > > /* I need to close the fd after shutdown UIO commands need to > > be > > > > * sent using the fd > > > > */ > > > > - if (job_ring->uio_fd !=3D 0) { > > > > + if (job_ring->uio_fd !=3D -1) { > > > > CAAM_JR_INFO( > > > > "Closed device file for job ring %d , fd =3D %d", > > > > job_ring->jr_id, job_ring->uio_fd); > > > > close(job_ring->uio_fd); > > > > + job_ring->uio_fd =3D -1; > > > > } > > > > } > > > > return 0; > > > > } > > > > + > > > > +void sec_init(void) > > > > +{ > > > > + int i; > > > > + > > > > + for (i =3D 0; i < g_uio_jr_num; i++) > > > > + g_uio_job_ring[i].uio_fd =3D -1; > > > > +} > > > > -- > > > > 2.19.1 > > > > > > > > > > There are still some functions(sec_uio_send_command, > > > caam_jr_enable_irqs, > > > caam_jr_disable_irqs) in caam_jr_uio.c, which are accepting uio_fd > > > parameter as uint32_t, please update them also. > > > > > I will update them in next version. > >