RE: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver

["Mani, Rajmohan" <rajmohan.mani@intel.com>]

Hi Greg,

> Subject: Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM)
> driver

...

> > > > +struct intel_iom {
> > > > +	struct device *dev;
> > > > +	void __iomem *regbar;
> > > > +};
> > > > +
> > > > +static struct intel_iom iom_dev;
> > >
> > > Why just one?  Why is this static?
> > >
> >
> > There is just one IOM device in the system.
> 
> For today, yes, no need to force yourself to have to change this in the future.
> Just use a normal per-instance variable instead please, if you really need it.
> 

Ack

> > > > +
> > > > +	/* Prevent this driver from being unloaded while in use */
> > > > +	if (!try_module_get(dev->driver->owner)) {
> > >
> > > Why are you poking around in a random device's driver's owner?
> > >
> > > That's not ok.  And probably totally racy.
> > >
> > > Handle module reference counts properly, not like this.
> > >
> >
> > Ack. Will use THIS_MODULE here.
> 
> No, that is not the answer, and is always wrong to use :(
> 

This should not matter anymore, as I find reference counting may not be needed.

> > > And why does it even matter that you incremented the module
> > > reference count?  What is that "protecting" you from?
> > >
> >
> > To prevent this driver from being unloaded, while it is being used.
> 
> Why does that matter?  Shouldn't normal reference counting and
> dependancies be all that you need?
> 

I find just dependencies are enough to prevent the driver from being unloaded.

With Intel PMC USB mux control driver, not using intel_iom_get()/intel_iom_put(),
just invoking intel_iom_port_status() is enough for rmmod to report failure
(citing the use by intel_pmc_mux) in unloading the IOM driver.

> > > > +		put_device(iom_dev.dev);
> > > > +		return ERR_PTR(-EBUSY);
> > > > +	}
> > > > +
> > > > +	return &iom_dev;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_get);
> > >
> > > Who calls this function?
> > >
> >
> > Intel PMC USB mux control driver, in this case.
> > The callers are expected to call intel_iom_get() before using the
> > intel_iom_port_status(), after which intel_iom_put() can be called to
> > release the IOM device instance.
> 
> Why not just have a single call if all this driver does is support one thing.  The
> reference counting shouldn't be needed at all, right?
> 

Ack. That looks to be the case, based on my findings.

> > > > +/**
> > > > + * intel_iom_put() - Put IOM device instance
> > > > + * @iom: IOM device instance
> > > > + *
> > > > + * This function releases the IOM device instance created using
> > > > + * intel_iom_get() and allows the driver to be unloaded.
> > > > + *
> > > > + * Call intel_iom_put() to release the instance.
> > > > + */
> > > > +void intel_iom_put(struct intel_iom *iom) {
> > > > +	if (!iom)
> > > > +		return;
> > > > +
> > > > +	module_put(iom->dev->driver->owner);
> > >
> > > And if the device doesn't have a driver?  boom :(
> > >
> > > Don't do this.
> > >
> >
> > Ack. Will use THIS_MODULE here.
> 
> Again, no, that will be even more incorrect.
> 

This shouldn't be relevant anymore.

> > > > +	put_device(iom->dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_put);
> > > > +
> > > > +/**
> > > > + * intel_iom_port_status() - Get status bits for the Type-C port
> > > > + * @iom: IOM device instance
> > > > + * @port: Type-C port number
> > > > + * @status: pointer to receive the status bits
> > > > + *
> > > > + * Returns 0 on success, error otherwise.
> > > > + */
> > > > +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32
> > > > +*status) {
> > > > +	void __iomem *reg;
> > > > +
> > > > +	if (!iom)
> > > > +		return -ENODEV;
> > > > +
> > > > +	if (!status || (port > IOM_MAX_PORTS - 1))
> > > > +		return -EINVAL;
> > > > +
> > > > +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> > > port;
> > > > +
> > > > +	*status = ioread32(reg);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > >
> > > So the whole driver is here just to read, directly from memory, a
> > > single
> > > 32 bit value?
> >
> > Yes.
> 
> Ok, then this whole driver could be about 90% smaller and more obvious.
> Don't add the reference counting, the static variables and all the other stuff
> just to get a 32bit number.
> 

Ack