Messages in this thread | | | Date | Thu, 23 Mar 2023 10:49:30 +0000 | Subject | Re: [PATCH v2 5/9] coresight: Dynamically add connections | From | James Clark <> |
| |
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
| |