Messages in this thread | ![/](/images/icornerl.gif) | | From | Mike Leach <> | Date | Tue, 21 Mar 2023 17:56:16 +0000 | Subject | Re: [PATCH v2 5/9] coresight: Dynamically add connections |
| |
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
> > + 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
| ![\](/images/icornerr.gif) |