On Thu, Apr 6, 2017 at 2:30 AM, Hefty, Sean <sean.hefty@xxxxxxxxx> wrote:
> I need to study this more. Just a couple of minor comments below.
>
>> diff --git a/drivers/infiniband/core/uverbs.h
>> b/drivers/infiniband/core/uverbs.h
>> index 5f8a7f2..826f827 100644
>> --- a/drivers/infiniband/core/uverbs.h
>> +++ b/drivers/infiniband/core/uverbs.h
>> @@ -102,17 +102,25 @@ struct ib_uverbs_device {
>> };
>>
>> struct ib_uverbs_event_file {
>> - struct kref ref;
>> - int is_async;
>> - struct ib_uverbs_file *uverbs_file;
>> spinlock_t lock;
>> int is_closed;
>> wait_queue_head_t poll_wait;
>> struct fasync_struct *async_queue;
>> struct list_head event_list;
>> +};
>
> I would rename this structure to something like ib_uverbs_event_queue, since the file aspect has been removed, with corresponding name changes where it is used.
Sure, the base structure could be renamed to ib_uverbs_event_queue.
>
>> +
>> +struct ib_uverbs_async_event_file {
>> + struct ib_uverbs_event_file ev_file;
>> + struct ib_uverbs_file *uverbs_file;
>> + struct kref ref;
>> struct list_head list;
>> };
>>
>> +struct ib_uverbs_completion_event_file {
>> + struct ib_uobject_file uobj_file;
>> + struct ib_uverbs_event_file ev_file;
>> +};
>> +
>> struct ib_uverbs_file {
>> struct kref ref;
>> struct mutex mutex;
>> @@ -120,7 +128,7 @@ struct ib_uverbs_file {
>> struct ib_uverbs_device *device;
>> struct ib_ucontext *ucontext;
>> struct ib_event_handler event_handler;
>> - struct ib_uverbs_event_file *async_file;
>> + struct ib_uverbs_async_event_file *async_file;
>> struct list_head list;
>> int is_closed;
>>
>
>> @@ -253,10 +253,12 @@ void ib_uverbs_release_file(struct kref *ref)
>> kfree(file);
>> }
>>
>> -static ssize_t ib_uverbs_event_read(struct file *filp, char __user
>> *buf,
>> - size_t count, loff_t *pos)
>> +static ssize_t ib_uverbs_event_read(struct ib_uverbs_event_file
>> *file,
>> + struct ib_uverbs_file *uverbs_file,
>> + struct file *filp, char __user *buf,
>> + size_t count, loff_t *pos,
>> + bool is_async)
>> {
>> - struct ib_uverbs_event_file *file = filp->private_data;
>> struct ib_uverbs_event *event;
>> int eventsz;
>> int ret = 0;
>> @@ -275,12 +277,12 @@ static ssize_t ib_uverbs_event_read(struct file
>> *filp, char __user *buf,
>> * and wake_up() guarentee this will see the null
>> set
>> * without using RCU
>> */
>> - !file->uverbs_file->device-
>> >ib_dev)))
>> + !uverbs_file->device->ib_dev)))
>> return -ERESTARTSYS;
>>
>> /* If device was disassociated and no event exists set an
>> error */
>> if (list_empty(&file->event_list) &&
>> - !file->uverbs_file->device->ib_dev)
>> + !uverbs_file->device->ib_dev)
>> return -EIO;
>>
>> spin_lock_irq(&file->lock);
>> @@ -288,7 +290,7 @@ static ssize_t ib_uverbs_event_read(struct file
>> *filp, char __user *buf,
>>
>> event = list_entry(file->event_list.next, struct
>> ib_uverbs_event, list);
>>
>> - if (file->is_async)
>> + if (is_async)
>> eventsz = sizeof (struct ib_uverbs_async_event_desc);
>> else
>> eventsz = sizeof (struct ib_uverbs_comp_event_desc);
>
> Consider adding an event size to ib_uverbs_event_file rather than assuming the size based on is_async.
>
Ok
>> @@ -318,11 +320,31 @@ static ssize_t ib_uverbs_event_read(struct file
>> *filp, char __user *buf,
>> return ret;
>> }
>>
>> -static unsigned int ib_uverbs_event_poll(struct file *filp,
>> +static ssize_t ib_uverbs_async_event_read(struct file *filp, char
>> __user *buf,
>> + size_t count, loff_t *pos)
>> +{
>> + struct ib_uverbs_async_event_file *file = filp->private_data;
>> +
>> + return ib_uverbs_event_read(&file->ev_file, file->uverbs_file,
>> filp,
>> + buf, count, pos, true);
>> +}
>> +
>> +static ssize_t ib_uverbs_comp_event_read(struct file *filp, char
>> __user *buf,
>> + size_t count, loff_t *pos)
>> +{
>> + struct ib_uverbs_completion_event_file *comp_ev_file =
>> + filp->private_data;
>> +
>> + return ib_uverbs_event_read(&comp_ev_file->ev_file,
>> + comp_ev_file->uobj_file.ufile, filp,
>> + buf, count, pos, false);
>> +}
>
> - Sean
Thanks for the review.
Matan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
