RE: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver
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
Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver
On Fri, Jul 17, 2020 at 06:03:55AM +, Mani, Rajmohan wrote: > Hi Greg, > > Thanks for the reviews. > > > -Original Message- > > From: Greg Kroah-Hartman > > Sent: Thursday, July 16, 2020 12:10 AM > > To: Mani, Rajmohan > > Cc: Darren Hart ; Andy Shevchenko > > ; Mika Westerberg > > ; Dmitry Torokhov > > ; Lee Jones ; Ayman > > Bagabas ; Masahiro Yamada > > ; Joseph, Jithu ; Blaž > > Hrastnik ; Srinivas Pandruvada > > ; linux-kernel@vger.kernel.org; > > platform-driver-...@vger.kernel.org; Heikki Krogerus > > ; linux-...@vger.kernel.org; > > pmal...@chromium.org; ble...@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 > > > --- > > > 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)
RE: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver
Hi Greg, Thanks for the reviews. > -Original Message- > From: Greg Kroah-Hartman > Sent: Thursday, July 16, 2020 12:10 AM > To: Mani, Rajmohan > Cc: Darren Hart ; Andy Shevchenko > ; Mika Westerberg > ; Dmitry Torokhov > ; Lee Jones ; Ayman > Bagabas ; Masahiro Yamada > ; Joseph, Jithu ; Blaž > Hrastnik ; Srinivas Pandruvada > ; linux-kernel@vger.kernel.org; > platform-driver-...@vger.kernel.org; Heikki Krogerus > ; linux-...@vger.kernel.org; > pmal...@chromium.org; ble...@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 > > --- > > 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 ..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) &
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
> ---
> 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.
> 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 ..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
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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?
> +
> +/**
> + * intel_iom_get() - Get IOM device instance
> + *
> + * This function returns the IOM device instance. This also ensures that
> + * this driver cannot be unloaded while the caller has the instance.
> + *
> + * Call intel_iom_put() to release the instance.
> + *
> + * Returns IOM device instance on success or error pointer otherwise.
> + */
> +struct intel_iom *intel_iom_get(void)
> +{
> + struct device *dev = get_device(iom_dev.dev);
Wht if dev is NULL?
> +
> + /* 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.
And why does it even matt
[PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver
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
---
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 +
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 ..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define IOM_PORT_STATUS_OFFSET 0x560
+
+struct intel_iom {
+ struct device *dev;
+ void __iomem *regbar;
+};
+
+static struct intel_iom iom_dev;
+
+/**
+ * intel_iom_get() - Get IOM device instance
+ *
+ * This function returns the IOM device instance. This also ensures that
+ * this driver cannot be unloaded while the caller has the instance.
+ *
+ * Call intel_iom_put() to release the instance.
+ *
+ * Returns IOM device instance on success or error pointer otherwise.
+ */
+struct intel_iom *intel_iom_get(void)
+{
+ struct device *dev = get_device(iom_dev.dev);
+
+ /* Prevent this driver from being unloaded while in use */
+ if (!try_module_get(dev->driver->owner)) {
+ put_device(iom_dev.dev);
+ return ERR_PTR(-EBUSY);
+ }
+
+ return &iom_dev;
+}
+EXPORT_SYMBOL_GPL(intel_iom_get);
+
+/**
+ * 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);
+ put_device(iom->dev);
+}
+EXPORT_SYMBOL_GPL(intel_iom_put)
