[PATCH linux dev-4.10 v2 6/9] drivers: fsi: occ: Add cancel to remove() and fix probe()
Andrew Jeffery
andrew at aj.id.au
Thu Oct 5 12:07:30 AEDT 2017
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()?
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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171005/8fbf73c0/attachment.sig>
More information about the openbmc
mailing list