Re: [PATCH v2 5/9] coresight: Dynamically add connections

[James Clark <james.clark@arm.com>]

On 21/03/2023 17:56, Mike Leach wrote:
> Hi James
> 
> On Thu, 16 Mar 2023 at 17:12, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 10/03/2023 16:06, James Clark wrote:
>>> Add a function for adding connections dynamically. This also removes
>>> the 1:1 mapping between port number and the index into the connections
>>> array. The only place this mapping was used was in the warning for
>>> duplicate output ports, which has been replaced by a search. Other
>>> uses of the port number already use the port member variable.
>>>
>>> Being able to dynamically add connections will allow other devices like
>>> CTI to re-use the connection mechanism despite not having explicit
>>> connections described in the DT.
>>>
>>> Signed-off-by: James Clark <james.clark@arm.com>
>>> ---
>>>   .../hwtracing/coresight/coresight-platform.c  | 77 ++++++++++++++-----
>>>   include/linux/coresight.h                     |  7 +-
>>>   2 files changed, 64 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>>> index c77238cdf448..16553f7dde12 100644
>>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>>> @@ -27,8 +27,9 @@ static int coresight_alloc_conns(struct device *dev,
>>>                                struct coresight_platform_data *pdata)
>>>   {
>>>       if (pdata->nr_outconns) {
>>> -             pdata->out_conns = devm_kcalloc(dev, pdata->nr_outconns,
>>> -                                         sizeof(*pdata->out_conns), GFP_KERNEL);
>>> +             pdata->out_conns = devm_krealloc_array(
>>> +                     dev, pdata->out_conns, pdata->nr_outconns,
>>
>> super minor nit:
>>                 pdata->out_conns = devm_krealloc_array(dev,
>>
>>
>>> +                     sizeof(*pdata->out_conns), GFP_KERNEL | __GFP_ZERO);
>>>               if (!pdata->out_conns)
>>>                       return -ENOMEM;
>>>       }
>>> @@ -36,6 +37,48 @@ static int coresight_alloc_conns(struct device *dev,
>>>       return 0;
>>>   }
>>>
>>> +/*
>>> + * Add a connection in the first free slot, or realloc
>>> + * if there is no space. @conn's contents is copied into the new slot.
>>> + *
>>> + * If the output port is already assigned on this device, return -EINVAL
>>> + */
>>> +int coresight_add_conn(struct device *dev,
>>> +                    struct coresight_platform_data *pdata,
>>> +                    const struct coresight_connection *conn)
>>> +{
>>> +     int ret;
>>> +     struct coresight_connection *free_conn = NULL;
>>> +     struct coresight_connection *i;
>>> +
>>> +     /*
>>> +      * Search for a free slot, and while looking for one, warn
>>> +      * on any existing duplicate output port.
>>> +      */
>>> +     for (i = pdata->out_conns; i < pdata->out_conns + pdata->nr_outconns;
>>> +          ++i) {
>>
>> minor nit: I see why you have gone against using "i" as index into
>> the array. But I think having that as the index is still better
>> readable.
>>
>>         for (i = 0; i < pdata->nr_outconns; i++) {
>>                 struct coresight_connection *c = &pdata->out_conns[i];
>>
>>> +             if (i->remote_fwnode && conn->port != -1 &&
>>> +                 i->port == conn->port) {
>>> +                     dev_warn(dev, "Duplicate output port %d\n", i->port);
>>> +                     return -EINVAL;
>>> +             }
> 
> This code assumes that slots are filled sequentially and that it is
> not possible to release slots out of order - i.e. if we find a free
> slot, there is not a match in a later slot.
> I can't think how this could happen but a comment to confirm this
> might be needed here.
> 
> When we had 1:1 port / array index then this check was guaranteed
>> Mike

I thought about this but I couldn't see an issue here. The loop always
runs to the end even if a free slot is found so it should find
duplicates in any order. Unless I'm missing some other edge case?

> 
> 
> 
>>> +             if (!i->remote_fwnode && !free_conn)
>>> +                     free_conn = i;
>>> +     }
>>> +
>>> +     if (!free_conn) {
>>
>> and:
>>         /* No free slots */
>>         if (i == pdata->nr_outconns) {
>>
>>> +             pdata->nr_outconns++;
>>> +             ret = coresight_alloc_conns(dev, pdata);
>>> +             if (ret)
>>> +                     return ret;
>>> +             free_conn = &pdata->out_conns[pdata->nr_outconns - 1];
>>> +     }
>>> +
>>
>> and:
>>         pdata->out_conns[i] = *conn;
>>
>>
>> Otherwise looks good to me.
>>
>> Suzuki
>>
>>
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK