| Message ID | 20200805025458.2978-7-tingwei@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: allow to build coresight as modules | expand |
On 08/05/2020 03:54 AM, Tingwei Zhang 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. > Use get_device()/put_device() to protect device data > in the middle of active session. > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > Tested-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > index b7151c5f81b1..1626bc885dfe 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; > Please could you add a pair of helper functions to hold/drop reference to a device/driver ? i.e, static inline bool coresight_get_ref(struct coresight_device *csdev) { struct device *dev = csdev->dev.parent; /* Make sure the driver cant be removed */ if (!try_module_get(dev->driver->owner)) return false; /* Make sure the device can't go away */ get_device(dev); pm_runtime_get_sync(dev); return true; } static inline void coresight_put_ref(struct coresight_device *csdev) { struct device *dev = csdev->dev.parent; pm_runtime_put(dev); put_device(dev); module_put(dev->driver->owner); } > @@ -648,10 +648,30 @@ 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; > + get_device(child->dev.parent); > pm_runtime_get_sync(child->dev.parent); > + } > } This could then be : if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) if (!coresight_get_ref(child)) goto err; > + if (!try_module_get(csdev->dev.parent->driver->owner)) > + goto err; > + get_device(csdev->dev.parent); > pm_runtime_get_sync(csdev->dev.parent); if (coresight_get_ref(csdev)) return 0; > + 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); > + put_device(child->dev.parent); > + module_put(child->dev.parent->driver->owner); > + } And this could be : if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) coresight_put_ref(child); > + } > + return -ENODEV; > } > > /* > @@ -663,12 +683,17 @@ static void coresight_drop_device(struct coresight_device *csdev) > int i; > > pm_runtime_put(csdev->dev.parent); > + put_device(csdev->dev.parent); > + module_put(csdev->dev.parent->driver->owner); coresight_put_ref(csdev); > 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); > + put_device(child->dev.parent); > + module_put(child->dev.parent->driver->owner); > + } coresight_put_ref(child); > } > } > > @@ -687,7 +712,7 @@ static int _coresight_build_path(struct coresight_device *csdev, > struct coresight_device *sink, > struct list_head *path) > { > - int i; > + int i, ret; > bool found = false; > struct coresight_node *node; > > @@ -721,7 +746,11 @@ static int _coresight_build_path(struct coresight_device *csdev, > if (!node) > return -ENOMEM; > > - coresight_grab_device(csdev); > + ret = coresight_grab_device(csdev); > + if (ret) { > + kfree(node); > + return ret; > + } > node->csdev = csdev; > list_add(&node->link, path); > > Cheers Suzuki
On Wed, Aug 05, 2020 at 06:55:40PM +0800, Suzuki K Poulose wrote: > On 08/05/2020 03:54 AM, Tingwei Zhang 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. > >Use get_device()/put_device() to protect device data > >in the middle of active session. > > > >Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > >Tested-by: Mike Leach <mike.leach@linaro.org> > >--- > > drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++---- > > 1 file changed, 34 insertions(+), 5 deletions(-) > > > >diff --git a/drivers/hwtracing/coresight/coresight.c > b/drivers/hwtracing/coresight/coresight.c > >index b7151c5f81b1..1626bc885dfe 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; > > Please could you add a pair of helper functions to hold/drop reference > to a device/driver ? i.e, > > static inline bool coresight_get_ref(struct coresight_device *csdev) > { > struct device *dev = csdev->dev.parent; > > /* Make sure the driver cant be removed */ > if (!try_module_get(dev->driver->owner)) > return false; > /* Make sure the device can't go away */ > get_device(dev); > pm_runtime_get_sync(dev); > return true; > } > Sure. I'll add it to next revision. Thanks, Tingwei > static inline void coresight_put_ref(struct coresight_device *csdev) > { > struct device *dev = csdev->dev.parent; > > pm_runtime_put(dev); > put_device(dev); > module_put(dev->driver->owner); > } > > > >@@ -648,10 +648,30 @@ 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; > >+ get_device(child->dev.parent); > > pm_runtime_get_sync(child->dev.parent); > >+ } > > } > > This could then be : > if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) > if (!coresight_get_ref(child)) > goto err; > > >+ if (!try_module_get(csdev->dev.parent->driver->owner)) > >+ goto err; > >+ get_device(csdev->dev.parent); > > pm_runtime_get_sync(csdev->dev.parent); > > if (coresight_get_ref(csdev)) > return 0; > > >+ 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); > >+ put_device(child->dev.parent); > >+ module_put(child->dev.parent->driver->owner); > >+ } > > And this could be : > > if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) > coresight_put_ref(child); > > >+ } > >+ return -ENODEV; > > } > > /* > >@@ -663,12 +683,17 @@ static void coresight_drop_device(struct > coresight_device *csdev) > > int i; > > pm_runtime_put(csdev->dev.parent); > >+ put_device(csdev->dev.parent); > >+ module_put(csdev->dev.parent->driver->owner); > > coresight_put_ref(csdev); > > > 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); > >+ put_device(child->dev.parent); > >+ module_put(child->dev.parent->driver->owner); > >+ } > > coresight_put_ref(child); > > > } > > } > >@@ -687,7 +712,7 @@ static int _coresight_build_path(struct > coresight_device *csdev, > > struct coresight_device *sink, > > struct list_head *path) > > { > >- int i; > >+ int i, ret; > > bool found = false; > > struct coresight_node *node; > >@@ -721,7 +746,11 @@ static int _coresight_build_path(struct > coresight_device *csdev, > > if (!node) > > return -ENOMEM; > >- coresight_grab_device(csdev); > >+ ret = coresight_grab_device(csdev); > >+ if (ret) { > >+ kfree(node); > >+ return ret; > >+ } > > node->csdev = csdev; > > list_add(&node->link, path); > > > > > Cheers > Suzuki > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index b7151c5f81b1..1626bc885dfe 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,30 @@ 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; + get_device(child->dev.parent); pm_runtime_get_sync(child->dev.parent); + } } + if (!try_module_get(csdev->dev.parent->driver->owner)) + goto err; + get_device(csdev->dev.parent); 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); + put_device(child->dev.parent); + module_put(child->dev.parent->driver->owner); + } + } + return -ENODEV; } /* @@ -663,12 +683,17 @@ static void coresight_drop_device(struct coresight_device *csdev) int i; pm_runtime_put(csdev->dev.parent); + put_device(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); + put_device(child->dev.parent); + module_put(child->dev.parent->driver->owner); + } } } @@ -687,7 +712,7 @@ static int _coresight_build_path(struct coresight_device *csdev, struct coresight_device *sink, struct list_head *path) { - int i; + int i, ret; bool found = false; struct coresight_node *node; @@ -721,7 +746,11 @@ static int _coresight_build_path(struct coresight_device *csdev, if (!node) return -ENOMEM; - coresight_grab_device(csdev); + ret = coresight_grab_device(csdev); + if (ret) { + kfree(node); + return ret; + } node->csdev = csdev; list_add(&node->link, path);