Re: [PATCH 4/6] staging: vme: add functions for bridge modulerefcounting

[Emilio G. Cota]

On Wed, Aug 10, 2011 at 11:09:56 +0100, Martyn Welch wrote:
> On 10/08/11 10:33, Manohar Vanga wrote:
> >  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> > @@ -1525,9 +1573,13 @@ static int vme_bus_probe(struct device *dev)
> >  	driver = dev_to_vme_driver(dev);
> >  	bridge = dev_to_bridge(dev);
> >  
> > +	vme_bridge_get(bridge->num);

Here the return pointer is not being checked, although things
would need to be very wrong for this to fail here.

> >  	if (driver->probe != NULL)
> >  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> >  
> > +	if (retval)
> > +		vme_bridge_put(bridge);
> > +
> 
> Given that we are recounting automatically in probe and remove, under what
> circumstances would we need to call them separately?

I agree, it's hard to imagine a scenario where "doubly locking"
the module would make any sense at all.

It would also be nice to:

- Increment the refcount of the bridge device with get_device(),
  this way we protect against the device removal as well.
  Bonus points if we increment the bridge module's refcount
  once per device driver using it and the bridge device's
  refcount once per device sitting on it.

- re-think what the return value of vme_bridge_get should be;
  guess my original idea was to return a reference to a bridge,
  which would be safely used by device drivers from there
  on. Since apparently we're not going this way, then perhaps
  an int (returning 0 on success and -ENXIO on failure) would
  be clearer.

		Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/