[PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()
Andrew Jeffery
andrew at aj.id.au
Thu Oct 5 11:32:29 AEDT 2017
On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames at us.ibm.com>
>
> Probe didn't handle a failed misc device registration properly. Remove
> had the potential for hangs. Also, remove the list of SBEFIFOs and
> instead just use the device "driver data" pointer.
What was the purpose of the list of SBEFIFOs? Are we losing something from the
intended design here? The list could be stored in the driver data pointer
right?
>
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> drivers/fsi/fsi-sbefifo.c | 109 ++++++++++++++++++++++------------------------
> 1 file changed, 52 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index a4cd353..fa34bd8 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -58,7 +58,6 @@ struct sbefifo {
> struct fsi_device *fsi_dev;
> struct miscdevice mdev;
> wait_queue_head_t wait;
> - struct list_head link;
> struct list_head xfrs;
> struct kref kref;
> spinlock_t lock;
> @@ -96,8 +95,6 @@ struct sbefifo_client {
> unsigned long f_flags;
> };
>
> -static struct list_head sbefifo_fifos;
> -
> static DEFINE_IDA(sbefifo_ida);
>
> static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
> @@ -267,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
> struct sbefifo *sbefifo = client->dev;
> struct sbefifo_xfr *xfr;
>
> + if (READ_ONCE(sbefifo->rc))
> + return ERR_PTR(sbefifo->rc);
> +
> xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
> if (!xfr)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> xfr->rbuf = &client->rbuf;
> xfr->wbuf = &client->wbuf;
> @@ -490,7 +490,7 @@ static void sbefifo_poll_timer(unsigned long data)
> }
>
> sbefifo_put(sbefifo);
> - wake_up(&sbefifo->wait);
> + wake_up_interruptible(&sbefifo->wait);
>
> out_unlock:
> spin_unlock(&sbefifo->lock);
> @@ -604,7 +604,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
> } else {
> kfree(xfr);
> list_del(&xfr->client);
> - wake_up(&sbefifo->wait);
> + wake_up_interruptible(&sbefifo->wait);
> }
> }
>
> @@ -664,7 +664,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
> }
>
> xfr = sbefifo_enq_xfr(client); /* this xfr queued up */
> - if (!xfr) {
> + if (IS_ERR(xfr)) {
Should this be in what is currently patch 0/9?
> spin_unlock_irq(&sbefifo->lock);
> ret = PTR_ERR(xfr);
> goto out;
> @@ -766,18 +766,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)
> struct sbefifo_client *sbefifo_drv_open(struct device *dev,
> unsigned long flags)
> {
> - struct sbefifo_client *client = NULL;
> - struct sbefifo *sbefifo;
> - struct fsi_device *fsi_dev = to_fsi_dev(dev);
> + struct sbefifo_client *client;
> + struct sbefifo *sbefifo = dev_get_drvdata(dev);
>
> - list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
> - if (sbefifo->fsi_dev != fsi_dev)
> - continue;
> + if (!sbefifo)
> + return NULL;
>
> - client = sbefifo_new_client(sbefifo);
> - if (client)
> - client->f_flags = flags;
> - }
> + client = sbefifo_new_client(sbefifo);
> + if (client)
> + client->f_flags = flags;
>
> return client;
> }
> @@ -850,69 +847,68 @@ static int sbefifo_probe(struct device *dev)
> return -EIO;
> }
>
> - sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> - sbefifo->mdev.fops = &sbefifo_fops;
> - sbefifo->mdev.name = sbefifo->name;
> - sbefifo->mdev.parent = dev;
> spin_lock_init(&sbefifo->lock);
> kref_init(&sbefifo->kref);
> + init_waitqueue_head(&sbefifo->wait);
> + INIT_LIST_HEAD(&sbefifo->xfrs);
>
> sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
> snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
> sbefifo->idx);
> - init_waitqueue_head(&sbefifo->wait);
> - INIT_LIST_HEAD(&sbefifo->xfrs);
>
> /* This bit of silicon doesn't offer any interrupts... */
> setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
> (unsigned long)sbefifo);
>
> - if (dev->of_node) {
> - /* create platform devs for dts child nodes (occ, etc) */
> - for_each_child_of_node(dev->of_node, np) {
> - snprintf(child_name, sizeof(child_name), "%s-dev%d",
> - sbefifo->name, child_idx++);
> - child = of_platform_device_create(np, child_name, dev);
> - if (!child)
> - dev_warn(dev,
> - "failed to create child node dev\n");
> - }
> + sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> + sbefifo->mdev.fops = &sbefifo_fops;
> + sbefifo->mdev.name = sbefifo->name;
> + sbefifo->mdev.parent = dev;
> + ret = misc_register(&sbefifo->mdev);
> + if (ret) {
> + dev_err(dev, "failed to register miscdevice: %d\n", ret);
> + ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> + sbefifo_put(sbefifo);
> + return ret;
> }
>
> - list_add(&sbefifo->link, &sbefifo_fifos);
> -
> - return misc_register(&sbefifo->mdev);
> + /* create platform devs for dts child nodes (occ, etc) */
> + for_each_available_child_of_node(dev->of_node, np) {
> + snprintf(child_name, sizeof(child_name), "%s-dev%d",
> + sbefifo->name, child_idx++);
> + child = of_platform_device_create(np, child_name, dev);
> + if (!child)
> + dev_warn(dev, "failed to create child %s dev\n",
> + child_name);
> + }
> +
> + dev_set_drvdata(dev, sbefifo);
> +
> + return 0;
> }
>
> static int sbefifo_remove(struct device *dev)
> {
> - struct fsi_device *fsi_dev = to_fsi_dev(dev);
> - struct sbefifo *sbefifo, *sbefifo_tmp;
> + struct sbefifo *sbefifo = dev_get_drvdata(dev);
> struct sbefifo_xfr *xfr;
>
> - list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> - if (sbefifo->fsi_dev != fsi_dev)
> - continue;
> + WRITE_ONCE(sbefifo->rc, -ENODEV);
> + wake_up_interruptible_all(&sbefifo->wait);
If we're removing, we want to wake up non-interruptible tasks as well, so I
think wake_up_all() is more appropriate.
>
> - device_for_each_child(dev, NULL, sbefifo_unregister_child);
> + misc_deregister(&sbefifo->mdev);
> + device_for_each_child(dev, NULL, sbefifo_unregister_child);
>
> - misc_deregister(&sbefifo->mdev);
> - list_del(&sbefifo->link);
> - ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> -
> - if (del_timer_sync(&sbefifo->poll_timer))
> - sbefifo_put(sbefifo);
> + ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>
> - spin_lock(&sbefifo->lock);
> - list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> - kfree(xfr);
> - spin_unlock(&sbefifo->lock);
> + if (del_timer_sync(&sbefifo->poll_timer))
> + sbefifo_put(sbefifo);
>
> - WRITE_ONCE(sbefifo->rc, -ENODEV);
> + spin_lock(&sbefifo->lock);
> + list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> + kfree(xfr);
> + spin_unlock(&sbefifo->lock);
>
> - wake_up(&sbefifo->wait);
> - sbefifo_put(sbefifo);
> - }
> + sbefifo_put(sbefifo);
>
> return 0;
> }
> @@ -937,7 +933,6 @@ static int sbefifo_remove(struct device *dev)
>
> static int sbefifo_init(void)
> {
> - INIT_LIST_HEAD(&sbefifo_fifos);
> return fsi_driver_register(&sbefifo_drv);
> }
>
-------------- 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/b7bfeb6b/attachment.sig>
More information about the openbmc
mailing list