[v2] crypto/caam_jr: fix wrong check of fd
Checks
Commit Message
From: Yunjian Wang <wangyunjian@huawei.com>
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 <wangyunjian@huawei.com>
---
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(-)
Comments
> -----Original Message-----
> From: wangyunjian <wangyunjian@huawei.com>
> Sent: Monday, May 11, 2020 10:07 AM
> To: dev@dpdk.org
> Cc: Gagandeep Singh <G.Singh@nxp.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; jerry.lilijun@huawei.com;
> xudingke@huawei.com; Yunjian Wang <wangyunjian@huawei.com>;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
>
> From: Yunjian Wang <wangyunjian@huawei.com>
>
> 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 <wangyunjian@huawei.com>
> ---
> 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 =
> {
> static void
> close_job_ring(struct sec_job_ring_t *job_ring)
> {
> - if (job_ring->irq_fd) {
> + if (job_ring->irq_fd != -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 = NULL;
> int i, ret = 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.
> caam_jr_logtype = rte_log_register("pmd.crypto.caam");
> if (caam_jr_logtype >= 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 = open(absolute_file_name, O_RDONLY);
> - SEC_ASSERT(fd > 0, fd, "Error opening file %s",
> + SEC_ASSERT(fd >= 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 = NULL;
> int i;
Please update the check "if (!uio_fd)".
> @@ -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 = 0;
> + job_ring->uio_fd = -1;
> if (job_ring->register_base_addr == NULL)
> return;
>
> @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
> int i;
>
> for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
> - if (g_uio_job_ring[i].uio_fd == 0) {
> + if (g_uio_job_ring[i].uio_fd == -1) {
> job_ring = &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 = open(uio_device_file_name, O_RDWR);
> - SEC_ASSERT(job_ring->uio_fd > 0, NULL,
> + SEC_ASSERT(job_ring->uio_fd >= 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 != 0) {
> + if (job_ring->uio_fd != -1) {
> CAAM_JR_INFO(
> "Closed device file for job ring %d , fd = %d",
> job_ring->jr_id, job_ring->uio_fd);
> close(job_ring->uio_fd);
> + job_ring->uio_fd = -1;
> }
> }
> return 0;
> }
> +
> +void sec_init(void)
> +{
> + int i;
> +
> + for (i = 0; i < g_uio_jr_num; i++)
> + g_uio_job_ring[i].uio_fd = -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.
> -----Original Message-----
> From: Gagandeep Singh [mailto:G.Singh@nxp.com]
> Sent: Tuesday, May 12, 2020 11:49 AM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
>
>
>
> > -----Original Message-----
> > From: wangyunjian <wangyunjian@huawei.com>
> > Sent: Monday, May 11, 2020 10:07 AM
> > To: dev@dpdk.org
> > Cc: Gagandeep Singh <G.Singh@nxp.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; jerry.lilijun@huawei.com;
> > xudingke@huawei.com; Yunjian Wang <wangyunjian@huawei.com>;
> > stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
> >
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > 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 <wangyunjian@huawei.com>
> > ---
> > 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 = { static void close_job_ring(struct
> > sec_job_ring_t *job_ring) {
> > - if (job_ring->irq_fd) {
> > + if (job_ring->irq_fd != -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 = NULL;
> > int i, ret = 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 = 0; i < MAX_SEC_JOB_RINGS; i++)
g_uio_job_ring[i].uio_fd = -1;
}
void sec_job_rings_init(void)
{
int i;
for (i = 0; i < MAX_SEC_JOB_RINGS; i++)
g_job_rings[i].irq_fd = -1;
}
RTE_INIT(caam_jr_init)
{
sec_uio_job_rings_init();
sec_job_rings_init();
}
>
> > caam_jr_logtype = rte_log_register("pmd.crypto.caam");
> > if (caam_jr_logtype >= 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 = open(absolute_file_name, O_RDONLY);
> > - SEC_ASSERT(fd > 0, fd, "Error opening file %s",
> > + SEC_ASSERT(fd >= 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 = 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 = 0;
> > + job_ring->uio_fd = -1;
> > if (job_ring->register_base_addr == NULL)
> > return;
> >
> > @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
> > int i;
> >
> > for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
> > - if (g_uio_job_ring[i].uio_fd == 0) {
> > + if (g_uio_job_ring[i].uio_fd == -1) {
> > job_ring = &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 = open(uio_device_file_name, O_RDWR);
> > - SEC_ASSERT(job_ring->uio_fd > 0, NULL,
> > + SEC_ASSERT(job_ring->uio_fd >= 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 != 0) {
> > + if (job_ring->uio_fd != -1) {
> > CAAM_JR_INFO(
> > "Closed device file for job ring %d , fd = %d",
> > job_ring->jr_id, job_ring->uio_fd);
> > close(job_ring->uio_fd);
> > + job_ring->uio_fd = -1;
> > }
> > }
> > return 0;
> > }
> > +
> > +void sec_init(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < g_uio_jr_num; i++)
> > + g_uio_job_ring[i].uio_fd = -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.
> -----Original Message-----
> From: wangyunjian <wangyunjian@huawei.com>
> Sent: Tuesday, May 12, 2020 4:28 PM
> To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> 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 <wangyunjian@huawei.com>; dev@dpdk.org
> > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> > <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> > stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
> >
> >
> >
> > > -----Original Message-----
> > > From: wangyunjian <wangyunjian@huawei.com>
> > > Sent: Monday, May 11, 2020 10:07 AM
> > > To: dev@dpdk.org
> > > Cc: Gagandeep Singh <G.Singh@nxp.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; jerry.lilijun@huawei.com;
> > > xudingke@huawei.com; Yunjian Wang <wangyunjian@huawei.com>;
> > > stable@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
> > >
> > > From: Yunjian Wang <wangyunjian@huawei.com>
> > >
> > > 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 <wangyunjian@huawei.com>
> > > ---
> > > 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 = { static void close_job_ring(struct
> > > sec_job_ring_t *job_ring) {
> > > - if (job_ring->irq_fd) {
> > > + if (job_ring->irq_fd != -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 = NULL;
> > > int i, ret = 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 = 0; i < MAX_SEC_JOB_RINGS; i++)
> g_uio_job_ring[i].uio_fd = -1;
> }
>
> void sec_job_rings_init(void)
> {
> int i;
>
> for (i = 0; i < MAX_SEC_JOB_RINGS; i++)
> g_job_rings[i].irq_fd = -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.
> >
> > > caam_jr_logtype = rte_log_register("pmd.crypto.caam");
> > > if (caam_jr_logtype >= 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 = open(absolute_file_name, O_RDONLY);
> > > - SEC_ASSERT(fd > 0, fd, "Error opening file %s",
> > > + SEC_ASSERT(fd >= 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 = 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 = 0;
> > > + job_ring->uio_fd = -1;
> > > if (job_ring->register_base_addr == NULL)
> > > return;
> > >
> > > @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
> > > int i;
> > >
> > > for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
> > > - if (g_uio_job_ring[i].uio_fd == 0) {
> > > + if (g_uio_job_ring[i].uio_fd == -1) {
> > > job_ring = &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 = open(uio_device_file_name, O_RDWR);
> > > - SEC_ASSERT(job_ring->uio_fd > 0, NULL,
> > > + SEC_ASSERT(job_ring->uio_fd >= 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 != 0) {
> > > + if (job_ring->uio_fd != -1) {
> > > CAAM_JR_INFO(
> > > "Closed device file for job ring %d , fd = %d",
> > > job_ring->jr_id, job_ring->uio_fd);
> > > close(job_ring->uio_fd);
> > > + job_ring->uio_fd = -1;
> > > }
> > > }
> > > return 0;
> > > }
> > > +
> > > +void sec_init(void)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < g_uio_jr_num; i++)
> > > + g_uio_job_ring[i].uio_fd = -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.
>
> -----Original Message-----
> From: Gagandeep Singh [mailto:G.Singh@nxp.com]
> Sent: Wednesday, May 13, 2020 12:11 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
>
>
>
> > -----Original Message-----
> > From: wangyunjian <wangyunjian@huawei.com>
> > Sent: Tuesday, May 12, 2020 4:28 PM
> > To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> > <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> > 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 <wangyunjian@huawei.com>; dev@dpdk.org
> > > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Lilijun (Jerry)
> > > <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> > > stable@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check
> > > of fd
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: wangyunjian <wangyunjian@huawei.com>
> > > > Sent: Monday, May 11, 2020 10:07 AM
> > > > To: dev@dpdk.org
> > > > Cc: Gagandeep Singh <G.Singh@nxp.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; jerry.lilijun@huawei.com;
> > > > xudingke@huawei.com; Yunjian Wang <wangyunjian@huawei.com>;
> > > > stable@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of
> > > > fd
> > > >
> > > > From: Yunjian Wang <wangyunjian@huawei.com>
> > > >
> > > > 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 <wangyunjian@huawei.com>
> > > > ---
> > > > 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 = { static void close_job_ring(struct
> > > > sec_job_ring_t *job_ring) {
> > > > - if (job_ring->irq_fd) {
> > > > + if (job_ring->irq_fd != -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 = NULL;
> > > > int i, ret = 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 = 0; i < MAX_SEC_JOB_RINGS; i++)
> > g_uio_job_ring[i].uio_fd = -1;
> > }
> >
> > void sec_job_rings_init(void)
> > {
> > int i;
> >
> > for (i = 0; i < MAX_SEC_JOB_RINGS; i++)
> > g_job_rings[i].irq_fd = -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.
Thanks,
Yunjian
>
> > >
> > > > caam_jr_logtype = rte_log_register("pmd.crypto.caam");
> > > > if (caam_jr_logtype >= 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 = open(absolute_file_name, O_RDONLY);
> > > > - SEC_ASSERT(fd > 0, fd, "Error opening file %s",
> > > > + SEC_ASSERT(fd >= 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 = 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 = 0;
> > > > + job_ring->uio_fd = -1;
> > > > if (job_ring->register_base_addr == NULL)
> > > > return;
> > > >
> > > > @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
> > > > int i;
> > > >
> > > > for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
> > > > - if (g_uio_job_ring[i].uio_fd == 0) {
> > > > + if (g_uio_job_ring[i].uio_fd == -1) {
> > > > job_ring = &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 = open(uio_device_file_name, O_RDWR);
> > > > - SEC_ASSERT(job_ring->uio_fd > 0, NULL,
> > > > + SEC_ASSERT(job_ring->uio_fd >= 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 != 0) {
> > > > + if (job_ring->uio_fd != -1) {
> > > > CAAM_JR_INFO(
> > > > "Closed device file for job ring %d , fd = %d",
> > > > job_ring->jr_id, job_ring->uio_fd);
> > > > close(job_ring->uio_fd);
> > > > + job_ring->uio_fd = -1;
> > > > }
> > > > }
> > > > return 0;
> > > > }
> > > > +
> > > > +void sec_init(void)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < g_uio_jr_num; i++)
> > > > + g_uio_job_ring[i].uio_fd = -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.
> >
@@ -2074,7 +2074,7 @@ static struct rte_security_ops caam_jr_security_ops = {
static void
close_job_ring(struct sec_job_ring_t *job_ring)
{
- if (job_ring->irq_fd) {
+ if (job_ring->irq_fd != -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 = NULL;
int i, ret = 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();
+
caam_jr_logtype = rte_log_register("pmd.crypto.caam");
if (caam_jr_logtype >= 0)
rte_log_set_level(caam_jr_logtype, RTE_LOG_NOTICE);
@@ -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
@@ -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 *
@@ -145,7 +145,7 @@ file_read_first_line(const char root[], const char subdir[],
"%s/%s/%s", root, subdir, filename);
fd = open(absolute_file_name, O_RDONLY);
- SEC_ASSERT(fd > 0, fd, "Error opening file %s",
+ SEC_ASSERT(fd >= 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 = NULL;
int i;
@@ -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 = 0;
+ job_ring->uio_fd = -1;
if (job_ring->register_base_addr == NULL)
return;
@@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
int i;
for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
- if (g_uio_job_ring[i].uio_fd == 0) {
+ if (g_uio_job_ring[i].uio_fd == -1) {
job_ring = &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 = open(uio_device_file_name, O_RDWR);
- SEC_ASSERT(job_ring->uio_fd > 0, NULL,
+ SEC_ASSERT(job_ring->uio_fd >= 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 != 0) {
+ if (job_ring->uio_fd != -1) {
CAAM_JR_INFO(
"Closed device file for job ring %d , fd = %d",
job_ring->jr_id, job_ring->uio_fd);
close(job_ring->uio_fd);
+ job_ring->uio_fd = -1;
}
}
return 0;
}
+
+void sec_init(void)
+{
+ int i;
+
+ for (i = 0; i < g_uio_jr_num; i++)
+ g_uio_job_ring[i].uio_fd = -1;
+}