[PATCH linux dev-4.10 v3 13/31] drivers: fsi: SBEFIFO: Fix probe() and remove()

Andrew Jeffery andrew at aj.id.au
Fri Oct 6 09:47:52 AEDT 2017


On Thu, 2017-10-05 at 14:24 -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.
> 
> Signed-off-by: Edward A. James <eajames at us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew at aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 111 ++++++++++++++++++++++------------------------
>  1 file changed, 53 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index a2acf76..9b560ec 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);
> @@ -605,7 +605,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,9 +664,9 @@ 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)) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(xfr);
>  		goto out;
>  	}
>  
> @@ -769,18 +769,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;
>  }
> @@ -854,69 +851,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_all(&sbefifo->wait);
>  
> -		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;
>  }
> @@ -941,7 +937,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/20171006/9cc6e0f1/attachment.sig>


More information about the openbmc mailing list