Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

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

Hi Tingwei,

On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
>
> Hi Mike,
>
>  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> > wrote:
> > >
> > > When coresight device is in an active session, driver module of
> > > that device should not be removed. Use try_get_module() in
> > > coresight_grab_device() to prevent module to be unloaded.
> > >
> > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > b/drivers/hwtracing/coresight/coresight.c
> > > index b7151c5f81b1..17bc76ea86ae 100644
> > > --- a/drivers/hwtracing/coresight/coresight.c
> > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > @@ -640,7 +640,7 @@ struct coresight_device
> > *coresight_get_sink_by_id(u32 id)
> > >   * don't appear on the trace path, they should be handled along with
> > the
> > >   * the master device.
> > >   */
> > > -static void coresight_grab_device(struct coresight_device *csdev)
> > > +static int coresight_grab_device(struct coresight_device *csdev)
> > >  {
> > >         int i;
> > >
> > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > coresight_device *csdev)
> > >                 struct coresight_device *child;
> > >
> > >                 child  = csdev->pdata->conns[i].child_dev;
> > > -               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > > +                       if
> > (!try_module_get(child->dev.parent->driver->owner))
> > > +                               goto err;
> > >                         pm_runtime_get_sync(child->dev.parent);
> > > +               }
> > >         }
> > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > +               goto err;
> > >         pm_runtime_get_sync(csdev->dev.parent);
> > > +       return 0;
> > > +err:
> > > +       for (i--; i >= 0; i--) {
> > > +               struct coresight_device *child;
> > > +
> > > +               child  = csdev->pdata->conns[i].child_dev;
> > > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > +                       pm_runtime_put(child->dev.parent);
> > > +       }
> > > +       return -ENODEV;
> > >  }
> > >
> > >  /*
> > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > coresight_device *csdev)
> > >         int i;
> > >
> > >         pm_runtime_put(csdev->dev.parent);
> > > +       module_put(csdev->dev.parent->driver->owner);
> > >         for (i = 0; i < csdev->pdata->nr_outport; i++) {
> > >                 struct coresight_device *child;
> > >
> > >                 child  = csdev->pdata->conns[i].child_dev;
> > > -               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > >                         pm_runtime_put(child->dev.parent);
> > > +                       module_put(child->dev.parent->driver->owner);
> > > +               }
> > >         }
> > >  }
> > >
> > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > coresight_device *csdev,
> > >         if (!node)
> > >                 return -ENOMEM;
> > >
> > > -       coresight_grab_device(csdev);
> > > +       if (coresight_grab_device(csdev))
> > > +               return -ENODEV;
> > >         node->csdev = csdev;
> > >         list_add(&node->link, path);
> > >
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > Forum,
> > > a Linux Foundation Collaborative Project
> > >
> >
> > The CTI devices are associated with other coresight components using
> > csdev->ect_dev; These are not handled in the main linear trace path as
> > these have a star topology interlinking all components.
> > However, when a component has an associated ect_dev then is enabled at
> > the same time as the other component is enabled.
> >
> > So a module_get/put will be needed for this association to prevent the
> > CTI being removed while a trace session is active.
> > I suggest in coresight.c : coresight_control_assoc_ectdev()
> >
>
> In module unload process, devices of that module will be removed. In
> this case, cti_remove() is called on all cti device when unloading
> cti module. All cti connections is cleaned at that time.
> csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> will return since since ect_csdev is NULL.
>
> I think we are safe without holding module reference since cti
> driver has done a pretty good job on clean up.
>

The issue here is not about clean up. We need to keep all the
programmed coresight modules loaded and operational while a coresight
trace session is in progress. The CTI can be used to control the
generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this
programming is lost and the trace data collected may not be what is
expected.

Regards

Mike

> Let me know if you still think we need hold the module reference.
>
> Thanks, Tingwei
> > Regards
> >
> > Mike
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel