drivers/hwmon/occ: Add reference count and switch to non-blocking read

From: Eddie James <[email protected]>

It's possible for the sbefifo ops to hang entirely if the system goes down (checkstop, etc). If a hwmon sysfs operation is hung, then unbind may hang as it waits for the kernfs mutex to remove hwmon attributes.

This change switches the p9 occ op to use a non-blocking read with a retry on -EAGAIN. This continues forever until either user cancels or driver sets the cancel bit in remove(). In addition, we need to switch our occ structure to be dynamically allocated separate from the device, as the device resources will be freed immediately after remove(), while hwmon ops may still be ocurring. This necessitates reference counting so we can free the occ structure once all users are done.

This patch depends on "kobject: Export kobject_get_unless_zero()".

Signed-off-by Eddie James <[email protected]>

---
  drivers/hwmon/occ/common.c | 42 +++++++++++++++++++++++++++++++++---------
  drivers/hwmon/occ/common.h |  2 ++
  drivers/hwmon/occ/p8_i2c.c | 23 ++++++++++++++++++++---
  drivers/hwmon/occ/p9_sbe.c | 37 ++++++++++++++++++++++++++++++++-----
  4 files changed, 87 insertions(+), 17 deletions(-)

drivers/hwmon/occ/common.c +33 -9

index 34002fb..747c6e1 100644
@@ -216,7 +216,8 @@ int occ_poll(struct occ *occ)
 
  done:
  	/* notify userspace if we change error state and have an error */
-	if (occ->error != error && occ->error && occ->error_attr_name)
+	if (occ->error != error && occ->error && occ->error_attr_name &&
+	    !occ->cancel)
  		sysfs_notify(&occ->bus_dev->kobj, NULL, occ->error_attr_name);
 
  	return rc;

@@ -224,19 +225,28 @@ int occ_poll(struct occ *occ)
 
  int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
  {
-	int rc, error = occ->error;
+	int rc, error;
  	u8 cmd[8];
  	u16 checksum = 0x24;
  	__be16 user_power_cap_be;
-	struct occ_poll_response_header *header =
-		(struct occ_poll_response_header *)occ->resp.data;
+	struct occ_poll_response_header *header;
+
+	if (!kobject_get_unless_zero(&occ->kobj))
+		return -ENODEV;
 
-	if (!(header->status & OCC_STAT_MASTER))
-		return -EPERM;
+	header = (struct occ_poll_response_header *)occ->resp.data;
+
+	if (!(header->status & OCC_STAT_MASTER)) {
+		rc = -EPERM;
+		goto done;
+	}
 
-	if (!(header->status & OCC_STAT_ACTIVE))
-		return -EACCES;
+	if (!(header->status & OCC_STAT_ACTIVE)) {
+		rc = -EACCES;
+		goto done;
+	}
 
+	error = occ->error;
  	user_power_cap_be = cpu_to_be16(user_power_cap);
 
  	cmd[0] = 0;

@@ -255,9 +265,12 @@ int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
  	mutex_unlock(&occ->lock);
 
  	/* notify userspace if we change error state and have an error*/
-	if (occ->error != error && occ->error && occ->error_attr_name)
+	if (occ->error != error && occ->error && occ->error_attr_name &&
+	    !occ->cancel)
  		sysfs_notify(&occ->bus_dev->kobj, NULL, occ->error_attr_name);
 
+done:
+	kobject_put(&occ->kobj);
  	return rc;
  }
 

@@ -265,6 +278,9 @@ int occ_update_response(struct occ *occ)
  {
  	int rc = 0;
 
+	if (!kobject_get_unless_zero(&occ->kobj))
+		return -ENODEV;
+
  	mutex_lock(&occ->lock);
 
  	if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {

@@ -272,7 +288,15 @@ int occ_update_response(struct occ *occ)
  		occ->last_update = jiffies;
  	}
 
+	/*
+	 * Make sure we fail if canceled so we don't access occ structure in
+	 * subsequent functions.
+	 */
+	if (occ->cancel)
+		rc = -ECANCELED;
+
  	mutex_unlock(&occ->lock);
+	kobject_put(&occ->kobj);
  	return rc;
  }
 

drivers/hwmon/occ/common.h +2 -0

