From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754853AbbEMSct (ORCPT ); Wed, 13 May 2015 14:32:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35954 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754832AbbEMScp (ORCPT ); Wed, 13 May 2015 14:32:45 -0400 Message-ID: <1431541925.3625.52.camel@redhat.com> Subject: Re: [PATCH 1/5] VFIO: platform: add reset_list and register/unregister functions From: Alex Williamson To: Eric Auger Cc: eric.auger@st.com, christoffer.dall@linaro.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, b.reynal@virtualopensystems.com, linux-kernel@vger.kernel.org, patches@linaro.org, agraf@suse.de, Bharat.Bhushan@freescale.com Date: Wed, 13 May 2015 12:32:05 -0600 In-Reply-To: <1431008843-28411-2-git-send-email-eric.auger@linaro.org> References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-2-git-send-email-eric.auger@linaro.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 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?! > > 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 > + 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 > + 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? > + 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 > + 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. > + > +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 */ From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH 1/5] VFIO: platform: add reset_list and register/unregister functions Date: Wed, 13 May 2015 12:32:05 -0600 Message-ID: <1431541925.3625.52.camel@redhat.com> References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-2-git-send-email-eric.auger@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, patches@linaro.org, linux-kernel@vger.kernel.org, eric.auger@st.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org To: Eric Auger Return-path: In-Reply-To: <1431008843-28411-2-git-send-email-eric.auger@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org 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 > --- > 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 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?! > > 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 > + 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 > + 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? > + 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 > + 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. > + > +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 */ From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.williamson@redhat.com (Alex Williamson) Date: Wed, 13 May 2015 12:32:05 -0600 Subject: [PATCH 1/5] VFIO: platform: add reset_list and register/unregister functions In-Reply-To: <1431008843-28411-2-git-send-email-eric.auger@linaro.org> References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-2-git-send-email-eric.auger@linaro.org> Message-ID: <1431541925.3625.52.camel@redhat.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > --- > 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 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?! > > 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 > + 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 > + 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? > + 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 > + 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. > + > +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 */