Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Vinod Koul]

On 06-05-19, 09:48, Pierre-Louis Bossart wrote:

> > > +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> > > +{
> > > +	if (d)
> > > +		return d->fs;
> > > +	return NULL;
> > > +}
> > > +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
> > 
> > _GPL()?
> 
> Oops, that's a big miss. will fix, thanks for spotting this.

Not really. The Soundwire code is dual licensed. Many of the soundwire
symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
'linux' specific so can be made _GPL.

Pierre, does Intel still care about this being dual licensed or not?

> 
> > 
> > But why is this exported at all?  No one calls this function.
> 
> I will have to check.

It is used by codec driver which are not upstream yet. So my suggestion
would be NOT to export this and only do so when we have users for it
That would be true for other APIs exported out as well.

> > 
> > > +struct sdw_slave_debugfs {
> > > +	struct sdw_slave *slave;
> > 
> > Same question as above, why do you need this pointer?
> 
> will check.

The deubugfs code does hold a ref to slave object to read the data and
dump, this particular instance might be able to get rid of (should be
doable)

> > And meta-comment, if you _EVER_ save off a pointer to a reference
> > counted object (like this and the above one), you HAVE to grab a
> > reference to it, otherwise it can go away at any point in time as that
> > is the point of reference counted objects.
> > 
> > So even if you do need/want this, you have to properly handle the
> > reference count by incrementing/decrementing it as needed.

Yes, but then device exit routine is supposed to do debugfs cleanup as
well, so that would ensure these references are dropped at that point of
time. Greg should that not take care of it or we *should* always do
refcounting.

Thanks
-- 
~Vinod