[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 04:37:04 AEDT 2017
On 10/05/2017 10:15 AM, Eddie James wrote:
>
>
> 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.
Ah, I see, I had the PTR_ERR(xfr) change below in the wrong patch set.
Will fix.
>
>>
>>> 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