From: Stuart Yoder <stuart.yoder@nxp.com>
To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
CC: German Rivera <german.rivera@nxp.com>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "agraf@suse.de" <agraf@suse.de>, "arnd@arndb.de" <arnd@arndb.de>,
        Leo Li <leoyang.li@nxp.com>, Roy Pledge <roy.pledge@nxp.com>,
        Haiying Wang <haiying.wang@nxp.com>
Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
Thread-Topic: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
Thread-Index: AQHSK6SU/JJyAb48sEOHR53H+I1ID6DHaGwwgCd/JmCAAeunoA==
Date: Tue, 29 Nov 2016 23:07:33 +0000
Message-ID: <VI1PR0401MB263816606339F10ADD3BEB7D8D8D0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
References: <1477058509-12547-1-git-send-email-stuart.yoder@nxp.com>
 <1477058509-12547-7-git-send-email-stuart.yoder@nxp.com>
 <VI1PR0401MB26389BCC00429B2C3396C3CD8DA30@VI1PR0401MB2638.eurprd04.prod.outlook.com>
 <VI1PR0402MB2847DA67590467E7CCF08F07948A0@VI1PR0402MB2847.eurprd04.prod.outlook.com>
In-Reply-To: <VI1PR0402MB2847DA67590467E7CCF08F07948A0@VI1PR0402MB2847.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Nov 2016 23:07:33.2153
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2640
X-OriginatorOrg: nxp.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAU5IjTk014200
Content-Length: 5311
Lines: 137


> -----Original Message-----
> From: Ruxandra Ioana Radulescu
> Sent: Monday, November 28, 2016 12:10 PM
> To: Stuart Yoder <stuart.yoder@nxp.com>; gregkh@linuxfoundation.org
> Cc: German Rivera <german.rivera@nxp.com>; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org;
> agraf@suse.de; arnd@arndb.de; Leo Li <leoyang.li@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Haiying Wang
> <haiying.wang@nxp.com>
> Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> > -----Original Message-----
> > From: Stuart Yoder [mailto:stuart.yoder@nxp.com]
> > Sent: Friday, October 21, 2016 9:02 AM
> > To: gregkh@linuxfoundation.org
> > Cc: German Rivera <german.rivera@nxp.com>; devel@driverdev.osuosl.org;
> > linux-kernel@vger.kernel.org; agraf@suse.de; arnd@arndb.de; Leo Li
> > <leoyang.li@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Roy Pledge
> > <roy.pledge@nxp.com>; Haiying Wang <haiying.wang@nxp.com>; Stuart
> > Yoder <stuart.yoder@nxp.com>
> > Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> >
> > From: Roy Pledge <Roy.Pledge@nxp.com>
> >
> > Add QBman APIs for frame queue and buffer pool operations.
> >
> > Signed-off-by: Roy Pledge <roy.pledge@nxp.com>
> > Signed-off-by: Haiying Wang <haiying.wang@nxp.com>
> > Signed-off-by: Stuart Yoder <stuart.yoder@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 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.  Will fix this on v3 respin.

Stuart