drivers/hwmon/occ: Switch to non-blocking for occ reads

PATCH In review Eddie James 2 participants 3 messages Sep 18, 2017 – Sep 19, 2017 Hardware monitoring

lore

Share & tools

lore

Eddie James

author

2017-09-18 21:12 UTC #0 lore raw

PATCH 1/1 drivers/hwmon/occ: Switch to non-blocking for occ reads

From: Eddie James <[email protected]>

It's possible for the sbefifo ops to hang entirely if the system goes down (checkstop, etc), meaning that the occ and occ-hwmon drivers may hang during remove().

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

---
 drivers/hwmon/occ/p9_sbe.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

drivers/hwmon/occ/p9_sbe.c +15 -4

index 2d50a94..a6aa4d2 100644
@@ -16,7 +16,10 @@
 #include <linux/sched.h>
 #include <linux/workqueue.h>
 
+#define OCC_CMD_INCOMPLETE_MS		10
+
 struct p9_sbe_occ {
+	bool cancel;
 	struct occ occ;
 	struct device *sbe;
 };

@@ -34,7 +37,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,9 +47,14 @@ 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));
-	if (rc < 0)
-		goto err;
+	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 (!p9_sbe_occ->cancel);
 
 	occ_drv_release(client);

@@ -164,6 +172,9 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 static int p9_sbe_occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+
+	p9_sbe_occ->cancel = true;
 
 	occ_remove_status_attrs(occ);

Signature · 1 line

1.8.3.1

Jeremy Kerr

2017-09-19 07:13 UTC #1 lore raw

Hi Eddie,

Quoted 3 lines

It's possible for the sbefifo ops to hang entirely if the system goes down (checkstop, etc), meaning that the occ and occ-hwmon drivers may hang during remove().

How does this address that though? Your comment implies that a thread is stuck in occ_drv_read, is that correct?

I see that you're now opening in non-blocking mode, but:

drivers/hwmon/occ/p9_sbe.c static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) · line 57

Quoted 11 lines

drivers/hwmon/occ/p9_sbe.c +8 -3

-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
-	if (rc < 0)
-		goto err;
+	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 (!p9_sbe_occ->cancel);

- the only way that you hit the !p9_sbe_occ->cancel condition is if we've run the ->remove callback, in which case we *definitely* shouldn't have threads still referencing the device. There seems to be nothing preventing p9_sbe_occ from being free()ed underneath you.

If this patch works, it suggests that there's either a reference-counting issue, or a memory leak.

Regards,

Jeremy

Eddie James

author

2017-09-19 14:56 UTC #2 lore raw

Quoted 7 lines · Jeremy Kerr

Hi Eddie,
Quoted 3 lines
It's possible for the sbefifo ops to hang entirely if the system goes down (checkstop, etc), meaning that the occ and occ-hwmon drivers may hang during remove().
How does this address that though? Your comment implies that a thread is stuck in occ_drv_read, is that correct?

Correct.

Quoted 17 lines

I see that you're now opening in non-blocking mode, but:
drivers/hwmon/occ/p9_sbe.c static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) · line 57
Quoted 11 lines
drivers/hwmon/occ/p9_sbe.c +8 -3
-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
-	if (rc < 0)
-		goto err;
+	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 (!p9_sbe_occ->cancel);
- the only way that you hit the !p9_sbe_occ->cancel condition is if we've run the ->remove callback, in which case we *definitely* shouldn't have threads still referencing the device.

Hmm. In this error case, remove() is called from the sysfs unbind pathway. I could be wrong, but I don't see anything in the kernel to block in or return from unbind if there are hwmon files open. So I think it's possible to get to remove() with threads hanging around.

Quoted 2 lines

There seems to be nothing
preventing p9_sbe_occ from being free()ed underneath you.

That is true... Most likely we get lucky due to trying to remove hwmon files before freeing p9_sbe_occ... I'll have to fix this.

Thanks for the review!
Eddie

Quoted 9 lines

If this patch works, it suggests that there's either a reference-counting issue, or a memory leak.
Regards,
Jeremy