index acb50bc..242d90c 100644
@@ -98,9 +98,11 @@ struct occ_attribute {
  };
 
  struct occ {
+	struct kobject kobj;
  	struct device *bus_dev;
  	struct device *hwmon;
 
+	bool cancel;
  	int error;
  	unsigned int error_count;
  	unsigned long last_safe;

drivers/hwmon/occ/p8_i2c.c +20 -3

index e1c6234..6856b2f 100644
@@ -13,6 +13,7 @@
  #include <linux/i2c.h>
  #include <linux/init.h>
  #include <linux/module.h>
+#include <linux/slab.h>
 
  struct p8_i2c_occ {
  	struct occ occ;

@@ -21,6 +22,18 @@ struct p8_i2c_occ {
 
  #define to_p8_i2c_occ(x)	container_of((x), struct p8_i2c_occ, occ)
 
+static void p8_i2c_occ_release(struct kobject *kobj)
+{
+	struct occ *occ = container_of(kobj, struct occ, kobj);
+	struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
+
+	kfree(p8_i2c_occ);
+}
+
+static struct kobj_type p8_i2c_occ_type = {
+	.release = p8_i2c_occ_release,
+};
+
  static int p8_i2c_occ_getscom(struct i2c_client *client, u32 address, u8 *data)
  {
  	ssize_t rc;

@@ -198,15 +211,15 @@ static int p8_i2c_occ_probe(struct i2c_client *client,
  {
  	int rc;
  	struct occ *occ;
-	struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev,
-						     sizeof(*p8_i2c_occ),
-						     GFP_KERNEL);
+	struct p8_i2c_occ *p8_i2c_occ = kzalloc(sizeof(*p8_i2c_occ),
+						GFP_KERNEL);
  	if (!p8_i2c_occ)
  		return -ENOMEM;
 
  	p8_i2c_occ->client = client;
 
  	occ = &p8_i2c_occ->occ;
+	kobject_init(&occ->kobj, &p8_i2c_occ_type);
  	occ->bus_dev = &client->dev;
  	occ->groups[0] = &occ->group;
  	occ->poll_cmd_data = 0x10;

@@ -257,10 +270,14 @@ static int p8_i2c_occ_remove(struct i2c_client *client)
  {
  	struct occ *occ = dev_get_drvdata(&client->dev);
 
+	occ->cancel = true;
+
  	occ_remove_status_attrs(occ);
 
  	atomic_dec(&occ_num_occs);
 
+	kobject_put(&occ->kobj);
+
  	return 0;
  }
 

drivers/hwmon/occ/p9_sbe.c +32 -5

index 2d50a94..99a6415 100644
@@ -14,8 +14,11 @@
  #include <linux/platform_device.h>
  #include <linux/occ.h>
  #include <linux/sched.h>
+#include <linux/slab.h>
  #include <linux/workqueue.h>
 
+#define OCC_CMD_INCOMPLETE_MS		10
+
  struct p9_sbe_occ {
  	struct occ occ;
  	struct device *sbe;

@@ -23,6 +26,18 @@ struct p9_sbe_occ {
 
  #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
+static void p9_sbe_occ_release(struct kobject *kobj)
+{
+	struct occ *occ = container_of(kobj, struct occ, kobj);
+	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+
+	kfree(p9_sbe_occ);
+}
+
+static struct kobj_type p9_sbe_occ_type = {
+	.release = p9_sbe_occ_release,
+};
+
  static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  {
  	int rc, error;

@@ -34,7 +49,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  	start = jiffies;
 
  retry:
-	client = occ_drv_open(p9_sbe_occ->sbe, 0);
+	client = occ_drv_open(p9_sbe_occ->sbe, O_NONBLOCK);
  	if (!client) {
  		rc = -ENODEV;
  		goto assign;

@@ -44,7 +59,15 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  	if (rc < 0)
  		goto err;
 
-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+	do {
+		rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+		if (rc != -EAGAIN)
+			break;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(msecs_to_jiffies(OCC_CMD_INCOMPLETE_MS));
+	} while (!occ->cancel);
+
  	if (rc < 0)
  		goto err;
 

@@ -135,15 +158,15 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
  {
  	int rc;
  	struct occ *occ;
-	struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev,
-						     sizeof(*p9_sbe_occ),
-						     GFP_KERNEL);
+	struct p9_sbe_occ *p9_sbe_occ = kzalloc(sizeof(*p9_sbe_occ),
+						GFP_KERNEL);
  	if (!p9_sbe_occ)
  		return -ENOMEM;
 
  	p9_sbe_occ->sbe = pdev->dev.parent;
 
  	occ = &p9_sbe_occ->occ;
+	kobject_init(&occ->kobj, &p9_sbe_occ_type);
  	occ->bus_dev = &pdev->dev;
  	occ->groups[0] = &occ->group;
  	occ->poll_cmd_data = 0x20;

@@ -165,10 +188,14 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
  {
  	struct occ *occ = platform_get_drvdata(pdev);
 
+	occ->cancel = true;
+
  	occ_remove_status_attrs(occ);
 
  	atomic_dec(&occ_num_occs);
 
+	kobject_put(&occ->kobj);
+
  	return 0;
  }
 

From: Eddie James <[email protected]>

It's possible for the sbefifo ops to hang entirely if the system goes down (checkstop, etc). If a hwmon sysfs operation is hung, then unbind may hang as it waits for the kernfs mutex to remove hwmon attributes.

This change switches the p9 occ op to use a non-blocking read with a retry on -EAGAIN. This continues forever until either user cancels or driver sets the cancel bit in remove(). In addition, we need to switch our occ structure to be dynamically allocated separate from the device, as the device resources will be freed immediately after remove(), while hwmon ops may still be ocurring. This necessitates reference counting so we can free the occ structure once all users are done.

This patch depends on "kobject: Export kobject_get_unless_zero()".

Signed-off-by Eddie James <[email protected]>

---
  drivers/hwmon/occ/common.c | 42 +++++++++++++++++++++++++++++++++---------
  drivers/hwmon/occ/common.h |  2 ++
  drivers/hwmon/occ/p8_i2c.c | 23 ++++++++++++++++++++---
  drivers/hwmon/occ/p9_sbe.c | 37 ++++++++++++++++++++++++++++++++-----
  4 files changed, 87 insertions(+), 17 deletions(-)

drivers/hwmon/occ/common.c +33 -9

index 34002fb..747c6e1 100644
@@ -216,7 +216,8 @@ int occ_poll(struct occ *occ)
 
  done:
  	/* notify userspace if we change error state and have an error */
-	if (occ->error != error && occ->error && occ->error_attr_name)
+	if (occ->error != error && occ->error && occ->error_attr_name &&
+	    !occ->cancel)
  		sysfs_notify(&occ->bus_dev->kobj, NULL, occ->error_attr_name);
 
  	return rc;

@@ -224,19 +225,28 @@ int occ_poll(struct occ *occ)
 
  int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
  {
-	int rc, error = occ->error;
+	int rc, error;
  	u8 cmd[8];
  	u16 checksum = 0x24;
  	__be16 user_power_cap_be;
-	struct occ_poll_response_header *header =
-		(struct occ_poll_response_header *)occ->resp.data;
+	struct occ_poll_response_header *header;
+
+	if (!kobject_get_unless_zero(&occ->kobj))
+		return -ENODEV;
 
-	if (!(header->status & OCC_STAT_MASTER))
-		return -EPERM;
+	header = (struct occ_poll_response_header *)occ->resp.data;
+
+	if (!(header->status & OCC_STAT_MASTER)) {
+		rc = -EPERM;
+		goto done;
+	}
 
-	if (!(header->status & OCC_STAT_ACTIVE))
-		return -EACCES;
+	if (!(header->status & OCC_STAT_ACTIVE)) {
+		rc = -EACCES;
+		goto done;
+	}
 
+	error = occ->error;
  	user_power_cap_be = cpu_to_be16(user_power_cap);
 
  	cmd[0] = 0;

@@ -255,9 +265,12 @@ int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
  	mutex_unlock(&occ->lock);
 
  	/* notify userspace if we change error state and have an error*/
-	if (occ->error != error && occ->error && occ->error_attr_name)
+	if (occ->error != error && occ->error && occ->error_attr_name &&
+	    !occ->cancel)
  		sysfs_notify(&occ->bus_dev->kobj, NULL, occ->error_attr_name);
 
+done:
+	kobject_put(&occ->kobj);
  	return rc;
  }
 

@@ -265,6 +278,9 @@ int occ_update_response(struct occ *occ)
  {
  	int rc = 0;
 
+	if (!kobject_get_unless_zero(&occ->kobj))
+		return -ENODEV;
+
  	mutex_lock(&occ->lock);
 
  	if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {

@@ -272,7 +288,15 @@ int occ_update_response(struct occ *occ)
  		occ->last_update = jiffies;
  	}
 
+	/*
+	 * Make sure we fail if canceled so we don't access occ structure in
+	 * subsequent functions.
+	 */
+	if (occ->cancel)
+		rc = -ECANCELED;
+
  	mutex_unlock(&occ->lock);
+	kobject_put(&occ->kobj);
  	return rc;
  }
 

drivers/hwmon/occ/common.h +2 -0

index acb50bc..242d90c 100644
@@ -98,9 +98,11 @@ struct occ_attribute {
  };
 
  struct occ {
+	struct kobject kobj;
  	struct device *bus_dev;
  	struct device *hwmon;
 
+	bool cancel;
  	int error;
  	unsigned int error_count;
  	unsigned long last_safe;

drivers/hwmon/occ/p8_i2c.c +20 -3

index e1c6234..6856b2f 100644
@@ -13,6 +13,7 @@
  #include <linux/i2c.h>
  #include <linux/init.h>
  #include <linux/module.h>
+#include <linux/slab.h>
 
  struct p8_i2c_occ {
  	struct occ occ;

@@ -21,6 +22,18 @@ struct p8_i2c_occ {
 
  #define to_p8_i2c_occ(x)	container_of((x), struct p8_i2c_occ, occ)
 
+static void p8_i2c_occ_release(struct kobject *kobj)
+{
+	struct occ *occ = container_of(kobj, struct occ, kobj);
+	struct p8_i2c_occ *p8_i2c_occ = to_p8_i2c_occ(occ);
+
+	kfree(p8_i2c_occ);
+}
+
+static struct kobj_type p8_i2c_occ_type = {
+	.release = p8_i2c_occ_release,
+};
+
  static int p8_i2c_occ_getscom(struct i2c_client *client, u32 address, u8 *data)
  {
  	ssize_t rc;

@@ -198,15 +211,15 @@ static int p8_i2c_occ_probe(struct i2c_client *client,
  {
  	int rc;
  	struct occ *occ;
-	struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev,
-						     sizeof(*p8_i2c_occ),
-						     GFP_KERNEL);
+	struct p8_i2c_occ *p8_i2c_occ = kzalloc(sizeof(*p8_i2c_occ),
+						GFP_KERNEL);
  	if (!p8_i2c_occ)
  		return -ENOMEM;
 
  	p8_i2c_occ->client = client;
 
  	occ = &p8_i2c_occ->occ;
+	kobject_init(&occ->kobj, &p8_i2c_occ_type);
  	occ->bus_dev = &client->dev;
  	occ->groups[0] = &occ->group;
  	occ->poll_cmd_data = 0x10;

@@ -257,10 +270,14 @@ static int p8_i2c_occ_remove(struct i2c_client *client)
  {
  	struct occ *occ = dev_get_drvdata(&client->dev);
 
+	occ->cancel = true;
+
  	occ_remove_status_attrs(occ);
 
  	atomic_dec(&occ_num_occs);
 
+	kobject_put(&occ->kobj);
+
  	return 0;
  }
 

drivers/hwmon/occ/p9_sbe.c +32 -5

index 2d50a94..99a6415 100644
@@ -14,8 +14,11 @@
  #include <linux/platform_device.h>
  #include <linux/occ.h>
  #include <linux/sched.h>
+#include <linux/slab.h>
  #include <linux/workqueue.h>
 
+#define OCC_CMD_INCOMPLETE_MS		10
+
  struct p9_sbe_occ {
  	struct occ occ;
  	struct device *sbe;

@@ -23,6 +26,18 @@ struct p9_sbe_occ {
 
  #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
+static void p9_sbe_occ_release(struct kobject *kobj)
+{
+	struct occ *occ = container_of(kobj, struct occ, kobj);
+	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+
+	kfree(p9_sbe_occ);
+}
+
+static struct kobj_type p9_sbe_occ_type = {
+	.release = p9_sbe_occ_release,
+};
+
  static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  {
  	int rc, error;

@@ -34,7 +49,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  	start = jiffies;
 
  retry:
-	client = occ_drv_open(p9_sbe_occ->sbe, 0);
+	client = occ_drv_open(p9_sbe_occ->sbe, O_NONBLOCK);
  	if (!client) {
  		rc = -ENODEV;
  		goto assign;

@@ -44,7 +59,15 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
  	if (rc < 0)
  		goto err;
 
-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+	do {
+		rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+		if (rc != -EAGAIN)
+			break;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(msecs_to_jiffies(OCC_CMD_INCOMPLETE_MS));
+	} while (!occ->cancel);
+
  	if (rc < 0)
  		goto err;
 

@@ -135,15 +158,15 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
  {
  	int rc;
  	struct occ *occ;
-	struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev,
-						     sizeof(*p9_sbe_occ),
-						     GFP_KERNEL);
+	struct p9_sbe_occ *p9_sbe_occ = kzalloc(sizeof(*p9_sbe_occ),
+						GFP_KERNEL);
  	if (!p9_sbe_occ)
  		return -ENOMEM;
 
  	p9_sbe_occ->sbe = pdev->dev.parent;
 
  	occ = &p9_sbe_occ->occ;
+	kobject_init(&occ->kobj, &p9_sbe_occ_type);
  	occ->bus_dev = &pdev->dev;
  	occ->groups[0] = &occ->group;
  	occ->poll_cmd_data = 0x20;

@@ -165,10 +188,14 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
  {
  	struct occ *occ = platform_get_drvdata(pdev);
 
+	occ->cancel = true;
+
  	occ_remove_status_attrs(occ);
 
  	atomic_dec(&occ_num_occs);
 
+	kobject_put(&occ->kobj);
+
  	return 0;
  }