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

[Mathieu Poirier]

On 3 November 2017 at 04:02, Suzuki K Poulose  wrote:
> 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 
>>> Signed-off-by: Suzuki K Poulose 
>>> ---
>>>   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.
>>

Ok, I haven't made it that far yet.  If it's used later one just leave
it as it is.

>
> 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'm good with that.

>
> I will address the other comments on indentation.
>
> Thanks for the detailed look
>
> Cheers
> Suzuki

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

[Mathieu Poirier]

On 3 November 2017 at 04:02, Suzuki K Poulose  wrote:
> 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 
>>> Signed-off-by: Suzuki K Poulose 
>>> ---
>>>   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.
>>

Ok, I haven't made it that far yet.  If it's used later one just leave
it as it is.

>
> 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'm good with that.

>
> I will address the other comments on indentation.
>
> Thanks for the detailed look
>
> Cheers
> Suzuki

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 
Signed-off-by: Suzuki K Poulose 
---
  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

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 
Signed-off-by: Suzuki K Poulose 
---
  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

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

[Mathieu Poirier]

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 
> Signed-off-by: Suzuki K Poulose 
> ---
>  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)
> +{
> + long r_offset, w_offset;
> + struct etr_sg_table *etr_table = etr_buf->private;
> + struct tmc_sg_table *table = etr_table->sg_table;
> +
> + r_offset = tmc_sg_get_data_page_offset(table, rrp);
> + if (r_offset < 0) {
> + dev_warn(table->dev, "Unable to map RRP %llx to offset\n",
> + rrp);
> + etr_buf->len = 0;
> + return;
> + }
> +
> + 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.

> + etr_buf->len = 0;
> + return;
> + }
> +
> + etr_buf->offset = r_offset;
> + if (etr_buf->full)
> + etr_buf->len = etr_buf->size;
> + else
> + etr_buf->len = (w_offset < r_offset) ?
> + etr_buf->size + w_offset - r_offset :
> + w_offset - r_offset;
> + tmc_sg_table_sync_data_range(table, r_offset, etr_buf->len);
> +}
> +
> +static const struct etr_buf_operations etr_sg_buf_ops = {
> + .alloc = tmc_etr_alloc_sg_buf,
> + .free = tmc_etr_free_sg_buf,
> + .sync = tmc_etr_sync_sg_buf,
> + .get_data = tmc_etr_get_data_sg_buf,
> +};
> +
> +static const struct etr_buf_operations *etr_buf_ops[] = {
> + [ETR_MODE_FLAT] = _flat_buf_ops,
> + [ETR_MODE_ETR_SG] = _sg_buf_ops,
> +};
> +
> +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)

> +{
> + int rc;
> +
> + switch (mode) {
> + case ETR_MODE_FLAT:
> + case ETR_MODE_ETR_SG:
> + rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);
> + if (!rc)
> + etr_buf->ops = etr_buf_ops[mode];
> + return rc;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/*
> + * 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.

> +{
> + 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.

> +  *  c) if none of the above holds, use it for smaller memory (< 1M).
> +  *
> 

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

[Mathieu Poirier]

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 
> Signed-off-by: Suzuki K Poulose 
> ---
>  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)
> +{
> + long r_offset, w_offset;
> + struct etr_sg_table *etr_table = etr_buf->private;
> + struct tmc_sg_table *table = etr_table->sg_table;
> +
> + r_offset = tmc_sg_get_data_page_offset(table, rrp);
> + if (r_offset < 0) {
> + dev_warn(table->dev, "Unable to map RRP %llx to offset\n",
> + rrp);
> + etr_buf->len = 0;
> + return;
> + }
> +
> + 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.

> + etr_buf->len = 0;
> + return;
> + }
> +
> + etr_buf->offset = r_offset;
> + if (etr_buf->full)
> + etr_buf->len = etr_buf->size;
> + else
> + etr_buf->len = (w_offset < r_offset) ?
> + etr_buf->size + w_offset - r_offset :
> + w_offset - r_offset;
> + tmc_sg_table_sync_data_range(table, r_offset, etr_buf->len);
> +}
> +
> +static const struct etr_buf_operations etr_sg_buf_ops = {
> + .alloc = tmc_etr_alloc_sg_buf,
> + .free = tmc_etr_free_sg_buf,
> + .sync = tmc_etr_sync_sg_buf,
> + .get_data = tmc_etr_get_data_sg_buf,
> +};
> +
> +static const struct etr_buf_operations *etr_buf_ops[] = {
> + [ETR_MODE_FLAT] = _flat_buf_ops,
> + [ETR_MODE_ETR_SG] = _sg_buf_ops,
> +};
> +
> +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)

> +{
> + int rc;
> +
> + switch (mode) {
> + case ETR_MODE_FLAT:
> + case ETR_MODE_ETR_SG:
> + rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);
> + if (!rc)
> + etr_buf->ops = etr_buf_ops[mode];
> + return rc;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/*
> + * 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.

> +{
> + 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.

> +  *  c) if none of the above holds, use it for smaller memory (< 1M).
> +  *
> +  * Fallback to available mechanisms.
> +