Hi Alex,
On 05/13/2015 08:32 PM, Alex Williamson wrote:
> On Thu, 2015-05-07 at 16:27 +0200, Eric Auger wrote:
>> vfio_platform_common now stores a lists of available reset functions.
>> Two functions are exposed to register/unregister a reset function. A
>> reset function is paired with a compat string.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 63 +++++++++++++++++++++++++++
>> drivers/vfio/platform/vfio_platform_private.h | 13 ++++++
>> 2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index abcff7a..edbf24c 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -23,6 +23,9 @@
>>
>> #include "vfio_platform_private.h"
>>
>> +struct list_head reset_list;
>> +LIST_HEAD(reset_list);
>> +
>
> Redundant? Static?
static, yes
>
>> static DEFINE_MUTEX(driver_lock);
>>
>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>> @@ -511,6 +514,13 @@ EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
>> struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>> {
>> struct vfio_platform_device *vdev;
>> + struct vfio_platform_reset_node *iter, *tmp;
>> +
>> + list_for_each_entry_safe(iter, tmp, &reset_list, link) {
>> + list_del(&iter->link);
>> + kfree(iter->compat);
>> + kfree(iter);
>> + }
>
>
> This doesn't make sense. We allow reset functions to be registered and
> unregistered, but we forget them all when any device is released?!
I acknowledge indeed. Can I rely on the reset module exit and associated
unregister_reset or shall I take this action in the vfio driver itself,
core?
>
>>
>> vdev = vfio_del_group_dev(dev);
>> if (vdev)
>> @@ -519,3 +529,56 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>> return vdev;
>> }
>> EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
>> +
>> +int vfio_platform_register_reset(char *compat, struct module *reset_owner,
>> + vfio_platform_reset_fn_t reset)
>> +{
>> + struct vfio_platform_reset_node *node, *iter;
>> + bool found = false;
>> +
>> + list_for_each_entry(iter, &reset_list, link) {
>> + if (!strcmp(iter->compat, compat)) {
>> + found = true;
>
> Just return errno here
ok
>
>> + break;
>> + }
>> + }
>> + if (found)
>> + return -EINVAL;
>> +
>> + node = kmalloc(sizeof(*node), GFP_KERNEL);
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + node->compat = kstrdup(compat, GFP_KERNEL);
>> + if (!node->compat)
>
> Leaking node
ok
>
>> + return -ENOMEM;
>> +
>> + node->owner = reset_owner;
>> + node->reset = reset;
>> +
>> + list_add(&node->link, &reset_list);
>
> Isn't this racy? Don't we need some locks around the list?
I will add a lock to protect access to the list.
>
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vfio_platform_register_reset);
>> +
>> +int vfio_platform_unregister_reset(char *compat)
>> +{
>> + struct vfio_platform_reset_node *iter;
>> + bool found = false;
>> +
>> + list_for_each_entry(iter, &reset_list, link) {
>> + if (!strcmp(iter->compat, compat)) {
>
> Return errno here
ok
>
>> + found = true;
>> + break;
>> + }
>> + }
>> + if (!found)
>> + return -EINVAL;
>> +
>> + list_del(&iter->link);
>
> Racy
>
>> + kfree(iter->compat);
>> + kfree(iter);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vfio_platform_unregister_reset);
>> +
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 5d31e04..da2d60b 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -69,6 +69,15 @@ struct vfio_platform_device {
>> int (*get_irq)(struct vfio_platform_device *vdev, int i);
>> };
>>
>> +typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
>
> Seems like this ought to be in a non-private header if we're exporting
> the [un]register functions.
I considered the vfio reset modules were internal to the vfio subsystem
but if you prefer I can expose that in vfio.h. I guess
register/unregister should become an external API then?
Thanks
Eric
>> +
>> +struct vfio_platform_reset_node {
>> + struct list_head link;
>> + char *compat;
>> + struct module *owner;
>> + vfio_platform_reset_fn_t reset;
>> +};
>> +
>> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> struct device *dev);
>> extern struct vfio_platform_device *vfio_platform_remove_common
>> @@ -82,4 +91,8 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>> unsigned start, unsigned count,
>> void *data);
>>
>> +extern int vfio_platform_register_reset(char *compat, struct module *owner,
>> + vfio_platform_reset_fn_t reset);
>> +extern int vfio_platform_unregister_reset(char *compat);
>> +
>> #endif /* VFIO_PLATFORM_PRIVATE_H */
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
