Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572988 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/9] staging: fsl-mc: fix device ref counting Greg KH <gregkh@linuxfoundation.org> - 2017-02-03 11:00 +0100
    Re: [PATCH 2/9] staging: fsl-mc: fix device ref counting Laurentiu Tudor <laurentiu.tudor@nxp.com> - 2017-02-03 11:20 +0100
      Re: [PATCH 2/9] staging: fsl-mc: fix device ref counting Greg KH <gregkh@linuxfoundation.org> - 2017-02-03 11:40 +0100

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

On Wed, Feb 01, 2017 at 05:43:22AM -0600, laurentiu.tudor@nxp.com wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> 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@nxp.com>
> ---
>  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...

thanks,

greg k-h

[toc] | [next] | [standalone]

#1573000

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@nxp.com wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> 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@nxp.com>
>> ---
>>   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.

---
Best Regards, Laurentiu

[toc] | [prev] | [next] | [standalone]

#1573019

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@nxp.com wrote:
> >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >>
> >> 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@nxp.com>
> >> ---
> >>   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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web