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

[Ruxandra Ioana Radulescu]

> -----Original Message-----
> From: Stuart Yoder [mailto:stuart.yoder@xxxxxxx]
> Sent: Friday, October 21, 2016 9:02 AM
> To: gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: German Rivera <german.rivera@xxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; agraf@xxxxxxx; arnd@xxxxxxxx; Leo Li
> <leoyang.li@xxxxxxx>; Roy Pledge <roy.pledge@xxxxxxx>; Roy Pledge
> <roy.pledge@xxxxxxx>; Haiying Wang <haiying.wang@xxxxxxx>; Stuart
> Yoder <stuart.yoder@xxxxxxx>
> Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> From: Roy Pledge <Roy.Pledge@xxxxxxx>
> 
> Add QBman APIs for frame queue and buffer pool operations.
> 
> Signed-off-by: Roy Pledge <roy.pledge@xxxxxxx>
> Signed-off-by: Haiying Wang <haiying.wang@xxxxxxx>
> Signed-off-by: Stuart Yoder <stuart.yoder@xxxxxxx>
> ---
>  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 0000000..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(&s->vdq.available)) {
> +		atomic_inc(&s->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