Re: [RFC 06/14] SoundWire: Add register/unregister APIs

[Vinod Koul]

On Mon, Nov 14, 2016 at 01:37:51PM +0000, Mark Brown wrote:
> On Fri, Oct 21, 2016 at 06:11:04PM +0530, Hardik Shah wrote:
> 
> > +static void sdw_mstr_release(struct device *dev)
> > +{
> > +	struct sdw_master *mstr = to_sdw_master(dev);
> > +
> > +	complete(&mstr->slv_released_complete);
> > +}
> 
> Other buses don't do this...  this is a big warning sign that you're
> abusing the driver model.

The whole master enumeration stuff, as we discussed in LPC will go away now.
So it will be more like other buses :)

> > +/**
> > + * sdw_slv_verify - return parameter as sdw_slave, or NULL
> > + * @dev: device, probably from some driver model iterator
> > + *
> > + * When traversing the driver model tree, perhaps using driver model
> > + * iterators like @device_for_each_child(), you can't assume very much
> > + * about the nodes you find. Use this function to avoid oopses caused
> > + * by wrongly treating some non-SDW device as an sdw_slave.
> > + */
> 
> This is also *very* scary, especially given that there's no analysis
> presented as to why there might be random other things on the bus.  Why
> does SoundWire need this when other buses don't?

Sure I will double check on this one and sounds to me we cna remove this..

> 
> > +static struct sdw_slave *sdw_slv_verify(struct device *dev)
> > +{
> > +	return (dev->type == &sdw_slv_type)
> > +			? to_sdw_slave(dev)
> > +			: NULL;
> 
> This is needlessly obfuscated, if you want to write an if statement
> write an if statement.

Sure thing

> > +static int sdw_slv_match(struct device *dev, struct device_driver *driver)
> > +{
> > +	struct sdw_slave *sdw_slv;
> > +	struct sdw_driver *sdw_drv = to_sdw_driver(driver);
> > +	struct sdw_slave_driver *drv;
> > +	int ret = 0;
> > +
> > +
> > +	if (sdw_drv->driver_type != SDW_DRIVER_TYPE_SLAVE)
> > +		return ret;
> 
> Why do we need a check like this?

Since folks were doing both slave and master matches, this was done to be
double sure, but this will go away now.

> > +static int sdw_mstr_probe(struct device *dev)
> > +{
> > +	const struct sdw_master_driver *sdrv =
> > +					to_sdw_master_driver(dev->driver);
> > +	struct sdw_master *mstr = to_sdw_master(dev);
> > +	int ret;
> > +
> > +	ret = dev_pm_domain_attach(dev, true);
> > +
> > +	if (ret != -EPROBE_DEFER) {
> > +		ret = sdrv->probe(mstr, sdw_match_mstr(sdrv->id_table, mstr));
> > +		if (ret < 0)
> > +			dev_pm_domain_detach(dev, true);
> > +	}
> 
> This looks *very* broken.  Surely if we fail to attach a pm_domain for
> any reason other than one not being there to attach we shouldn't be
> trying to probe the device?

Yes I agree, we shouldnt be doing probing in that case. Will fix that up.

> 
> > +EXPORT_SYMBOL_GPL(snd_sdw_master_register_driver);
> 
> This is EXPORT_SYMBOL_GPL() but the bus itself is dual licensed GPL/BSD
> - seems a bit inconsistent.

Thanks for pointing this out. The symbols should use EXPORT_SYMBOL() only
and the ones which link to other kernel GPL ones would need to be GPL ones.

> > +/**
> > + * snd_sdw_master_add: Registers the SoundWire Master interface. This needs
> > + *	to be called for each Master interface supported by SoC. This
> > + *	represents One clock and data line (Optionally multiple data lanes)
> > + *	of Master interface.
> > + *
> > + * @master: the Master to be added.
> > + */
> > +int snd_sdw_master_add(struct sdw_master *master)
> 
> This lies at the heart of the issues that seem to exist with the misuse
> of the driver model in this code.  Normally what we see is that the
> controller would instantiate as whatever bus type the controller is
> attached by (typically a PCI or platform device) and then it wouild
> register a bus with the bus subsystem which would then instantiate
> slaves.  Instead we have this system where the bus is registered by
> something in the system and then the master is a driver on the bus
> parallel to the slaves but with a separate driver type that causes
> confusion.  Without having seen a master driver it's not even clear how
> this is going to work and allow the master to talk to its own hardware.

Yes as discussed the whole master stuff will eb redone so you wont see these
bits in next rev.

-- 
~Vinod

Attachment: signature.asc
Description: Digital signature