[PATCH linux dev-4.10 v2 6/9] drivers: fsi: occ: Add cancel to remove() and fix probe()

Eddie James eajames at linux.vnet.ibm.com
Fri Oct 6 03:02:05 AEDT 2017



On 10/04/2017 08:07 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames at us.ibm.com>
>>   
>> Need some data to indicate to clients and the rest of the driver when
>> the device is being removed, so add a cancel boolean. Fix up both the
>> probe and remove functions to properly handle failures and prevent
>> deadlocks.
>>   
>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 62 insertions(+), 24 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index a554550..550ae11 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -46,6 +46,7 @@ struct occ {
>>   	spinlock_t list_lock;		/* lock access to the xfrs list */
>>   	struct mutex occ_lock;		/* lock access to the hardware */
>>   	struct work_struct work;
>> +	bool cancel;
>>   };
>>   
>>   #define to_occ(x)	container_of((x), struct occ, mdev)
>> @@ -117,12 +118,15 @@ struct occ_client {
>>   
>>   static DEFINE_IDA(occ_ida);
>>   
>> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
>> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>>   {
>>   	int empty;
>>   	struct occ_client *client = to_client(xfr);
>>   	struct occ *occ = client->occ;
>>   
>> +	if (occ->cancel)
>> +		return -ECANCELED;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	empty = list_empty(&occ->xfrs);
>> @@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>>   
>>   	if (empty)
>>   		queue_work(occ_wq, &occ->work);
>> +
>> +	return 0;
>>   }
>>   
>>   static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>>   {
>> -	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	struct occ_client *client;
>> +
>> +	if (occ->cancel)
>> +		return ERR_PTR(-ECANCELED);
>>   
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>>   	if (!client)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>   
>>   	client->occ = occ;
>>   	spin_lock_init(&client->lock);
>> @@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
>>   	struct occ *occ = to_occ(mdev);
>>   
>>   	client = occ_open_common(occ, file->f_flags);
>> -	if (!client)
>> -		return -ENOMEM;
>> +	if (IS_ERR(client))
>> +		return PTR_ERR(client);
>>   
>>   	file->private_data = client;
>>   
>> @@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   	int rc;
>>   	size_t bytes;
>>   	struct occ_xfr *xfr = &client->xfr;
>> +	struct occ *occ = client->occ;
>>   
>>   	if (len > OCC_SRAM_BYTES)
>>   		return -EINVAL;
>> @@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   					      test_bit(XFR_COMPLETE,
>>   						       &xfr->flags) ||
>>   					      test_bit(XFR_CANCELED,
>> -						       &xfr->flags));
>> +						       &xfr->flags) ||
>> +					      occ->cancel);
>>   
>>   		spin_lock_irq(&client->lock);
>>   
>> @@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   
>>   	spin_lock_irq(&client->lock);
>>   
>> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>>   		rc = -EBUSY;
>>   		goto done;
>>   	}
>> @@ -309,8 +321,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   	xfr->cmd_data_length = data_length + 6;
>>   	client->read_offset = 0;
>>   
>> -	occ_enqueue_xfr(xfr);
>> +	rc = occ_enqueue_xfr(xfr);
>> +	if (rc)
>> +		goto done;
>>   
>> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>>   	rc = len;
>>   
>>   done:
>> @@ -579,6 +594,9 @@ static void occ_worker(struct work_struct *work)
>>   	struct device *sbefifo = occ->sbefifo;
>>   
>>   again:
>> +	if (occ->cancel)
>> +		return;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
>> @@ -672,12 +690,17 @@ static void occ_worker(struct work_struct *work)
>>   
>>   struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>>   {
>> +	struct occ_client *client;
>>   	struct occ *occ = dev_get_drvdata(dev);
>>   
>>   	if (!occ)
>>   		return NULL;
>>   
>> -	return occ_open_common(occ, flags);
>> +	client = occ_open_common(occ, flags);
>> +	if (IS_ERR(client))
>> +		return NULL;
>> +
>> +	return client;
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_open);
>>   
>> @@ -748,23 +771,13 @@ static int occ_probe(struct platform_device *pdev)
>>   			if (occ->idx < 0)
>>   				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   							  GFP_KERNEL);
>> -		} else
>> +		} else {
>>   			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   						  GFP_KERNEL);
>> -
>> -		/* create platform devs for dts child nodes (hwmon, etc) */
>> -		for_each_child_of_node(dev->of_node, np) {
>> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> -				 occ->idx, child_idx++);
>> -			child = of_platform_device_create(np, child_name, dev);
>> -			if (!child)
>> -				dev_warn(dev,
>> -					 "failed to create child node dev\n");
>>   		}
>> -	} else
>> +	} else {
>>   		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
>> -
>> -	platform_set_drvdata(pdev, occ);
>> +	}
>>   
>>   	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>>   	occ->mdev.fops = &occ_fops;
>> @@ -774,20 +787,45 @@ static int occ_probe(struct platform_device *pdev)
>>   
>>   	rc = misc_register(&occ->mdev);
>>   	if (rc) {
>> -		dev_err(dev, "failed to register miscdevice\n");
>> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
>> +		ida_simple_remove(&occ_ida, occ->idx);
>>   		return rc;
>>   	}
>>   
>> +	/* create platform devs for dts child nodes (hwmon, etc) */
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> +			 occ->idx, child_idx++);
>> +		child = of_platform_device_create(np, child_name, dev);
>> +		if (!child)
>> +			dev_warn(dev, "failed to create child node dev\n");
>> +	}
>> +
>> +	platform_set_drvdata(pdev, occ);
>> +
>>   	return 0;
>>   }
>>   
>>   static int occ_remove(struct platform_device *pdev)
>>   {
>>   	struct occ *occ = platform_get_drvdata(pdev);
>> +	struct occ_xfr *xfr;
>> +	struct occ_client *client;
>> +
>> +	occ->cancel = true;
>> +
>> +	spin_lock_irq(&occ->list_lock);
>> +	list_for_each_entry(xfr, &occ->xfrs, link) {
>> +		client = to_client(xfr);
>> +		wake_up_interruptible(&client->wait);
> Again, would go for just wake_up() here, and maybe wake_up_all()?

Sure, though again, there are no non-interruptible wait_events in the 
driver. We should switch to _all() though, thanks.

Thanks,
Eddie

>
> Andrew
>
>> +	}
>> +	spin_unlock_irq(&occ->list_lock);
>>   
>> -	flush_work(&occ->work);
>>   	misc_deregister(&occ->mdev);
>>   	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
>> +
>> +	cancel_work_sync(&occ->work);
>> +
>>   	ida_simple_remove(&occ_ida, occ->idx);
>>   
>>   	return 0;



More information about the openbmc mailing list