Re: [PATCH 02/17] coresight tmc: Hide trace buffer handling for file read

[Suzuki K Poulose]

On 20/10/17 13:34, Julien Thierry wrote:
> Hi Suzuki,
>
> On 19/10/17 18:15, Suzuki K Poulose wrote:
> > At the moment we adjust the buffer pointers for reading the trace
> > data via misc device in the common code for ETF/ETB and ETR. Since
> > we are going to change how we manage the buffer for ETR, let us
> > move the buffer manipulation to the respective driver files, hiding
> > it from the common code. We do so by adding type specific helpers
> > for finding the length of data and the pointer to the buffer,
> > for a given length at a file position.
> >
> > Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> > Â drivers/hwtracing/coresight/coresight-tmc-etf.c | 16 ++++++++++++
> > Â drivers/hwtracing/coresight/coresight-tmc-etr.c | 33 ++++++++++++++++++++++++
> > Â drivers/hwtracing/coresight/coresight-tmc.cÂÂÂÂ | 34 ++++++++++++++-----------
> > Â drivers/hwtracing/coresight/coresight-tmc.hÂÂÂÂ |Â 4 +++
> > Â 4 files changed, 72 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index e2513b786242..0b6f1eb746de 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -120,6 +120,22 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
> > ÂÂÂÂÂ CS_LOCK(drvdata->base);
> > Â }
> > +/*
> > + * Return the available trace data in the buffer from @pos, with
> > + * a maximum limit of @len, updating the @bufpp on where to
> > + * find it.
> > + */
> > +ssize_t tmc_etb_get_sysfs_trace(struct tmc_drvdata *drvdata,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t pos, size_t len, char **bufpp)
> > +{
> > +ÂÂÂ /* Adjust the len to available size @pos */
> > +ÂÂÂ if (pos + len > drvdata->len)
> > +ÂÂÂÂÂÂÂ len = drvdata->len - pos;
> > +ÂÂÂ if (len > 0)
>
> Do we have some guarantee that "pos <= drvdata->len"? because since len is unsigned this check only covers the case were len is 0.
>
> Maybe it would be better to use a signed variable to store the result of the difference.
>
> > +ÂÂÂÂÂÂÂ *bufpp = drvdata->buf + pos;


> > + * Return the available trace data in the buffer @pos, with a maximum
> > + * limit of @len, also updating the @bufpp on where to find it.
> > + */
> > +ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t pos, size_t len, char **bufpp)
> > +{
> > +ÂÂÂ char *bufp = drvdata->buf + pos;
> > +ÂÂÂ char *bufend = (char *)(drvdata->vaddr + drvdata->size);
> > +
> > +ÂÂÂ /* Adjust the len to available size @pos */
> > +ÂÂÂ if (pos + len > drvdata->len)
> > +ÂÂÂÂÂÂÂ len = drvdata->len - pos;
> > +
> > +ÂÂÂ if (len <= 0)
> > +ÂÂÂÂÂÂÂ return len;
>
> Similar issue here.

Thanks for spotting. I will fix it.

Cheers