[PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()
Eddie James
eajames at linux.vnet.ibm.com
Fri Oct 6 02:15:19 AEDT 2017
On 10/04/2017 07:32 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>
>>
>> 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?
Right, originally we weren't sure if we could use the fsi device driver
data pointer. The only purpose of the list is to be able to find our
sbefifo device when given an fsi device pointer.
>
>>
>> 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?
No. This patch set changes the return value of sbefifo_enq_xfr to return
an appropriate error pointer.
>
>> 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.
OK. Though there are no non-interruptible wait_event calls in this driver.
Thanks,
Eddie
>
>>
>> - 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);
>> }
>>
More information about the openbmc
mailing list