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 5C683C433FE for ; Tue, 8 Mar 2022 13:36:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347160AbiCHNhf (ORCPT ); Tue, 8 Mar 2022 08:37:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42150 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347141AbiCHNhc (ORCPT ); Tue, 8 Mar 2022 08:37:32 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id CF6CF496A6; Tue, 8 Mar 2022 05:36:34 -0800 (PST) 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 83721139F; Tue, 8 Mar 2022 05:36:34 -0800 (PST) Received: from [10.57.22.11] (unknown [10.57.22.11]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 62C083FA58; Tue, 8 Mar 2022 05:36:32 -0800 (PST) Message-ID: Date: Tue, 8 Mar 2022 13:36:30 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH v2] coresight: core: Fix coresight device probe failure issue From: Suzuki K Poulose To: Mao Jinlong , Mathieu Poirier , Mike Leach , Leo Yan , Alexander Shishkin Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Tingwei Zhang , Yuanfang Zhang , Tao Zhang , Hao Zhang References: <20220304082350.30069-1-quic_jinlmao@quicinc.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/03/2022 11:56, Suzuki K Poulose wrote: > Hi Jinlong > > On 04/03/2022 08:23, Mao Jinlong wrote: >> It is possibe that probe failure issue happens when the device >> and its child_device's probe happens at the same time. >> In coresight_make_links, has_conns_grp is true for parent, but >> has_conns_grp is false for child device as has_conns_grp is set >> to true in coresight_create_conns_sysfs_group. The probe of parent >> device will fail at this condition. Add has_conns_grp check for >> child device before make the links and make the process from >> device_register to connection_create be atomic to avoid this >> probe failure issue. >> >> Suggested-by: Suzuki K Poulose >> Suggested-by: Mike Leach >> Signed-off-by: Mao Jinlong >> --- >>   drivers/hwtracing/coresight/coresight-core.c | 12 ++++++------ >>   1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-core.c >> b/drivers/hwtracing/coresight/coresight-core.c >> index 88653d1c06a4..b3e3bc59c09b 100644 >> --- a/drivers/hwtracing/coresight/coresight-core.c >> +++ b/drivers/hwtracing/coresight/coresight-core.c >> @@ -1382,7 +1382,7 @@ static int coresight_fixup_device_conns(struct >> coresight_device *csdev) >>               continue; >>           conn->child_dev = >>               coresight_find_csdev_by_fwnode(conn->child_fwnode); >> -        if (conn->child_dev) { >> +        if (conn->child_dev && conn->child_dev->has_conns_grp) { >>               ret = coresight_make_links(csdev, conn, >>                              conn->child_dev); >>               if (ret) >> @@ -1594,7 +1594,8 @@ struct coresight_device >> *coresight_register(struct coresight_desc *desc) >>       refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL); >>       if (!refcnts) { >>           ret = -ENOMEM; >> -        goto err_free_csdev; >> +        kfree(csdev); >> +        goto err_out; >>       } >>       csdev->refcnt = refcnts; >> @@ -1619,8 +1620,10 @@ struct coresight_device >> *coresight_register(struct coresight_desc *desc) >>       csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev)); >>       dev_set_name(&csdev->dev, "%s", desc->name); >> +    mutex_lock(&coresight_mutex); >>       ret = device_register(&csdev->dev); >>       if (ret) { >> +        mutex_unlock(&coresight_mutex); >>           put_device(&csdev->dev); >>           /* >>            * All resources are free'd explicitly via >> @@ -1634,6 +1637,7 @@ struct coresight_device >> *coresight_register(struct coresight_desc *desc) >>           ret = etm_perf_add_symlink_sink(csdev); >>           if (ret) { >> +            mutex_unlock(&coresight_mutex); >>               device_unregister(&csdev->dev); >>               /* >>                * As with the above, all resources are free'd >> @@ -1645,8 +1649,6 @@ struct coresight_device >> *coresight_register(struct coresight_desc *desc) >>           } >>       } >> -    mutex_lock(&coresight_mutex); >> - >>       ret = coresight_create_conns_sysfs_group(csdev); >>       if (!ret) >>           ret = coresight_fixup_device_conns(csdev); >> @@ -1663,8 +1665,6 @@ struct coresight_device >> *coresight_register(struct coresight_desc *desc) >>       return csdev; >> -err_free_csdev: >> -    kfree(csdev); >>   err_out: >>       /* Cleanup the connection information */ >>       coresight_release_platform_data(NULL, desc->pdata); > > Could we consolidate the unlock sequence to a single point with > something like this (untested): > > > diff --git a/drivers/hwtracing/coresight/coresight-core.c > b/drivers/hwtracing/coresight/coresight-core.c > index af00dca8d1ac..198ee140c6e6 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1571,6 +1571,7 @@ struct coresight_device *coresight_register(struct > coresight_desc *desc) >      int nr_refcnts = 1; >      atomic_t *refcnts = NULL; >      struct coresight_device *csdev; > +    bool registered = false; > >      csdev = kzalloc(sizeof(*csdev), GFP_KERNEL); >      if (!csdev) { > @@ -1591,7 +1592,8 @@ struct coresight_device *coresight_register(struct > coresight_desc *desc) >      refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL); >      if (!refcnts) { >          ret = -ENOMEM; > -        goto err_free_csdev; > +        kfree(csdev); > +        goto err_out; >      } > >      csdev->refcnt = refcnts; > @@ -1616,6 +1618,13 @@ struct coresight_device > *coresight_register(struct coresight_desc *desc) >      csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev)); >      dev_set_name(&csdev->dev, "%s", desc->name); > > +    /* > +     * Make sure the device registration and the connection fixup > +     * are synchronised, so that we don't see uninitialised devices > +     * on the coresight bus while trying to resolve the connections. > +     */ > +    mutex_lock(&coresight_mutex); > + >      ret = device_register(&csdev->dev); >      if (ret) { >          put_device(&csdev->dev); > @@ -1623,7 +1632,7 @@ struct coresight_device *coresight_register(struct > coresight_desc *desc) >           * All resources are free'd explicitly via >           * coresight_device_release(), triggered from put_device(). >           */ > -        goto err_out; > +        goto out_unlock; >      } > >      if (csdev->type == CORESIGHT_DEV_TYPE_SINK || > @@ -1638,11 +1647,11 @@ struct coresight_device > *coresight_register(struct coresight_desc *desc) >               * from put_device(), which is in turn called from >               * function device_unregister(). >               */ > -            goto err_out; > +            goto out_unlock; >          } >      } > - > -    mutex_lock(&coresight_mutex); > +    /* Device is now registered */ > +    registered = true; > >      ret = coresight_create_conns_sysfs_group(csdev); >      if (!ret) > @@ -1652,16 +1661,15 @@ struct coresight_device > *coresight_register(struct coresight_desc *desc) >      if (!ret && cti_assoc_ops && cti_assoc_ops->add) >          cti_assoc_ops->add(csdev); > > +out_unlock: >      mutex_unlock(&coresight_mutex); > -    if (ret) { > +    /* Success */ > +    if (!ret) > +        return csdev; > + > +    /* Unregister the device if needed */ > +    if (registered) >          coresight_unregister(csdev); > -        return ERR_PTR(ret); > -    } Apologies, this should be preserved : i.e, if (registered) { coresight_unregister(csdev); return ERR_PTR(ret); } Thanks Suzuki