[PATCH linux dev-4.10 v3 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()
Eddie James
eajames at linux.vnet.ibm.com
Fri Oct 6 12:20:55 AEDT 2017
On 10/05/2017 06:36 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:24 -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 8555e99..1770823 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);
> I don't think this test is helpful, in that it's racy. What if remove() gets
> called immediately after the thread calling into open() has passed the test?
Yep true. This was helpful before I fixed the memory management of the
client, though obviously still racy. No need for it now.
Thnaks,
Eddie
>
>>
>> + 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;
>> }
>> @@ -310,8 +322,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:
>> @@ -583,6 +598,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);
>> @@ -676,12 +694,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);
>>
>> @@ -752,23 +775,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;
>> @@ -778,20 +791,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_all(&client->wait);
>> + }
>> + 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