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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANLsYky-vi4b29nr+rwbcSP3dUMADDV1o8irAX=XDSq6MkuLSA@mail.gmail.com>
Date:   Fri, 3 Nov 2017 14:13:21 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <Suzuki.Poulose@....com>
Cc:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        rob.walker@....com, Mike Leach <mike.leach@...aro.org>,
        coresight@...ts.linaro.org
Subject: Re: [PATCH 07/17] coresight: tmc etr: Add transparent buffer management

On 3 November 2017 at 04:02, Suzuki K Poulose <Suzuki.Poulose@....com> 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 <mathieu.poirier@...aro.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>>> ---
>>>   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