[PATCH 0/2] Add Intel Input Output Manager driver

[PATCH 0/2] Add Intel Input Output Manager driver

[Rajmohan Mani]

Hi,

This patch series add support for Intel Input Output Manager (IOM)
driver, which is leveraged by the Intel PMC USB mux control driver.

This patch series has a dependency on the following 4 patches, that
are in Greg's usb-next branch.
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tree/?h=usb-next

ffe82945d8eb usb: typec: intel_pmc_mux: Add support for USB4
cab9219d2af4 usb: typec: intel_pmc_mux: Definitions for response status bits
1a1be50b5ebd usb: typec: Add data structure for Enter_USB message
6701911bb1c1 usb: typec: Combine the definitions for Accessory and USB modes

Heikki Krogerus (1):
  usb: typec: intel_pmc_mux: Check the port status before connect

Rajmohan Mani (1):
  platform/x86: Add Intel Input Output Manager (IOM) driver

 drivers/platform/x86/Kconfig                |  16 +++
 drivers/platform/x86/Makefile               |   1 +
 drivers/platform/x86/intel_iom.c            | 133 ++++++++++++++++++++
 drivers/usb/typec/mux/Kconfig               |   1 +
 drivers/usb/typec/mux/intel_pmc_mux.c       |  73 +++++++++--
 include/linux/platform_data/x86/intel_iom.h |  62 +++++++++
 6 files changed, 276 insertions(+), 10 deletions(-)
 create mode 100644 drivers/platform/x86/intel_iom.c
 create mode 100644 include/linux/platform_data/x86/intel_iom.h

-- 
2.20.1

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

[Rajmohan Mani]

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 +++++++++
 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;
+
+/**
+ * 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);
+
+/**
+ * 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);
+
+static int intel_iom_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *addr;
+
+	addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(addr))
+		return PTR_ERR(addr);
+
+	iom_dev.regbar = addr;
+
+	iom_dev.dev = dev;
+	dev_set_drvdata(dev, &iom_dev);
+	return 0;
+}
+
+static const struct acpi_device_id intel_iom_acpi_ids[] = {
+	{ "INTC1072" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
+
+static struct platform_driver intel_iom_driver = {
+	.probe = intel_iom_probe,
+	.driver = {
+		.name = "intel_iom",
+		.acpi_match_table = intel_iom_acpi_ids,
+	},
+};
+
+module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
+
+MODULE_AUTHOR("Rajmohan Mani <rajmohan.mani@intel.com>");
+MODULE_DESCRIPTION("Intel IOM driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/x86/intel_iom.h b/include/linux/platform_data/x86/intel_iom.h
new file mode 100644
index 000000000000..d86c60b25a44
--- /dev/null
+++ b/include/linux/platform_data/x86/intel_iom.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _PLATFORM_DATA_X86_INTEL_IOM_H_
+#define _PLATFORM_DATA_X86_INTEL_IOM_H_
+
+#include <linux/bits.h>
+#include <linux/types.h>
+
+/* Input Output Manager (IOM) PORT STATUS */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_MASK		GENMASK(9, 6)
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_SHIFT		0x06
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_USB		0x03
+/* activity type: Safe Mode */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_SAFE_MODE		0x04
+/* activity type: Display Port */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_DP		0x05
+/* activity type: Display Port Multi Function Device */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_DP_MFD		0x06
+/* activity type: Thunderbolt */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_TBT		0x07
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_ALT_MODE_USB	0x0c
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_ALT_MODE_TBT_USB	0x0d
+/* Upstream Facing Port Information */
+#define IOM_PORT_STATUS_UFP				BIT(10)
+/* Display Port Hot Plug Detect status */
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_MASK		GENMASK(13, 12)
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_SHIFT		0x0c
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_ASSERT		0x01
+#define IOM_PORT_STATUS_DHPD_HPD_SOURCE_TBT		BIT(14)
+#define IOM_PORT_STATUS_CONNECTED			BIT(31)
+
+#define IOM_MAX_PORTS					4
+/* Register length in bytes */
+#define IOM_REG_LEN					4
+
+struct intel_iom;
+
+#ifdef CONFIG_ACPI
+
+struct intel_iom *intel_iom_get(void);
+void intel_iom_put(struct intel_iom *iom);
+int intel_iom_port_status(struct intel_iom *iom, u8 port, u32 *status);
+
+#else
+
+struct intel_iom *intel_iom_get(void)
+{
+	return NULL;
+}
+
+void intel_iom_put(struct intel_iom *iom)
+{
+}
+
+int intel_iom_port_status(struct intel_iom *iom, u8 port, u32 *status)
+{
+	return 0;
+}
+
+#endif
+
+#endif /* _PLATFORM_DATA_X86_INTEL_IOM_H_ */
-- 
2.20.1

[PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Rajmohan Mani]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The PMC microcontroller that we use for configuration, does
not supply any status information back. For port status we
need to talk to another controller on the board called IOM
(I/O manager).

By checking the port status before configuring the muxes, we
can make sure that we do not reconfigure the port after
bootup when the system firmware (for example BIOS) has
already configured it.

Using the status information also to check if DisplayPort
HPD is still asserted when the cable plug is disconnected,
and clearing it if it is.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/mux/Kconfig         |  1 +
 drivers/usb/typec/mux/intel_pmc_mux.c | 73 +++++++++++++++++++++++----
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index a4dbd11f8ee2..46f5512de63d 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -12,6 +12,7 @@ config TYPEC_MUX_PI3USB30532
 config TYPEC_MUX_INTEL_PMC
 	tristate "Intel PMC mux control"
 	depends on INTEL_SCU_IPC
+	depends on INTEL_IOM
 	select USB_ROLE_SWITCH
 	help
 	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 2aba07c7b221..84101fb99934 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -9,6 +9,7 @@
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/x86/intel_iom.h>
 #include <linux/property.h>
 #include <linux/usb/pd.h>
 #include <linux/usb/role.h>
@@ -83,10 +84,17 @@ enum {
 #define PMC_USB_DP_HPD_LVL		BIT(4)
 #define PMC_USB_DP_HPD_IRQ		BIT(5)
 
+/* IOM Port Status */
+#define IOM_PORT_ACTIVITY_IS(_status_, _type_)				\
+	((((_status_) & IOM_PORT_STATUS_ACTIVITY_TYPE_MASK) >>		\
+	  IOM_PORT_STATUS_ACTIVITY_TYPE_SHIFT) ==			\
+	 (IOM_PORT_STATUS_ACTIVITY_TYPE_##_type_))
+
 struct pmc_usb;
 
 struct pmc_usb_port {
 	int num;
+	u32 iom_status;
 	struct pmc_usb *pmc;
 	struct typec_mux *typec_mux;
 	struct typec_switch *typec_sw;
@@ -105,6 +113,7 @@ struct pmc_usb_port {
 struct pmc_usb {
 	u8 num_ports;
 	struct device *dev;
+	struct intel_iom *iom;
 	struct intel_scu_ipc_dev *ipc;
 	struct pmc_usb_port *port;
 };
@@ -145,18 +154,17 @@ static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
 }
 
 static int
-pmc_usb_mux_dp_hpd(struct pmc_usb_port *port, struct typec_mux_state *state)
+pmc_usb_mux_dp_hpd(struct pmc_usb_port *port, struct typec_displayport_data *dp)
 {
-	struct typec_displayport_data *data = state->data;
 	u8 msg[2] = { };
 
 	msg[0] = PMC_USB_DP_HPD;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
-	if (data->status & DP_STATUS_IRQ_HPD)
+	if (dp->status & DP_STATUS_IRQ_HPD)
 		msg[1] = PMC_USB_DP_HPD_IRQ;
 
-	if (data->status & DP_STATUS_HPD_STATE)
+	if (dp->status & DP_STATUS_HPD_STATE)
 		msg[1] |= PMC_USB_DP_HPD_LVL;
 
 	return pmc_usb_command(port, msg, sizeof(msg));
@@ -169,8 +177,12 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 	struct altmode_req req = { };
 	int ret;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, DP) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, DP_MFD))
+		return 0;
+
 	if (data->status & DP_STATUS_IRQ_HPD)
-		return pmc_usb_mux_dp_hpd(port, state);
+		return pmc_usb_mux_dp_hpd(port, state->data);
 
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
@@ -193,7 +205,7 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 		return ret;
 
 	if (data->status & DP_STATUS_HPD_STATE)
-		return pmc_usb_mux_dp_hpd(port, state);
+		return pmc_usb_mux_dp_hpd(port, state->data);
 
 	return 0;
 }
@@ -205,6 +217,10 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
 	u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
 	struct altmode_req req = { };
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
+		return 0;
+
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 	req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
@@ -239,6 +255,10 @@ pmc_usb_mux_usb4(struct pmc_usb_port *port, struct typec_mux_state *state)
 	struct altmode_req req = { };
 	u8 cable_speed;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
+		return 0;
+
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 	req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
@@ -273,6 +293,9 @@ static int pmc_usb_mux_safe_state(struct pmc_usb_port *port)
 {
 	u8 msg;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, SAFE_MODE))
