Re: [PATCH 5/8] staging: vme: add functions for bridge module refcounting

[Martyn Welch <martyn.welch@ge.com>]

On 09/08/11 20:19, Emilio G. Cota wrote:
> On Tue, Aug 09, 2011 at 10:00:09 +0100, Martyn Welch wrote:
>> So, out of the 3 bus types you have used to demonstrate that refcounts should
>> always be handled explicitly, 2 of the 3 (at least to me) appear to implicitly
>> handling refcounting.
> (snip)
>> It appears to me that both PCI and RapidIO (both buses that you have tried to
>> use to defend your position) don't seem to me to by-and-large expect drivers
>> to explicitly manage refcounting.
> 
> Well if that's the impression you get, perhaps you need to
> actually read the code:
> 
> In drivers/rapidio/rio-driver.c:
> 
> /**
>  * rio_dev_get - Increments the reference count of the RIO device structure
>  *
>  * @rdev: RIO device being referenced
>  *
>  * Each live reference to a device should be refcounted.
>  *
>  * Drivers for RIO devices should normally record such references in
>  * their probe() methods, when they bind to a device, and release
>  * them by calling rio_dev_put(), in their disconnect() methods.
>  */
> struct rio_dev *rio_dev_get(struct rio_dev *rdev)
> {
>         if (rdev)
>                 get_device(&rdev->dev);
> 
>         return rdev;
> }
> 
> You can find a similar function (with almost the very same description)
> for usb and pci.
> 

And I think you need to go and do a grep of the code and find out where those
functions are actually used, rather than blindly relying on the comment.

>> That leaves USB, which I'd argue is a very different bus to VME.
> 
> From the device model's viewpoint, VME is not special in any way,
> something you don't seem to understand.
> 
>>> I'm tired of your non-arguments. I'm just trying to persuade you
>>> to do what everyone else is doing, with technical reasons. To me
>>> (and to everybody else in this list, I'd imagine) refcounting
>>> should be explicit. You're going against what I perceive are
>>> well-established pratices in the kernel. I can't understand it.
>>
>> Which I have yet to see you convincingly backup. I see a
>> large amount of bluff about oppses and how buses apparently
>> deal with refcounting.  I also know I have had a number of
>> commits to the VME code by a number of others (which is a
>> matter of public record, you can look at the commits in git)
>> that haven't complained about how the bus code is structured.
> 
> You're in denial. Manohar's patch fixes a very obvious bug,
> which is fixed in the same way other buses deal with this.
> 

Go grep the code.

>> I also know I have had a number of commits to the VME code
>> by a number of others (which is a matter of public record,
>> you can look at the commits in git) that haven't complained
>> about how the bus code is structured.
> 
> The fact that other people didn't notice this bug is irrelevant.
> 

Suitable bug fixes are welcome.

>> You have proposed a completely different structure. Manohar has proposed some
>> changes and I am trying to work with him to find a solution that satisfies
>> both of us.
> 
> ??? I just want you to ack Manohar's patch.
> 

I won't be in it's current form. I'm looking forward to seeing Manohar's
revised patch series.

Martyn

-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189