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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Jul 17, 2020 at 06:03:55AM +0000, Mani, Rajmohan wrote:
> Hi Greg,
> 
> Thanks for the reviews.
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Thursday, July 16, 2020 12:10 AM
> > To: Mani, Rajmohan <rajmohan.mani@intel.com>
> > Cc: Darren Hart <dvhart@infradead.org>; Andy Shevchenko
> > <andy@infradead.org>; Mika Westerberg
> > <mika.westerberg@linux.intel.com>; Dmitry Torokhov
> > <dmitry.torokhov@gmail.com>; Lee Jones <lee.jones@linaro.org>; Ayman
> > Bagabas <ayman.bagabas@gmail.com>; Masahiro Yamada
> > <masahiroy@kernel.org>; Joseph, Jithu <jithu.joseph@intel.com>; Blaž
> > Hrastnik <blaz@mxxn.io>; Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>; linux-kernel@vger.kernel.org;
> > platform-driver-x86@vger.kernel.org; Heikki Krogerus
> > <heikki.krogerus@linux.intel.com>; linux-usb@vger.kernel.org;
> > pmalani@chromium.org; bleung@chromium.org
> > Subject: Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM)
> > driver
> > 
> > On Wed, Jul 15, 2020 at 05:33:09PM -0700, Rajmohan Mani wrote:
> > > Input Output Manager (IOM) is part of the Tiger Lake SoC that
> > > configures the Type-C Sub System (TCSS). IOM is a micro controller
> > > that handles Type-C topology, configuration and PM functions of
> > > various Type-C devices connected on the platform.
> > >
> > > This driver helps read relevant information such as Type-C port status
> > > (whether a device is connected to a Type-C port or not) and the
> > > activity type on the Type-C ports (such as USB, Display Port,
> > > Thunderbolt), for consumption by other drivers.
> > >
> > > Currently intel_iom_port_status() API is exported by this driver, that
> > > has information about the Type-C port status and port activity type.
> > >
> > > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > > ---
> > >  drivers/platform/x86/Kconfig                |  16 +++
> > >  drivers/platform/x86/Makefile               |   1 +
> > >  drivers/platform/x86/intel_iom.c            | 133 ++++++++++++++++++++
> > >  include/linux/platform_data/x86/intel_iom.h |  62 +++++++++
> > 
> > Why do you need a .h file for a single .c file that no one else shares this data?
> > Just put it all in the .c file please.
> > 
> 
> The APIs exported by this driver, are used by the caller (Intel PMC USB mux
> control driver), hence the need for header file.
> 
> > >  4 files changed, 212 insertions(+)
> > >  create mode 100644 drivers/platform/x86/intel_iom.c  create mode
> > > 100644 include/linux/platform_data/x86/intel_iom.h
> > >
> > > diff --git a/drivers/platform/x86/Kconfig
> > > b/drivers/platform/x86/Kconfig index 0581a54cf562..271feddb20ef 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -816,6 +816,22 @@ config INTEL_INT0002_VGPIO
> > >  	  To compile this driver as a module, choose M here: the module will
> > >  	  be called intel_int0002_vgpio.
> > >
> > > +config INTEL_IOM
> > > +	tristate "Intel Input Output Manager (IOM) driver"
> > > +	depends on ACPI && PCI
> > > +	help
> > > +	  This driver helps read relevant information such as Type-C port
> > > +	  status (whether a device is connected to a Type-C port or not)
> > > +	  and the activity type on the Type-C ports (such as USB, Display
> > > +	  Port, Thunderbolt), for consumption by other drivers.
> > > +
> > > +	  Currently intel_iom_port_status() API is exported by this driver,
> > > +	  that has information about the Type-C port status and port activity
> > > +	  type.
> > > +
> > > +	  To compile this driver as a module, choose M here: the module will
> > > +	  be called intel_iom.
> > > +
> > >  config INTEL_MENLOW
> > >  	tristate "Thermal Management driver for Intel menlow platform"
> > >  	depends on ACPI_THERMAL
> > > diff --git a/drivers/platform/x86/Makefile
> > > b/drivers/platform/x86/Makefile index 2b85852a1a87..d71e4620a7c6
> > > 100644
> > > --- a/drivers/platform/x86/Makefile
> > > +++ b/drivers/platform/x86/Makefile
> > > @@ -76,6 +76,7 @@ intel_cht_int33fe-objs			:=
> > intel_cht_int33fe_common.o \
> > >  					   intel_cht_int33fe_microb.o
> > >  obj-$(CONFIG_INTEL_HID_EVENT)		+= intel-hid.o
> > >  obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
> > > +obj-$(CONFIG_INTEL_IOM)			+= intel_iom.o
> > >  obj-$(CONFIG_INTEL_MENLOW)		+= intel_menlow.o
> > >  obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
> > >  obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
> > > diff --git a/drivers/platform/x86/intel_iom.c
> > > b/drivers/platform/x86/intel_iom.c
> > > new file mode 100644
> > > index 000000000000..ece0fe720b2d
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/intel_iom.c
> > > @@ -0,0 +1,133 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Intel Core SoC Input Output Manager (IOM) driver.
> > > + *
> > > + * This driver provides access to the Input Output Manager (IOM)
> > > +(that
> > > + * is part of Tiger Lake SoC) registers that can be used to know
> > > +about
> > > + * Type-C Sub System related information (such as Type-C port status,
> > > + * activity type on Type-C ports).
> > > + *
> > > + * Copyright (C) 2020, Intel Corporation
> > > + * Author: Rajmohan Mani <rajmohan.mani@intel.com>  */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_data/x86/intel_iom.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#define IOM_PORT_STATUS_OFFSET				0x560
> > > +
> > > +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.

> > > +
> > > +	/* 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 :(

> > 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?

> > > +		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?

> > > +/**
> > > + * 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.

> > > +	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.

As it is, this feels like an excuse to get paid per lines of code :(

greg k-h