subject:"RE\: \[PATCH 6\/9\] bus\: fsl\-mc\: dpio\: add QBMan portal APIs for DPAA2"

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

2016-11-29 Thread Stuart Yoder



> -Original Message-
> From: Ruxandra Ioana Radulescu
> Sent: Monday, November 28, 2016 12:10 PM
> To: Stuart Yoder <stuart.yo...@nxp.com>; gre...@linuxfoundation.org
> Cc: German Rivera <german.riv...@nxp.com>; de...@driverdev.osuosl.org; 
> linux-ker...@vger.kernel.org;
> ag...@suse.de; a...@arndb.de; Leo Li <leoyang...@nxp.com>; Roy Pledge 
> <roy.ple...@nxp.com>; Haiying Wang
> <haiying.w...@nxp.com>
> Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> > -Original Message-
> > From: Stuart Yoder [mailto:stuart.yo...@nxp.com]
> > Sent: Friday, October 21, 2016 9:02 AM
> > To: gre...@linuxfoundation.org
> > Cc: German Rivera <german.riv...@nxp.com>; de...@driverdev.osuosl.org;
> > linux-ker...@vger.kernel.org; ag...@suse.de; a...@arndb.de; Leo Li
> > <leoyang...@nxp.com>; Roy Pledge <roy.ple...@nxp.com>; Roy Pledge
> > <roy.ple...@nxp.com>; Haiying Wang <haiying.w...@nxp.com>; Stuart
> > Yoder <stuart.yo...@nxp.com>
> > Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> >
> > From: Roy Pledge <roy.ple...@nxp.com>
> >
> > Add QBman APIs for frame queue and buffer pool operations.
> >
> > Signed-off-by: Roy Pledge <roy.ple...@nxp.com>
> > Signed-off-by: Haiying Wang <haiying.w...@nxp.com>
> > Signed-off-by: Stuart Yoder <stuart.yo...@nxp.com>
> > ---
> >  drivers/bus/fsl-mc/dpio/Makefile   |2 +-
> >  drivers/bus/fsl-mc/dpio/qbman-portal.c | 1009
> > 
> >  drivers/bus/fsl-mc/dpio/qbman-portal.h |  464 +++
> >  3 files changed, 1474 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.c
> >  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.h
> >
> > diff --git a/drivers/bus/fsl-mc/dpio/Makefile b/drivers/bus/fsl-
> > mc/dpio/Makefile
> > index 128befc..6588498 100644
> > --- a/drivers/bus/fsl-mc/dpio/Makefile
> > +++ b/drivers/bus/fsl-mc/dpio/Makefile
> > @@ -6,4 +6,4 @@ subdir-ccflags-y := -Werror
> >
> >  obj-$(CONFIG_FSL_MC_DPIO) += fsl-mc-dpio.o
> >
> > -fsl-mc-dpio-objs := dpio.o
> > +fsl-mc-dpio-objs := dpio.o qbman-portal.o
> > diff --git a/drivers/bus/fsl-mc/dpio/qbman-portal.c b/drivers/bus/fsl-
> > mc/dpio/qbman-portal.c
> > new file mode 100644
> > index 000..1eb3dd9
> > --- /dev/null
> > +++ b/drivers/bus/fsl-mc/dpio/qbman-portal.c
> 
> [...]
> 
> > +/**
> > + * qbman_swp_pull() - Issue the pull dequeue command
> > + * @s: the software portal object
> > + * @d: the software portal descriptor which has been configured with
> > + * the set of qbman_pull_desc_set_*() calls
> > + *
> > + * Return 0 for success, and -EBUSY if the software portal is not ready
> > + * to do pull dequeue.
> > + */
> > +int qbman_swp_pull(struct qbman_swp *s, struct qbman_pull_desc *d)
> > +{
> > +   struct qbman_pull_desc *p;
> > +
> > +   if (!atomic_dec_and_test(>vdq.available)) {
> > +   atomic_inc(>vdq.available);
> > +   return -EBUSY;
> > +   }
> > +   s->vdq.storage = (void *)d->rsp_addr_virt;
> > +   d->tok = 1;
> > +   p = qbman_get_cmd(s, QBMAN_CENA_SWP_VDQCR);
> > +   *p = *d;
> > +   dma_wmb();
> > +
> > +   /* Set the verb byte, have to substitute in the valid-bit */
> > +   p->verb |= s->vdq.valid_bit;
> > +   s->vdq.valid_bit ^= QB_VALID_BIT;
> > +
> > +   return 0;
> > +}
> > +
> [...]
> > +
> > +/**
> > + * qbman_result_has_new_result() - Check and get the dequeue response
> > from the
> > + * dq storage memory set in pull dequeue 
> > command
> > + * @s: the software portal object
> > + * @dq: the dequeue result read from the memory
> > + *
> > + * Return 1 for getting a valid dequeue result, or 0 for not getting a 
> > valid
> > + * dequeue result.
> > + *
> > + * Only used for user-provided storage of dequeue results, not DQRR. For
> > + * efficiency purposes, the driver will perform any required endianness
> > + * conversion to ensure that the user's dequeue result storage is in host-
> > endian
> > + * format. As such, once the user has called
> > qbman_result_has_new_result() and
> > + * been returned a valid dequeue result, they should not call it again on
> > + * the same memory location (except of course if another dequeue
> > command has
> > + * be

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

2016-11-28 Thread Ruxandra Ioana Radulescu

> -Original Message-
> From: Stuart Yoder [mailto:stuart.yo...@nxp.com]
> Sent: Friday, October 21, 2016 9:02 AM
> To: gre...@linuxfoundation.org
> Cc: German Rivera ; de...@driverdev.osuosl.org;
> linux-ker...@vger.kernel.org; ag...@suse.de; a...@arndb.de; Leo Li
> ; Roy Pledge ; Roy Pledge
> ; Haiying Wang ; Stuart
> Yoder 
> Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> From: Roy Pledge 
> 
> Add QBman APIs for frame queue and buffer pool operations.
> 
> Signed-off-by: Roy Pledge 
> Signed-off-by: Haiying Wang 
> Signed-off-by: Stuart Yoder 
> ---
>  drivers/bus/fsl-mc/dpio/Makefile   |2 +-
>  drivers/bus/fsl-mc/dpio/qbman-portal.c | 1009
> 
>  drivers/bus/fsl-mc/dpio/qbman-portal.h |  464 +++
>  3 files changed, 1474 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.c
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.h
> 
> diff --git a/drivers/bus/fsl-mc/dpio/Makefile b/drivers/bus/fsl-
> mc/dpio/Makefile
> index 128befc..6588498 100644
> --- a/drivers/bus/fsl-mc/dpio/Makefile
> +++ b/drivers/bus/fsl-mc/dpio/Makefile
> @@ -6,4 +6,4 @@ subdir-ccflags-y := -Werror
> 
>  obj-$(CONFIG_FSL_MC_DPIO) += fsl-mc-dpio.o
> 
> -fsl-mc-dpio-objs := dpio.o
> +fsl-mc-dpio-objs := dpio.o qbman-portal.o
> diff --git a/drivers/bus/fsl-mc/dpio/qbman-portal.c b/drivers/bus/fsl-
> mc/dpio/qbman-portal.c
> new file mode 100644
> index 000..1eb3dd9
> --- /dev/null
> +++ b/drivers/bus/fsl-mc/dpio/qbman-portal.c

[...]

> +/**
> + * qbman_swp_pull() - Issue the pull dequeue command
> + * @s: the software portal object
> + * @d: the software portal descriptor which has been configured with
> + * the set of qbman_pull_desc_set_*() calls
> + *
> + * Return 0 for success, and -EBUSY if the software portal is not ready
> + * to do pull dequeue.
> + */
> +int qbman_swp_pull(struct qbman_swp *s, struct qbman_pull_desc *d)
> +{
> + struct qbman_pull_desc *p;
> +
> + if (!atomic_dec_and_test(>vdq.available)) {
> + atomic_inc(>vdq.available);
> + return -EBUSY;
> + }
> + s->vdq.storage = (void *)d->rsp_addr_virt;
> + d->tok = 1;
> + p = qbman_get_cmd(s, QBMAN_CENA_SWP_VDQCR);
> + *p = *d;
> + dma_wmb();
> +
> + /* Set the verb byte, have to substitute in the valid-bit */
> + p->verb |= s->vdq.valid_bit;
> + s->vdq.valid_bit ^= QB_VALID_BIT;
> +
> + return 0;
> +}
> +
[...] 
> +
> +/**
> + * qbman_result_has_new_result() - Check and get the dequeue response
> from the
> + * dq storage memory set in pull dequeue 
> command
> + * @s: the software portal object
> + * @dq: the dequeue result read from the memory
> + *
> + * Return 1 for getting a valid dequeue result, or 0 for not getting a valid
> + * dequeue result.
> + *
> + * Only used for user-provided storage of dequeue results, not DQRR. For
> + * efficiency purposes, the driver will perform any required endianness
> + * conversion to ensure that the user's dequeue result storage is in host-
> endian
> + * format. As such, once the user has called
> qbman_result_has_new_result() and
> + * been returned a valid dequeue result, they should not call it again on
> + * the same memory location (except of course if another dequeue
> command has
> + * been executed to produce a new result to that location).
> + */
> +int qbman_result_has_new_result(struct qbman_swp *s, const struct
> dpaa2_dq *dq)
> +{
> + if (!dq->dq.tok)
> + return 0;

While testing the Ethernet driver I discovered that sometimes the above
check fails.

When we check a store entry for the first time, if the hardware didn't
manage to fill it with a valid respose yet, we may find a non-null value in the
token field (because the stores have uninitialized memory). So by only
checking that token is !=0, we risk processing an uninitialized memory area
as a valid dequeue entry.

We should always compare the token field against 1, the value that is given
to the hardware on the dequeue command. It might also be a good idea
to use a define here, to make it clear 1 is a magic value.

And I think we should also zero the stores when they are first allocated,
since even with the proposed fix there's still a (much smaller) risk of finding
our exact token value in an uninitialized memory area.

Thanks,
Ioana
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

2016-11-10 Thread Stuart Yoder



> -Original Message-
> From: Ruxandra Ioana Radulescu
> Sent: Thursday, November 10, 2016 9:04 AM
> To: Stuart Yoder <stuart.yo...@nxp.com>; gre...@linuxfoundation.org
> Cc: German Rivera <german.riv...@nxp.com>; de...@driverdev.osuosl.org; 
> linux-ker...@vger.kernel.org;
> ag...@suse.de; a...@arndb.de; Leo Li <leoyang...@nxp.com>; Roy Pledge 
> <roy.ple...@nxp.com>; Haiying Wang
> <haiying.w...@nxp.com>; Stuart Yoder <stuart.yo...@nxp.com>
> Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> 
> > -Original Message-
> > From: Stuart Yoder [mailto:stuart.yo...@nxp.com]
> > Sent: Friday, October 21, 2016 9:02 AM
> > To: gre...@linuxfoundation.org
> > Cc: German Rivera <german.riv...@nxp.com>; de...@driverdev.osuosl.org;
> > linux-ker...@vger.kernel.org; ag...@suse.de; a...@arndb.de; Leo Li
> > <leoyang...@nxp.com>; Roy Pledge <roy.ple...@nxp.com>; Roy Pledge
> > <roy.ple...@nxp.com>; Haiying Wang <haiying.w...@nxp.com>; Stuart
> > Yoder <stuart.yo...@nxp.com>
> > Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> >
> > From: Roy Pledge <roy.ple...@nxp.com>
> >
> > Add QBman APIs for frame queue and buffer pool operations.
> >
> > Signed-off-by: Roy Pledge <roy.ple...@nxp.com>
> > Signed-off-by: Haiying Wang <haiying.w...@nxp.com>
> > Signed-off-by: Stuart Yoder <stuart.yo...@nxp.com>
> > ---
> >  drivers/bus/fsl-mc/dpio/Makefile   |2 +-
> >  drivers/bus/fsl-mc/dpio/qbman-portal.c | 1009
> > 
> >  drivers/bus/fsl-mc/dpio/qbman-portal.h |  464 +++
> >  3 files changed, 1474 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.c
> >  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.h
> >
> > diff --git a/drivers/bus/fsl-mc/dpio/Makefile b/drivers/bus/fsl-
> > mc/dpio/Makefile
> > index 128befc..6588498 100644
> > --- a/drivers/bus/fsl-mc/dpio/Makefile
> > +++ b/drivers/bus/fsl-mc/dpio/Makefile
> > @@ -6,4 +6,4 @@ subdir-ccflags-y := -Werror
> >
> >  obj-$(CONFIG_FSL_MC_DPIO) += fsl-mc-dpio.o
> >
> > -fsl-mc-dpio-objs := dpio.o
> > +fsl-mc-dpio-objs := dpio.o qbman-portal.o
> > diff --git a/drivers/bus/fsl-mc/dpio/qbman-portal.c b/drivers/bus/fsl-
> > mc/dpio/qbman-portal.c
> > new file mode 100644
> > index 000..1eb3dd9
> > --- /dev/null
> > +++ b/drivers/bus/fsl-mc/dpio/qbman-portal.c
> > @@ -0,0 +1,1009 @@
> 
> [...]
> 
> > +/**
> > + * qbman_swp_release() - Issue a buffer release command
> > + * @s:   the software portal object
> > + * @d:   the release descriptor
> > + * @buffers: a pointer pointing to the buffer address to be released
> > + * @num_buffers: number of buffers to be released,  must be less than 8
> > + *
> > + * Return 0 for success, -EBUSY if the release command ring is not ready.
> > + */
> > +int qbman_swp_release(struct qbman_swp *s, const struct
> > qbman_release_desc *d,
> > + const u64 *buffers, unsigned int num_buffers)
> > +{
> > +   int i;
> > +   struct qbman_release_desc *p;
> > +   u32 rar;
> > +
> > +   if (!num_buffers || (num_buffers > 7))
> > +   return -EINVAL;
> > +
> > +   rar = qbman_read_register(s, QBMAN_CINH_SWP_RAR);
> > +   if (!RAR_SUCCESS(rar))
> > +   return -EBUSY;
> > +
> > +   /* Start the release command */
> > +   p = qbman_get_cmd(s, QBMAN_CENA_SWP_RCR(RAR_IDX(rar)));
> > +   /* Copy the caller's buffer pointers to the command */
> > +   for (i = 0; i < num_buffers; i++)
> > +   p->buf[i] = cpu_to_le64(buffers[i]);
> > +
> 
> Hi Stuart,
> We also need to set BPID field in the buffer release command, something like:
> +   p->bpid = d->bpid;
> Without this all buffers will be released to buffer pool id 0, which is 
> incorrect.

Will fix on next respin.

> > +   /*
> > +* Set the verb byte, have to substitute in the valid-bit and the
> > number
> > +* of buffers.
> > +*/
> > +   dma_wmb();
> > +   p->verb = d->verb | RAR_VB(rar) | num_buffers;
> > +
> > +   return 0;
> > +}
> > +
> > +struct qbman_acquire_desc {
> > +   u8 verb;
> > +   u8 reserved;
> > +   u16 bpid;
> > +   u8 num;
> > +   u8 reserved2[59];
> > +};
> > +
> > +struct qbman_acquire_rslt {
> > +   u8 v

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

2016-11-10 Thread Ruxandra Ioana Radulescu

 
> -Original Message-
> From: Stuart Yoder [mailto:stuart.yo...@nxp.com]
> Sent: Friday, October 21, 2016 9:02 AM
> To: gre...@linuxfoundation.org
> Cc: German Rivera ; de...@driverdev.osuosl.org;
> linux-ker...@vger.kernel.org; ag...@suse.de; a...@arndb.de; Leo Li
> ; Roy Pledge ; Roy Pledge
> ; Haiying Wang ; Stuart
> Yoder 
> Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> From: Roy Pledge 
> 
> Add QBman APIs for frame queue and buffer pool operations.
> 
> Signed-off-by: Roy Pledge 
> Signed-off-by: Haiying Wang 
> Signed-off-by: Stuart Yoder 
> ---
>  drivers/bus/fsl-mc/dpio/Makefile   |2 +-
>  drivers/bus/fsl-mc/dpio/qbman-portal.c | 1009
> 
>  drivers/bus/fsl-mc/dpio/qbman-portal.h |  464 +++
>  3 files changed, 1474 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.c
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.h
> 
> diff --git a/drivers/bus/fsl-mc/dpio/Makefile b/drivers/bus/fsl-
> mc/dpio/Makefile
> index 128befc..6588498 100644
> --- a/drivers/bus/fsl-mc/dpio/Makefile
> +++ b/drivers/bus/fsl-mc/dpio/Makefile
> @@ -6,4 +6,4 @@ subdir-ccflags-y := -Werror
> 
>  obj-$(CONFIG_FSL_MC_DPIO) += fsl-mc-dpio.o
> 
> -fsl-mc-dpio-objs := dpio.o
> +fsl-mc-dpio-objs := dpio.o qbman-portal.o
> diff --git a/drivers/bus/fsl-mc/dpio/qbman-portal.c b/drivers/bus/fsl-
> mc/dpio/qbman-portal.c
> new file mode 100644
> index 000..1eb3dd9
> --- /dev/null
> +++ b/drivers/bus/fsl-mc/dpio/qbman-portal.c
> @@ -0,0 +1,1009 @@

[...] 

> +/**
> + * qbman_swp_release() - Issue a buffer release command
> + * @s:   the software portal object
> + * @d:   the release descriptor
> + * @buffers: a pointer pointing to the buffer address to be released
> + * @num_buffers: number of buffers to be released,  must be less than 8
> + *
> + * Return 0 for success, -EBUSY if the release command ring is not ready.
> + */
> +int qbman_swp_release(struct qbman_swp *s, const struct
> qbman_release_desc *d,
> +   const u64 *buffers, unsigned int num_buffers)
> +{
> + int i;
> + struct qbman_release_desc *p;
> + u32 rar;
> +
> + if (!num_buffers || (num_buffers > 7))
> + return -EINVAL;
> +
> + rar = qbman_read_register(s, QBMAN_CINH_SWP_RAR);
> + if (!RAR_SUCCESS(rar))
> + return -EBUSY;
> +
> + /* Start the release command */
> + p = qbman_get_cmd(s, QBMAN_CENA_SWP_RCR(RAR_IDX(rar)));
> + /* Copy the caller's buffer pointers to the command */
> + for (i = 0; i < num_buffers; i++)
> + p->buf[i] = cpu_to_le64(buffers[i]);
> +

Hi Stuart,
We also need to set BPID field in the buffer release command, something like:
+   p->bpid = d->bpid;
Without this all buffers will be released to buffer pool id 0, which is 
incorrect.

> + /*
> +  * Set the verb byte, have to substitute in the valid-bit and the
> number
> +  * of buffers.
> +  */
> + dma_wmb();
> + p->verb = d->verb | RAR_VB(rar) | num_buffers;
> +
> + return 0;
> +}
> +
> +struct qbman_acquire_desc {
> + u8 verb;
> + u8 reserved;
> + u16 bpid;
> + u8 num;
> + u8 reserved2[59];
> +};
> +
> +struct qbman_acquire_rslt {
> + u8 verb;
> + u8 rslt;
> + u16 reserved;
> + u8 num;
> + u8 reserved2[3];
> + u64 buf[7];
> +};
> +
> +/**
> + * qbman_swp_acquire() - Issue a buffer acquire command
> + * @s:   the software portal object
> + * @bpid:the buffer pool index
> + * @buffers: a pointer pointing to the acquired buffer addresses
> + * @num_buffers: number of buffers to be acquired, must be less than 8
> + *
> + * Return 0 for success, or negative error code if the acquire command
> + * fails.
> + */
> +int qbman_swp_acquire(struct qbman_swp *s, u16 bpid, u64 *buffers,
> +   unsigned int num_buffers)
> +{
> + struct qbman_acquire_desc *p;
> + struct qbman_acquire_rslt *r;
> + int i;
> +
> + if (!num_buffers || (num_buffers > 7))
> + return -EINVAL;
> +
> + /* Start the management command */
> + p = qbman_swp_mc_start(s);

qbman_swp_mc_start() returns a pointer to where the QBMan
management command must be written, but doesn't clear any
previous values found there.
We should memset the area to zero before using it.
Same comment applies to other places where this function is used.

> +
> + if (!p)
> + return -EBUSY;
> +
> + /* Encode the caller-provided attributes */
> + p->bpid = cpu_to_le16(bpid);
> + p->num = num_buffers;
> +
> + /* Complete the management command */
> + r = qbman_swp_mc_complete(s, p, p->verb |
> QBMAN_MC_ACQUIRE);

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

4 matches

Site Navigation

Mail list logo

Footer information