From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8CBFC74A5B for ; Wed, 29 Mar 2023 12:02:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229956AbjC2MCi (ORCPT ); Wed, 29 Mar 2023 08:02:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40866 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229485AbjC2MCg (ORCPT ); Wed, 29 Mar 2023 08:02:36 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id AE03BC0 for ; Wed, 29 Mar 2023 05:02:34 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BA80A1FB; Wed, 29 Mar 2023 05:03:18 -0700 (PDT) Received: from [10.57.20.1] (unknown [10.57.20.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5514B3F6C4; Wed, 29 Mar 2023 05:02:33 -0700 (PDT) Message-ID: Date: Wed, 29 Mar 2023 13:02:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 From: James Clark Subject: Re: [PATCH v2 5/9] coresight: Dynamically add connections To: Suzuki K Poulose , coresight@lists.linaro.org Cc: Mathieu Poirier , Mike Leach , Leo Yan , Alexander Shishkin , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20230310160610.742382-1-james.clark@arm.com> <20230310160610.742382-6-james.clark@arm.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16/03/2023 17:12, Suzuki K Poulose 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 >> --- >>   .../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, > This is actually clang-format's doing when using the kernel rules in the root of the repo. I started using it because of some other style comments I got before. Not sure if this time it's just done something bad or it's technically ok. I formatted everything in V3 with it so it should at least all be consistent. >         >> +            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; >> +        } >> +        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 > >