* [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
@ 2017-10-06 14:29 Eddie James
2017-10-09 2:00 ` Joel Stanley
0 siblings, 1 reply; 5+ messages in thread
From: Eddie James @ 2017-10-06 14:29 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Prevent hanging forever waiting for OCC ops to complete.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Changes since v4:
* Add comments for the spinlock
---
drivers/hwmon/occ/p9_sbe.c | 49 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index c7e0d9c..ac18422 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -14,37 +14,68 @@
#include <linux/platform_device.h>
#include <linux/occ.h>
#include <linux/sched.h>
+#include <linux/spinlock.h>
#include <linux/workqueue.h>
struct p9_sbe_occ {
struct occ occ;
struct device *sbe;
+
+ /*
+ * Pointer to occ device client. We store this so that we can cancel
+ * the client operations in remove() if necessary. We only need one
+ * pointer since we do one OCC operation (open, write, read, close) at
+ * a time (access to p9_sbe_occ_send_cmd is locked in the common code
+ * with occ.lock).
+ */
+ struct occ_client *client;
+
+ /*
+ * This lock controls access to the client pointer and ensures atomic
+ * open, close and NULL assignment. This prevents simultaneous opening
+ * and closing of the client, or closing multiple times.
+ */
+ spinlock_t lock;
};
#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ)
+static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
+{
+ struct occ_client *tmp_client;
+
+ spin_lock_irq(&occ->lock);
+ tmp_client = occ->client;
+ occ->client = NULL;
+ occ_drv_release(tmp_client);
+ spin_unlock_irq(&occ->lock);
+}
+
static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
{
int rc, error;
- struct occ_client *client;
struct occ_response *resp = &occ->resp;
struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
- client = occ_drv_open(p9_sbe_occ->sbe, 0);
- if (!client) {
+ spin_lock_irq(&p9_sbe_occ->lock);
+ if (p9_sbe_occ->sbe)
+ p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
+ spin_unlock_irq(&p9_sbe_occ->lock);
+
+ if (!p9_sbe_occ->client) {
rc = -ENODEV;
goto assign;
}
- rc = occ_drv_write(client, (const char *)&cmd[1], 7);
+ rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
if (rc < 0)
goto err;
- rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+ rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
if (rc < 0)
goto err;
- occ_drv_release(client);
+ p9_sbe_occ_close_client(p9_sbe_occ);
switch (resp->return_status) {
case RESP_RETURN_CMD_IN_PRG:
@@ -72,7 +103,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
goto done;
err:
- occ_drv_release(client);
+ p9_sbe_occ_close_client(p9_sbe_occ);
dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc);
assign:
error = rc;
@@ -132,6 +163,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
p9_sbe_occ->sbe = pdev->dev.parent;
occ = &p9_sbe_occ->occ;
+ spin_lock_init(&p9_sbe_occ->lock);
occ->bus_dev = &pdev->dev;
occ->groups[0] = &occ->group;
occ->poll_cmd_data = 0x20;
@@ -152,7 +184,10 @@ 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->sbe = NULL;
+ p9_sbe_occ_close_client(p9_sbe_occ);
occ_remove_status_attrs(occ);
atomic_dec(&occ_num_occs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
2017-10-06 14:29 [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove() Eddie James
@ 2017-10-09 2:00 ` Joel Stanley
2017-10-09 14:33 ` Eddie James
0 siblings, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2017-10-09 2:00 UTC (permalink / raw)
To: Eddie James; +Cc: OpenBMC Maillist, Andrew Jeffery, Edward A. James
On Fri, Oct 6, 2017 at 11:59 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Prevent hanging forever waiting for OCC ops to complete.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>
> Changes since v4:
> * Add comments for the spinlock
> ---
> drivers/hwmon/occ/p9_sbe.c | 49 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index c7e0d9c..ac18422 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -14,37 +14,68 @@
> #include <linux/platform_device.h>
> #include <linux/occ.h>
> #include <linux/sched.h>
> +#include <linux/spinlock.h>
> #include <linux/workqueue.h>
>
> struct p9_sbe_occ {
> struct occ occ;
> struct device *sbe;
> +
> + /*
> + * Pointer to occ device client. We store this so that we can cancel
> + * the client operations in remove() if necessary. We only need one
> + * pointer since we do one OCC operation (open, write, read, close) at
> + * a time (access to p9_sbe_occ_send_cmd is locked in the common code
> + * with occ.lock).
> + */
> + struct occ_client *client;
> +
> + /*
> + * This lock controls access to the client pointer and ensures atomic
> + * open, close and NULL assignment. This prevents simultaneous opening
> + * and closing of the client, or closing multiple times.
> + */
> + spinlock_t lock;
> };
>
> #define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ)
>
> +static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
> +{
> + struct occ_client *tmp_client;
> +
> + spin_lock_irq(&occ->lock);
Why not spin_lock_irqsave/spin_unlock_irqsave? Same throughout this patch.
Cheers,
Joel
> + tmp_client = occ->client;
> + occ->client = NULL;
> + occ_drv_release(tmp_client);
> + spin_unlock_irq(&occ->lock);
> +}
> +
> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> {
> int rc, error;
> - struct occ_client *client;
> struct occ_response *resp = &occ->resp;
> struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
>
> - client = occ_drv_open(p9_sbe_occ->sbe, 0);
> - if (!client) {
> + spin_lock_irq(&p9_sbe_occ->lock);
> + if (p9_sbe_occ->sbe)
> + p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
> + spin_unlock_irq(&p9_sbe_occ->lock);
> +
> + if (!p9_sbe_occ->client) {
> rc = -ENODEV;
> goto assign;
> }
>
> - rc = occ_drv_write(client, (const char *)&cmd[1], 7);
> + rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
> if (rc < 0)
> goto err;
>
> - rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
> + rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
> if (rc < 0)
> goto err;
>
> - occ_drv_release(client);
> + p9_sbe_occ_close_client(p9_sbe_occ);
>
> switch (resp->return_status) {
> case RESP_RETURN_CMD_IN_PRG:
> @@ -72,7 +103,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> goto done;
>
> err:
> - occ_drv_release(client);
> + p9_sbe_occ_close_client(p9_sbe_occ);
> dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc);
> assign:
> error = rc;
> @@ -132,6 +163,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
> p9_sbe_occ->sbe = pdev->dev.parent;
>
> occ = &p9_sbe_occ->occ;
> + spin_lock_init(&p9_sbe_occ->lock);
> occ->bus_dev = &pdev->dev;
> occ->groups[0] = &occ->group;
> occ->poll_cmd_data = 0x20;
> @@ -152,7 +184,10 @@ 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->sbe = NULL;
> + p9_sbe_occ_close_client(p9_sbe_occ);
> occ_remove_status_attrs(occ);
>
> atomic_dec(&occ_num_occs);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
2017-10-09 2:00 ` Joel Stanley
@ 2017-10-09 14:33 ` Eddie James
2017-10-10 2:57 ` Joel Stanley
0 siblings, 1 reply; 5+ messages in thread
From: Eddie James @ 2017-10-09 14:33 UTC (permalink / raw)
To: Joel Stanley; +Cc: Andrew Jeffery, Edward A. James, OpenBMC Maillist
On 10/08/2017 09:00 PM, Joel Stanley wrote:
> On Fri, Oct 6, 2017 at 11:59 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Prevent hanging forever waiting for OCC ops to complete.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>>
>> Changes since v4:
>> * Add comments for the spinlock
>> ---
>> drivers/hwmon/occ/p9_sbe.c | 49 +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 42 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
>> index c7e0d9c..ac18422 100644
>> --- a/drivers/hwmon/occ/p9_sbe.c
>> +++ b/drivers/hwmon/occ/p9_sbe.c
>> @@ -14,37 +14,68 @@
>> #include <linux/platform_device.h>
>> #include <linux/occ.h>
>> #include <linux/sched.h>
>> +#include <linux/spinlock.h>
>> #include <linux/workqueue.h>
>>
>> struct p9_sbe_occ {
>> struct occ occ;
>> struct device *sbe;
>> +
>> + /*
>> + * Pointer to occ device client. We store this so that we can cancel
>> + * the client operations in remove() if necessary. We only need one
>> + * pointer since we do one OCC operation (open, write, read, close) at
>> + * a time (access to p9_sbe_occ_send_cmd is locked in the common code
>> + * with occ.lock).
>> + */
>> + struct occ_client *client;
>> +
>> + /*
>> + * This lock controls access to the client pointer and ensures atomic
>> + * open, close and NULL assignment. This prevents simultaneous opening
>> + * and closing of the client, or closing multiple times.
>> + */
>> + spinlock_t lock;
>> };
>>
>> #define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ)
>>
>> +static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
>> +{
>> + struct occ_client *tmp_client;
>> +
>> + spin_lock_irq(&occ->lock);
> Why not spin_lock_irqsave/spin_unlock_irqsave? Same throughout this patch.
Well spin_lock_irq/spin_unlock_irq is used through the occ and sbefifo
drivers. Just following suit here. I can switch it if necessary.
Thanks,
Eddie
>
> Cheers,
>
> Joel
>
>> + tmp_client = occ->client;
>> + occ->client = NULL;
>> + occ_drv_release(tmp_client);
>> + spin_unlock_irq(&occ->lock);
>> +}
>> +
>> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>> {
>> int rc, error;
>> - struct occ_client *client;
>> struct occ_response *resp = &occ->resp;
>> struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
>>
>> - client = occ_drv_open(p9_sbe_occ->sbe, 0);
>> - if (!client) {
>> + spin_lock_irq(&p9_sbe_occ->lock);
>> + if (p9_sbe_occ->sbe)
>> + p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
>> + spin_unlock_irq(&p9_sbe_occ->lock);
>> +
>> + if (!p9_sbe_occ->client) {
>> rc = -ENODEV;
>> goto assign;
>> }
>>
>> - rc = occ_drv_write(client, (const char *)&cmd[1], 7);
>> + rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
>> if (rc < 0)
>> goto err;
>>
>> - rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
>> + rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
>> if (rc < 0)
>> goto err;
>>
>> - occ_drv_release(client);
>> + p9_sbe_occ_close_client(p9_sbe_occ);
>>
>> switch (resp->return_status) {
>> case RESP_RETURN_CMD_IN_PRG:
>> @@ -72,7 +103,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>> goto done;
>>
>> err:
>> - occ_drv_release(client);
>> + p9_sbe_occ_close_client(p9_sbe_occ);
>> dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc);
>> assign:
>> error = rc;
>> @@ -132,6 +163,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>> p9_sbe_occ->sbe = pdev->dev.parent;
>>
>> occ = &p9_sbe_occ->occ;
>> + spin_lock_init(&p9_sbe_occ->lock);
>> occ->bus_dev = &pdev->dev;
>> occ->groups[0] = &occ->group;
>> occ->poll_cmd_data = 0x20;
>> @@ -152,7 +184,10 @@ 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->sbe = NULL;
>> + p9_sbe_occ_close_client(p9_sbe_occ);
>> occ_remove_status_attrs(occ);
>>
>> atomic_dec(&occ_num_occs);
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
2017-10-09 14:33 ` Eddie James
@ 2017-10-10 2:57 ` Joel Stanley
2017-10-16 18:34 ` Brad Bishop
0 siblings, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2017-10-10 2:57 UTC (permalink / raw)
To: Eddie James; +Cc: Andrew Jeffery, Edward A. James, OpenBMC Maillist
2017-10-10 0:03 GMT+09:30 Eddie James <eajames@linux.vnet.ibm.com>:
>
>
> On 10/08/2017 09:00 PM, Joel Stanley wrote:
>>> +static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
>>> +{
>>> + struct occ_client *tmp_client;
>>> +
>>> + spin_lock_irq(&occ->lock);
>>
>> Why not spin_lock_irqsave/spin_unlock_irqsave? Same throughout this patch.
>
>
> Well spin_lock_irq/spin_unlock_irq is used through the occ and sbefifo
> drivers. Just following suit here. I can switch it if necessary.
If you understand the differences and used the non-save/restore
versions intentionally, then we can keep the driver as it is.
In general, to be safe, more experienced kernel hackers than myself
recommend the irqsave/irqrestore versions of the spin lock functions
as they are safe to call from any context (IRQ or otherwise).
Cheers,
Joel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
2017-10-10 2:57 ` Joel Stanley
@ 2017-10-16 18:34 ` Brad Bishop
0 siblings, 0 replies; 5+ messages in thread
From: Brad Bishop @ 2017-10-16 18:34 UTC (permalink / raw)
To: Joel Stanley
Cc: Eddie James, Andrew Jeffery, Edward A. James, OpenBMC Maillist
>>>
>>> Why not spin_lock_irqsave/spin_unlock_irqsave? Same throughout this patch.
>>
>>
>> Well spin_lock_irq/spin_unlock_irq is used through the occ and sbefifo
>> drivers. Just following suit here. I can switch it if necessary.
>
> If you understand the differences and used the non-save/restore
> versions intentionally, then we can keep the driver as it is.
This is my fault. The initial sbefifo driver introduced this. I don’t remember
what my mode of thinking was. Regardless, it sounds like it was wrong.
>
> In general, to be safe, more experienced kernel hackers than myself
> recommend the irqsave/irqrestore versions of the spin lock functions
> as they are safe to call from any context (IRQ or otherwise).
>
> Cheers,
>
> Joel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-16 18:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-06 14:29 [PATCH linux dev-4.10 v5 31/31] drivers: hwmon: occ: Cancel occ operations in remove() Eddie James
2017-10-09 2:00 ` Joel Stanley
2017-10-09 14:33 ` Eddie James
2017-10-10 2:57 ` Joel Stanley
2017-10-16 18:34 ` Brad Bishop
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.