+		return 0;
+
 	msg = PMC_USB_SAFE_MODE;
 	msg |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -283,6 +306,9 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
 {
 	u8 msg[2];
 
+	if (port->iom_status & IOM_PORT_STATUS_CONNECTED)
+		return 0;
+
 	msg[0] = PMC_USB_CONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -295,8 +321,18 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
 
 static int pmc_usb_disconnect(struct pmc_usb_port *port)
 {
+	struct typec_displayport_data data = { };
 	u8 msg[2];
 
+	if (!(port->iom_status & IOM_PORT_STATUS_CONNECTED))
+		return 0;
+
+	/* Clear DisplayPort HPD if it's still asserted. */
+	if (((port->iom_status & IOM_PORT_STATUS_DHPD_HPD_STATUS_MASK) >>
+	     IOM_PORT_STATUS_DHPD_HPD_STATUS_SHIFT) &
+	    IOM_PORT_STATUS_DHPD_HPD_STATUS_ASSERT)
+		pmc_usb_mux_dp_hpd(port, &data);
+
 	msg[0] = PMC_USB_DISCONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -309,6 +345,11 @@ static int
 pmc_usb_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
 {
 	struct pmc_usb_port *port = typec_mux_get_drvdata(mux);
+	int ret;
+
+	ret = intel_iom_port_status(port->pmc->iom, port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	if (state->mode == TYPEC_STATE_SAFE)
 		return pmc_usb_mux_safe_state(port);
@@ -341,9 +382,11 @@ static int pmc_usb_set_orientation(struct typec_switch *sw,
 				   enum typec_orientation orientation)
 {
 	struct pmc_usb_port *port = typec_switch_get_drvdata(sw);
+	int ret;
 
-	if (port->orientation == orientation)
-		return 0;
+	ret = intel_iom_port_status(port->pmc->iom, port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	port->orientation = orientation;
 
@@ -360,9 +403,11 @@ static int pmc_usb_set_orientation(struct typec_switch *sw,
 static int pmc_usb_set_role(struct usb_role_switch *sw, enum usb_role role)
 {
 	struct pmc_usb_port *port = usb_role_switch_get_drvdata(sw);
+	int ret;
 
-	if (port->role == role)
-		return 0;
+	ret = intel_iom_port_status(port->pmc->iom, port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	port->role = role;
 
@@ -472,6 +517,10 @@ static int pmc_usb_probe(struct platform_device *pdev)
 
 	pmc->dev = &pdev->dev;
 
+	pmc->iom = intel_iom_get();
+	if (IS_ERR(pmc->iom))
+		return PTR_ERR(pmc->iom);
+
 	/*
 	 * For every physical USB connector (USB2 and USB3 combo) there is a
 	 * child ACPI device node under the PMC mux ACPI device object.
@@ -496,6 +545,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
 		typec_mux_unregister(pmc->port[i].typec_mux);
 	}
 
+	intel_iom_put(pmc->iom);
+
 	return ret;
 }
 
@@ -509,6 +560,8 @@ static int pmc_usb_remove(struct platform_device *pdev)
 		typec_mux_unregister(pmc->port[i].typec_mux);
 	}
 
+	intel_iom_put(pmc->iom);
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Greg Kroah-Hartman]

On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> The PMC microcontroller that we use for configuration, does
> not supply any status information back. For port status we
> need to talk to another controller on the board called IOM
> (I/O manager).
> 
> By checking the port status before configuring the muxes, we
> can make sure that we do not reconfigure the port after
> bootup when the system firmware (for example BIOS) has
> already configured it.
> 
> Using the status information also to check if DisplayPort
> HPD is still asserted when the cable plug is disconnected,
> and clearing it if it is.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

You can't just forward on patches from others without also adding your
signed-off-by to them, right?

Please fix up this series and try again.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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.

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

> +
> +/**
> + * 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 matter that you incremented the module reference
count?  What is that "protecting" you from?

> +		put_device(iom_dev.dev);
> +		return ERR_PTR(-EBUSY);
> +	}
> +
> +	return &iom_dev;
> +}
> +EXPORT_SYMBOL_GPL(intel_iom_get);

Who calls this function?

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

> +	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?  Doesn't that seem like a lot of overkill?  Why can't the
caller just do this themselves?

greg k-h

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

[Mani, Rajmohan]

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.

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

Ack. Will add a check for 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.
> 

Ack. Will use THIS_MODULE here.

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

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

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

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

> Doesn't that seem like a lot of overkill?  Why can't the caller just
> do this themselves?
> 

Ack.
Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
So I thought it made sense to keep these 2 devices / drivers apart, despite
the overkill. Heikki also agreed with this approach, given the above.

> greg k-h

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

[Mani, Rajmohan]

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.

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

Ack. Will add a check for 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.
> 

Ack. Will use THIS_MODULE here.

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

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

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

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

> Doesn't that seem like a lot of overkill?  Why can't the caller just
> do this themselves?
> 

Ack.
Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
So I thought it made sense to keep these 2 devices / drivers apart, despite
the overkill. Heikki also agreed with this approach, given the above.

> greg k-h

RE: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Mani, Rajmohan]

Hi Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Thursday, July 16, 2020 12:05 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 2/2] usb: typec: intel_pmc_mux: Check the port status
> before connect
> 
> On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > The PMC microcontroller that we use for configuration, does not supply
> > any status information back. For port status we need to talk to
> > another controller on the board called IOM (I/O manager).
> >
> > By checking the port status before configuring the muxes, we can make
> > sure that we do not reconfigure the port after bootup when the system
> > firmware (for example BIOS) has already configured it.
> >
> > Using the status information also to check if DisplayPort HPD is still
> > asserted when the cable plug is disconnected, and clearing it if it
> > is.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> You can't just forward on patches from others without also adding your
> signed-off-by to them, right?
> 

Sorry I missed this.

> Please fix up this series and try again.
> 

Ack. Will fix this with v2.

> thanks,
> 
> greg k-h

RE: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Mani, Rajmohan]

Hi Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Thursday, July 16, 2020 12:05 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 2/2] usb: typec: intel_pmc_mux: Check the port status
> before connect
> 
> On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > The PMC microcontroller that we use for configuration, does not supply
> > any status information back. For port status we need to talk to
> > another controller on the board called IOM (I/O manager).
> >
> > By checking the port status before configuring the muxes, we can make
> > sure that we do not reconfigure the port after bootup when the system
> > firmware (for example BIOS) has already configured it.
> >
> > Using the status information also to check if DisplayPort HPD is still
> > asserted when the cable plug is disconnected, and clearing it if it
> > is.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> You can't just forward on patches from others without also adding your
> signed-off-by to them, right?
> 

Sorry I missed this.

> Please fix up this series and try again.
> 

Ack. Will fix this with v2.

> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

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

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

[Greg Kroah-Hartman]

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

Re: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Greg Kroah-Hartman]

On Fri, Jul 17, 2020 at 06:04:51AM +0000, Mani, Rajmohan wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Thursday, July 16, 2020 12:05 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 2/2] usb: typec: intel_pmc_mux: Check the port status
> > before connect
> > 
> > On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >
> > > The PMC microcontroller that we use for configuration, does not supply
> > > any status information back. For port status we need to talk to
> > > another controller on the board called IOM (I/O manager).
> > >
> > > By checking the port status before configuring the muxes, we can make
> > > sure that we do not reconfigure the port after bootup when the system
> > > firmware (for example BIOS) has already configured it.
> > >
> > > Using the status information also to check if DisplayPort HPD is still
> > > asserted when the cable plug is disconnected, and clearing it if it
> > > is.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > You can't just forward on patches from others without also adding your
> > signed-off-by to them, right?
> > 
> 
> Sorry I missed this.
> 
> > Please fix up this series and try again.
> > 
> 
> Ack. Will fix this with v2.

Stop and go talk to the proper people in Intel for how to submit patches
to the kernel as you are not following the rules they have documented
for you.

thanks,

greg k-h

Re: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Greg Kroah-Hartman]

On Fri, Jul 17, 2020 at 06:04:51AM +0000, Mani, Rajmohan wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Thursday, July 16, 2020 12:05 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 2/2] usb: typec: intel_pmc_mux: Check the port status
> > before connect
> > 
> > On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >
> > > The PMC microcontroller that we use for configuration, does not supply
> > > any status information back. For port status we need to talk to
> > > another controller on the board called IOM (I/O manager).
> > >
> > > By checking the port status before configuring the muxes, we can make
> > > sure that we do not reconfigure the port after bootup when the system
> > > firmware (for example BIOS) has already configured it.
> > >
> > > Using the status information also to check if DisplayPort HPD is still
> > > asserted when the cable plug is disconnected, and clearing it if it
> > > is.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > You can't just forward on patches from others without also adding your
> > signed-off-by to them, right?
> > 
> 
> Sorry I missed this.
> 
> > Please fix up this series and try again.
> > 
> 
> Ack. Will fix this with v2.

Stop and go talk to the proper people in Intel for how to submit patches
to the kernel as you are not following the rules they have documented
for you.

thanks,

greg k-h

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

[Mani, Rajmohan]

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

[Mani, Rajmohan]

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