Re: [PATCH linux dev-4.10 v4 31/31] drivers: hwmon: occ: Cancel occ operations in remove()

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3648 bytes --]

On Thu, 2017-10-05 at 21:05 -0500, Eddie James 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>
> ---
>  drivers/hwmon/occ/p9_sbe.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index c7e0d9c..9842e0d 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -14,37 +14,54 @@
>  #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;
> +	struct occ_client *client;
> +	spinlock_t lock;

Can you please explain the semantics of this lock in a comment or in
the commit message? Who should hold it, when and why, and in what order
with respect to other locks if applicable?

We've discussed it in private previously but I admit the details have
escaped me, and the way it gets used in this patch looks pretty
strange.

Andrew


>  };
>  
>  #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 +89,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 +149,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 +170,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);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]