Re: [PATCH 2/9] staging: fsl-mc: fix device ref counting

[Greg KH]

On Fri, Feb 03, 2017 at 10:17:53AM +0000, Laurentiu Tudor wrote:
> Hi Greg,
> 
> Thanks for having a look. Comment below.
> 
> On 02/03/2017 11:56 AM, Greg KH wrote:
> > On Wed, Feb 01, 2017 at 05:43:22AM -0600, laurentiu.tudor@xxxxxxx wrote:
> >> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
> >>
> >> Drop unneeded get_device() call at device creation
> >> and, as per documentation, drop reference count
> >> after using device_find_child() return.
> >>
> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
> >> ---
> >>   drivers/staging/fsl-mc/bus/dprc-driver.c | 1 +
> >>   drivers/staging/fsl-mc/bus/fsl-mc-bus.c  | 1 -
> >>   2 files changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
> >> index 4e416d8..e4b0341 100644
> >> --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
> >> +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
> >> @@ -188,6 +188,7 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
> >>   		child_dev = fsl_mc_device_lookup(obj_desc, mc_bus_dev);
> >>   		if (child_dev) {
> >>   			check_plugged_state_change(child_dev, obj_desc);
> >> +			put_device(&child_dev->dev);
> >>   			continue;
> >>   		}
> >>
> >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> index cc20dc4..7c6a43b 100644
> >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> @@ -537,7 +537,6 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> >>   		goto error_cleanup_dev;
> >>   	}
> >>
> >> -	(void)get_device(&mc_dev->dev);
> >
> > This implies that your device reference counting is totally wrong and
> > messed up.  Does this fix anything?  Break anything?  It should do
> > something different now...
> 
> It fixes the refcounting in the sense that I'm now seeing the error
> that i think you were referring to in your previous reviews,
> when we hot unplug a device:
> 
> "Device 'foo.N' does not have a release() function, it is broken and 
> must be fixed."
> 
> See next patch that adds the required callback.
> 
> Regarding this particular get_device(), i have no clue why the
> original author placed it here. I've looked over other bus 
> implementations and didn't see something similar.

Ah, that makes more sense, thanks.  I've now applied this and the next
patch and will wait for you to respin the rest.

thanks,

greg k-h