Re: [PATCH 07/17] coresight: tmc etr: Add transparent buffer management

[Suzuki K Poulose]

On 02/11/17 17:48, Mathieu Poirier wrote:
> On Thu, Oct 19, 2017 at 06:15:43PM +0100, Suzuki K Poulose wrote:
> > At the moment we always use contiguous memory for TMC ETR tracing
> > when used from sysfs. The size of the buffer is fixed at boot time
> > and can only be changed by modifiying the DT. With the introduction
> > of SG support we could support really large buffers in that mode.
> > This patch abstracts the buffer used for ETR to switch between a
> > contiguous buffer or a SG table depending on the availability of
> > the memory.
> >
> > This also enables the sysfs mode to use the ETR in SG mode depending
> > on configured the trace buffer size. Also, since ETR will use the
> > new infrastructure to manage the buffer, we can get rid of some
> > of the members in the tmc_drvdata and clean up the fields a bit.
> >
> > Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> >   drivers/hwtracing/coresight/coresight-tmc-etr.c | 433 +++++++++++++++++++-----
> >   drivers/hwtracing/coresight/coresight-tmc.h     |  60 +++-
> >   2 files changed, 403 insertions(+), 90 deletions(-)
>
> [..]
>   
> > +
> > +static void tmc_etr_sync_sg_buf(struct etr_buf *etr_buf, u64 rrp, u64 rwp)

> > +	w_offset = tmc_sg_get_data_page_offset(table, rwp);
> > +	if (w_offset < 0) {
> > +		dev_warn(table->dev, "Unable to map RWP %llx to offset\n",
> > +				rwp);
>
>                  dev_warn(table->dev,
>                           "Unable to map RWP %llx to offset\n", rwq);
>
> It looks a little better and we respect indentation rules.  Same for r_offset.

> > +static inline int tmc_etr_mode_alloc_buf(int mode,
> > +				  struct tmc_drvdata *drvdata,
> > +				  struct etr_buf *etr_buf, int node,
> > +				  void **pages)
>
> static inline int
> tmc_etr_mode_alloc_buf(int mode,
>                         struct tmc_drvdata *drvdata,
>                         struct etr_buf *etr_buf, int node,
>                         void **pages)

> > + * tmc_alloc_etr_buf: Allocate a buffer use by ETR.
> > + * @drvdata	: ETR device details.
> > + * @size	: size of the requested buffer.
> > + * @flags	: Required properties of the type of buffer.
> > + * @node	: Node for memory allocations.
> > + * @pages	: An optional list of pages.
> > + */
> > +static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
> > +					  ssize_t size, int flags,
> > +					  int node, void **pages)
>
> Please fix indentation.  Also @flags isn't used.

Yep, flags is only used later and can move it to the patch where we use it.

> > +{
> > +	int rc = -ENOMEM;
> > +	bool has_etr_sg, has_iommu;
> > +	struct etr_buf *etr_buf;
> > +
> > +	has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG);
> > +	has_iommu = iommu_get_domain_for_dev(drvdata->dev);
> > +
> > +	etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL);
> > +	if (!etr_buf)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	etr_buf->size = size;
> > +
> > +	/*
> > +	 * If we have to use an existing list of pages, we cannot reliably
> > +	 * use a contiguous DMA memory (even if we have an IOMMU). Otherwise,
> > +	 * we use the contiguous DMA memory if :
> > +	 *  a) The ETR cannot use Scatter-Gather.
> > +	 *  b) if not a, we have an IOMMU backup
>
> Please rework the above sentence.

How about :
	   b) if (a) is not true and we have an IOMMU connected to the ETR.

I will address the other comments on indentation.

Thanks for the detailed look

Cheers
Suzuki