[v10,3/7] i2c: fsi: Add port structures

Message ID 1528918579-27602-4-git-send-email-eajames@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • i2c: Add FSI-attached I2C master algorithm
Related show

Commit Message

Eddie James June 13, 2018, 7:36 p.m.
Add and initialize I2C adapters for each port on the FSI-attached I2C
master. Ports for each master are defined in the devicetree.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Comments

Benjamin Herrenschmidt June 20, 2018, 3:34 a.m. | #1
On Wed, 2018-06-13 at 14:36 -0500, Eddie James wrote:
>  }
>  
> +static int fsi_i2c_remove(struct device *dev)
> +{
> +       struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> +       struct fsi_i2c_port *port;
> +
> +       list_for_each_entry(port, &i2c->ports, list) {
> +               i2c_del_adapter(&port->adapter);
> +               kfree(port);
> +       }
> +
> +       return 0;
> +}
> +

This is a use-after-free, the list linkage of the freed port is used to
get to the next one. With memory poisoning, kbooom !

You can fold that in:

From f9d9092160897e7308f6990067a03e937339537f Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 20 Jun 2018 13:27:32 +1000
Subject: [PATCH] i2c: fsi: Fix use after free

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/i2c/busses/i2c-fsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 713959b44403..ff69ab6aa79a 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -696,9 +696,10 @@ static int fsi_i2c_probe(struct device *dev)
 static int fsi_i2c_remove(struct device *dev)
 {
 	struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
-	struct fsi_i2c_port *port;
+	struct fsi_i2c_port *port, *tmp;
 
-	list_for_each_entry(port, &i2c->ports, list) {
+	list_for_each_entry_safe(port,tmp, &i2c->ports, list) {
+		list_del(&port->list);
 		i2c_del_adapter(&port->adapter);
 		kfree(port);
 	}
Joel Stanley June 20, 2018, 3:59 a.m. | #2
On 20 June 2018 at 13:04, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2018-06-13 at 14:36 -0500, Eddie James wrote:
>>  }
>>
>> +static int fsi_i2c_remove(struct device *dev)
>> +{
>> +       struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
>> +       struct fsi_i2c_port *port;
>> +
>> +       list_for_each_entry(port, &i2c->ports, list) {
>> +               i2c_del_adapter(&port->adapter);
>> +               kfree(port);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>
> This is a use-after-free, the list linkage of the freed port is used to
> get to the next one. With memory poisoning, kbooom !
>
> You can fold that in:
>
> From f9d9092160897e7308f6990067a03e937339537f Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Wed, 20 Jun 2018 13:27:32 +1000
> Subject: [PATCH] i2c: fsi: Fix use after free

This fixes the issue I was seeing. For the series:

Tested-by: Joel Stanley <joel@jms.id.au>

Thanks,

Joel


>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/i2c/busses/i2c-fsi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index 713959b44403..ff69ab6aa79a 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -696,9 +696,10 @@ static int fsi_i2c_probe(struct device *dev)
>  static int fsi_i2c_remove(struct device *dev)
>  {
>         struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> -       struct fsi_i2c_port *port;
> +       struct fsi_i2c_port *port, *tmp;
>
> -       list_for_each_entry(port, &i2c->ports, list) {
> +       list_for_each_entry_safe(port,tmp, &i2c->ports, list) {
> +               list_del(&port->list);
>                 i2c_del_adapter(&port->adapter);
>                 kfree(port);
>         }
>

Patch

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 694bbb4..695818f 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -17,7 +17,10 @@ 
 #include <linux/fsi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -128,6 +131,14 @@ 
 struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
+	struct list_head	ports;
+};
+
+struct fsi_i2c_port {
+	struct list_head	list;
+	struct i2c_adapter	adapter;
+	struct fsi_i2c_master	*master;
+	u16			port;
 };
 
 static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -181,9 +192,38 @@  static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
 	return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
 }
 
+static int fsi_i2c_set_port(struct fsi_i2c_port *port)
+{
+	int rc;
+	struct fsi_device *fsi = port->master->fsi;
+	u32 mode, dummy = 0;
+
+	rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	if (FIELD_GET(I2C_MODE_PORT, mode) == port->port)
+		return 0;
+
+	mode = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT, port->port);
+	rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	/* reset engine when port is changed */
+	return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
+}
+
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			int num)
 {
+	int rc;
+	struct fsi_i2c_port *port = adap->algo_data;
+
+	rc = fsi_i2c_set_port(port);
+	if (rc)
+		return rc;
+
 	return -EOPNOTSUPP;
 }
 
@@ -202,23 +242,72 @@  static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
 static int fsi_i2c_probe(struct device *dev)
 {
 	struct fsi_i2c_master *i2c;
+	struct fsi_i2c_port *port;
+	struct device_node *np;
 	int rc;
+	u32 port_no;
 
 	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
 	i2c->fsi = to_fsi_dev(dev);
+	INIT_LIST_HEAD(&i2c->ports);
 
 	rc = fsi_i2c_dev_init(i2c);
 	if (rc)
 		return rc;
 
+	/* Add adapter for each i2c port of the master. */
+	for_each_available_child_of_node(dev->of_node, np) {
+		rc = of_property_read_u32(np, "reg", &port_no);
+		if (rc || port_no > USHRT_MAX)
+			continue;
+
+		port = kzalloc(sizeof(*port), GFP_KERNEL);
+		if (!port)
+			break;
+
+		port->master = i2c;
+		port->port = port_no;
+
+		port->adapter.owner = THIS_MODULE;
+		port->adapter.dev.of_node = np;
+		port->adapter.dev.parent = dev;
+		port->adapter.algo = &fsi_i2c_algorithm;
+		port->adapter.algo_data = port;
+
+		snprintf(port->adapter.name, sizeof(port->adapter.name),
+			 "i2c_bus-%u", port_no);
+
+		rc = i2c_add_adapter(&port->adapter);
+		if (rc < 0) {
+			dev_err(dev, "Failed to register adapter: %d\n", rc);
+			kfree(port);
+			continue;
+		}
+
+		list_add(&port->list, &i2c->ports);
+	}
+
 	dev_set_drvdata(dev, i2c);
 
 	return 0;
 }
 
+static int fsi_i2c_remove(struct device *dev)
+{
+	struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
+	struct fsi_i2c_port *port;
+
+	list_for_each_entry(port, &i2c->ports, list) {
+		i2c_del_adapter(&port->adapter);
+		kfree(port);
+	}
+
+	return 0;
+}
+
 static const struct fsi_device_id fsi_i2c_ids[] = {
 	{ FSI_ENGID_I2C, FSI_VERSION_ANY },
 	{ }
@@ -230,6 +319,7 @@  static int fsi_i2c_probe(struct device *dev)
 		.name = "i2c-fsi",
 		.bus = &fsi_bus_type,
 		.probe = fsi_i2c_probe,
+		.remove = fsi_i2c_remove,
 	},
 };