Re: [PATCH v4 5/6] coresight: cti: Add in sysfs links to other coresight devices.

[Mike Leach <mike.leach@linaro.org>]

Hi Mathieu,

On Fri, 14 Feb 2020 at 22:58, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Tue, Feb 11, 2020 at 10:58:07AM +0000, Mike Leach wrote:
> > Adds in sysfs links for connections where the connected device is another
> > coresight device. This allows examination of the coresight topology.
> >
> > Non-coresight connections remain just as a reference name.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/coresight-cti.c | 41 ++++++++++++++++++++-
> >  1 file changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> > index 9e18e176831c..f620e9460e7d 100644
> > --- a/drivers/hwtracing/coresight/coresight-cti.c
> > +++ b/drivers/hwtracing/coresight/coresight-cti.c
> > @@ -441,6 +441,37 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,
> >       return err;
> >  }
> >
> > +static void cti_add_sysfs_link(struct cti_drvdata *drvdata,
> > +                            struct cti_trig_con *tc)
> > +{
> > +     struct coresight_sysfs_link link_info;
> > +
> > +     link_info.orig = drvdata->csdev;
> > +     link_info.orig_name = tc->con_dev_name;
> > +     link_info.target = tc->con_dev;
> > +     link_info.target_name = dev_name(&drvdata->csdev->dev);
> > +     coresight_add_sysfs_link(&link_info);
>
> I understand there isn't much to do if a problem occurs so just catch the error
> and add a comment to assert you're doing this on purpose.
>

When I revisited this code I saw an imbalance between the case where
the CTI is created first and the associated CSdev is created first.
The result could be shutdown path where the CTI removes sysfs links
after the csdev has been removed - which really shouldn't happen.
Also we could try to remove a sysfs link that we failed to set in the
first place - again not ideal

I've reworked this code to fix this, and now if the sysfs link fails
to be set, then we do not set the association between CTI and csdev
components.
This is not sufficient to fail either component from registering, as
we may have successfully registered previous associations with the
same CTI.

It seems unlikely that the sysfs link could fail, but if it does, is
it worth using a dev_warn() to at least log the failure?

Regards

Mike


> > +}
> > +
> > +static void cti_remove_all_sysfs_links(struct cti_drvdata *drvdata)
> > +{
> > +     struct cti_trig_con *tc;
> > +     struct cti_device *ctidev = &drvdata->ctidev;
> > +     struct coresight_sysfs_link link_info;
> > +
> > +     /* origin device and target link name constant for this cti */
> > +     link_info.orig = drvdata->csdev;
> > +     link_info.target_name = dev_name(&drvdata->csdev->dev);
> > +
> > +     list_for_each_entry(tc, &ctidev->trig_cons, node) {
> > +             if (tc->con_dev) {
> > +                     link_info.target = tc->con_dev;
> > +                     link_info.orig_name = tc->con_dev_name;
> > +                     coresight_remove_sysfs_link(&link_info);
> > +             }
> > +     }
> > +}
> > +
> >  /*
> >   * Look for a matching connection device name in the list of connections.
> >   * If found then swap in the csdev name, set trig con association pointer
> > @@ -452,6 +483,8 @@ cti_match_fixup_csdev(struct cti_device *ctidev, const char *node_name,
> >  {
> >       struct cti_trig_con *tc;
> >       const char *csdev_name;
> > +     struct cti_drvdata *drvdata = container_of(ctidev, struct cti_drvdata,
> > +                                                ctidev);
> >
> >       list_for_each_entry(tc, &ctidev->trig_cons, node) {
> >               if (tc->con_dev_name) {
> > @@ -462,6 +495,7 @@ cti_match_fixup_csdev(struct cti_device *ctidev, const char *node_name,
> >                                       devm_kstrdup(&csdev->dev, csdev_name,
> >                                                    GFP_KERNEL);
> >                               tc->con_dev = csdev;
> > +                             cti_add_sysfs_link(drvdata, tc);
> >                               return true;
> >                       }
> >               }
> > @@ -546,10 +580,12 @@ static void cti_update_conn_xrefs(struct cti_drvdata *drvdata)
> >       struct cti_device *ctidev = &drvdata->ctidev;
> >
> >       list_for_each_entry(tc, &ctidev->trig_cons, node) {
> > -             if (tc->con_dev)
> > +             if (tc->con_dev) {
> >                       /* set tc->con_dev->ect_dev */
> >                       coresight_set_assoc_ectdev_mutex(tc->con_dev,
> >                                                        drvdata->csdev);
> > +                     cti_add_sysfs_link(drvdata, tc);
> > +             }
> >       }
> >  }
> >
> > @@ -602,6 +638,9 @@ static void cti_device_release(struct device *dev)
> >       mutex_lock(&ect_mutex);
> >       cti_remove_conn_xrefs(drvdata);
> >
> > +     /* clear the dynamic sysfs associate with connections */
>
> s/associate/associated
>
> > +     cti_remove_all_sysfs_links(drvdata);
> > +
> >       /* remove from the list */
> >       list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) {
> >               if (ect_item == drvdata) {
>
> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> > --
> > 2.17.1
> >



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK