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

[Greg Kroah-Hartman]

On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote:
> Hi Greg,
> 
> On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 23, 2020 at 12:27:48PM +0800, 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.
> > 
> > Are you sure this works?  Why is it needed at all?  Why not just tear
> > down the children properly when a module is removed so that you don't
> > need this at all?
> 
> Using the terms parent and child is somewhat ambiguous...  This is not a
> parent-child relationship but simply an association between devices, something
> like port 1 on device "parent" is connected to port 2 on device "child".  The
> parent-child nomenclature was chosen to reflect that a device appears before
> another in a coresight path.  Otherwise there is no other relation between
> devices, hence the choice of using try_get_module()/put_module() to prevent
> drivers from being taken away.  I'd be happy to proceed differently but haven't
> found better options.
> 
> Going back to parent/child, we could have chosen left/right, up/down or A/B, all
> of which are just as confusion. 

Ok, thanks.

But this causes confusion for everyone as seen below:

> > > Signed-off-by: Tingwei Zhang <tingwei at 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))
> > 
> > Why the child's parent?  Why not the child itself?
> 
> The device structure of each coresight_device is not associated with a driver.
> It is there to take advantages of device goodies such as dev.type, dev.group,
> dev.release and dev.bus.  Coresight IP blocks are discovered on the AMBA bus and as
> such amba_device::dev::driver holds the driver itself.  In coresight_register()
> the association coresigth::dev::parent = amba_device::dev is made.
> 
> > 
> > 
> > > +				goto err;
> > 
> > What about the error given to you here?  Why throw that away?
> > 
> > >  			pm_runtime_get_sync(child->dev.parent);
> > > +		}
> > >  	}
> > > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > > +		goto err;
> > 
> > You don't reduce the child's parent's driver owner module reference
> > here?
> 
> Here @parent is referencing the current device.  Now that helper devices
> connected to any of its outgoing ports have been enabled (and a reference count 
> to the helper device driver incremented), a reference count to the current device
> driver can also be incremented. 

I mean the fact that your error handling does not seem to roll back the
module reference count you got up above in the other loop.

Or if it does, it's really really not obvious, and should at the very
least, be commented as to how it's all cleaning up properly.

thanks,

greg k-h