Re: [PATCH v11 3/4] add FPGA manager core

[atull <atull@opensource.altera.com>]

On Wed, 23 Sep 2015, Josh Cartwright wrote:

> On Wed, Sep 23, 2015 at 12:10:13PM -0500, atull wrote:
> > On Tue, 22 Sep 2015, Josh Cartwright wrote:
> [..]
> > > > +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> > > > +{
> > > > +	struct fpga_manager *mgr;
> > > > +	struct device *dev;
> > > > +
> > > > +	if (!node)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	dev = class_find_device(fpga_mgr_class, NULL, node,
> > > > +				fpga_mgr_of_node_match);
> > > > +	if (!dev)
> > > > +		return ERR_PTR(-ENODEV);
> > > > +
> > > > +	mgr = to_fpga_manager(dev);
> > > > +	put_device(dev);
> > > 
> > > Who's ensuring the FPGA manager device's lifetime between _get and _put?
> > 
> > Well... get_device and put_device are not sufficient?
> 
> Sorry if I was too opaque.
> 
> You've just put_device()'d the only reference to the device you had
> here, so nothing is preventing from the device from being ripped away
> while it's being used.
> 
> You should remove the put_device() call here, and add a corresponding
> put_device() in fpga_mgr_put().
> 
> The module refcounting is also a bit strange, as you are
> acquiring/releasing a reference to THIS_MODULE, but you also should care
> about the lower-level driver's module refcount.
> 

Good call.  Thanks for the clarification.  I'll have something more robust
in v12.

> [..]
> > > > +	dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> > > > +	if (dt_label)
> > > > +		ret = dev_set_name(&mgr->dev, "%s", dt_label);
> > > > +	else
> > > > +		ret = dev_set_name(&mgr->dev, "fpga%d", id);
> > > 
> > > I'm wondering if an alias {} node is better for this.
> > 
> > I could look into that.  Is there an example of that you particularly
> > like for this?
> 
> Look at i2c's usage of the of_alias_*() functions.

OK yes, will use that in the next version.

Thanks for the review,
Alan

> 
>   Josh
>