From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933007AbbENI5d (ORCPT ); Thu, 14 May 2015 04:57:33 -0400 Received: from mail-wg0-f51.google.com ([74.125.82.51]:36535 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932430AbbENI53 (ORCPT ); Thu, 14 May 2015 04:57:29 -0400 Message-ID: <55546374.7000208@linaro.org> Date: Thu, 14 May 2015 10:57:24 +0200 From: Eric Auger User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Alex Williamson 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 Subject: Re: [PATCH 4/5] VFIO: platform: populate reset function according to compat References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-5-git-send-email-eric.auger@linaro.org> <1431541996.3625.55.camel@redhat.com> In-Reply-To: <1431541996.3625.55.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/13/2015 08:33 PM, Alex Williamson wrote: > On Thu, 2015-05-07 at 16:27 +0200, Eric Auger wrote: >> Add the reset function lookup according to the device compat >> string. This lookup is added at different places: >> - on VFIO_DEVICE_GET_INFO >> - on VFIO_DEVICE_RESET >> - on device release >> >> A reference to the module implementing the reset function is taken >> on first reset function lookup and released on vfio platform device >> release. >> >> Signed-off-by: Eric Auger >> --- >> drivers/vfio/platform/vfio_platform_common.c | 50 ++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >> index 0d10018..bd7e44c 100644 >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -28,6 +28,52 @@ LIST_HEAD(reset_list); >> >> static DEFINE_MUTEX(driver_lock); >> >> +static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (!strcmp(iter->compat, compat) && >> + try_module_get(iter->owner)) >> + return iter->reset; >> + } >> + >> + return ERR_PTR(-ENODEV); > > return NULL imo ok > >> +} >> + >> +static vfio_platform_reset_fn_t vfio_platform_get_reset( >> + struct vfio_platform_device *vdev) >> +{ >> + struct device *dev = vdev->get_device(vdev); >> + const char *compat_str_array[2]; >> + vfio_platform_reset_fn_t reset; >> + int ret; >> + >> + if (!IS_ERR_OR_NULL(vdev->reset)) >> + return vdev->reset; >> + >> + ret = device_property_read_string_array(dev, "compatible", >> + compat_str_array, 2); >> + if (!ret) >> + return ERR_PTR(ret); >> + >> + reset = vfio_platform_lookup_reset(compat_str_array[0]); >> + return reset; > > Something got allocated into compat_str_array and gets leaked here. is there any allocation? device_property_read_string_array does not return -ENOMEM. > >> +} >> + >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (iter->reset == vdev->reset) { >> + module_put(iter->owner); >> + vdev->reset = NULL; >> + return; >> + } >> + } >> +} >> + >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >> { >> int cnt = 0, i; >> @@ -103,10 +149,12 @@ static void vfio_platform_release(void *device_data) >> mutex_lock(&driver_lock); >> >> if (!(--vdev->refcnt)) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->reset(vdev); >> vfio_platform_regions_cleanup(vdev); >> vfio_platform_irq_cleanup(vdev); >> + vfio_platform_put_reset(vdev); >> } >> >> mutex_unlock(&driver_lock); >> @@ -164,6 +212,7 @@ static long vfio_platform_ioctl(void *device_data, >> if (info.argsz < minsz) >> return -EINVAL; >> >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->flags |= VFIO_DEVICE_FLAGS_RESET; >> info.flags = vdev->flags; >> @@ -260,6 +309,7 @@ static long vfio_platform_ioctl(void *device_data, >> return ret; >> >> } else if (cmd == VFIO_DEVICE_RESET) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> return vdev->reset(vdev); >> else > > I count 3 gets and 1 put, isn't the module reference count increase > showing that? vfio_platform_get_reset simply returns if the function pointer already is populated so there is no systematic ref increment. This looks like it hasn't been tested. It did testing with external and in-kernel modules through Why would we do a > get every time we want to do a reset? My doubt were about the order of probing between the vfio-platform_driver and the vfio reset module? This question was the rationale of this implementation choice. But again the actual ref count increment is devised to be done once on the first entry point (iotcl or internal release) Do one get when the device is > registered or opened and one put when the device is unregistered or > closed. We don't want erratic userspace behavior that the reset > property of a device can come and go. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Auger Subject: Re: [PATCH 4/5] VFIO: platform: populate reset function according to compat Date: Thu, 14 May 2015 10:57:24 +0200 Message-ID: <55546374.7000208@linaro.org> References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-5-git-send-email-eric.auger@linaro.org> <1431541996.3625.55.camel@redhat.com> 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: Alex Williamson Return-path: In-Reply-To: <1431541996.3625.55.camel@redhat.com> 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 05/13/2015 08:33 PM, Alex Williamson wrote: > On Thu, 2015-05-07 at 16:27 +0200, Eric Auger wrote: >> Add the reset function lookup according to the device compat >> string. This lookup is added at different places: >> - on VFIO_DEVICE_GET_INFO >> - on VFIO_DEVICE_RESET >> - on device release >> >> A reference to the module implementing the reset function is taken >> on first reset function lookup and released on vfio platform device >> release. >> >> Signed-off-by: Eric Auger >> --- >> drivers/vfio/platform/vfio_platform_common.c | 50 ++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >> index 0d10018..bd7e44c 100644 >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -28,6 +28,52 @@ LIST_HEAD(reset_list); >> >> static DEFINE_MUTEX(driver_lock); >> >> +static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (!strcmp(iter->compat, compat) && >> + try_module_get(iter->owner)) >> + return iter->reset; >> + } >> + >> + return ERR_PTR(-ENODEV); > > return NULL imo ok > >> +} >> + >> +static vfio_platform_reset_fn_t vfio_platform_get_reset( >> + struct vfio_platform_device *vdev) >> +{ >> + struct device *dev = vdev->get_device(vdev); >> + const char *compat_str_array[2]; >> + vfio_platform_reset_fn_t reset; >> + int ret; >> + >> + if (!IS_ERR_OR_NULL(vdev->reset)) >> + return vdev->reset; >> + >> + ret = device_property_read_string_array(dev, "compatible", >> + compat_str_array, 2); >> + if (!ret) >> + return ERR_PTR(ret); >> + >> + reset = vfio_platform_lookup_reset(compat_str_array[0]); >> + return reset; > > Something got allocated into compat_str_array and gets leaked here. is there any allocation? device_property_read_string_array does not return -ENOMEM. > >> +} >> + >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (iter->reset == vdev->reset) { >> + module_put(iter->owner); >> + vdev->reset = NULL; >> + return; >> + } >> + } >> +} >> + >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >> { >> int cnt = 0, i; >> @@ -103,10 +149,12 @@ static void vfio_platform_release(void *device_data) >> mutex_lock(&driver_lock); >> >> if (!(--vdev->refcnt)) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->reset(vdev); >> vfio_platform_regions_cleanup(vdev); >> vfio_platform_irq_cleanup(vdev); >> + vfio_platform_put_reset(vdev); >> } >> >> mutex_unlock(&driver_lock); >> @@ -164,6 +212,7 @@ static long vfio_platform_ioctl(void *device_data, >> if (info.argsz < minsz) >> return -EINVAL; >> >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->flags |= VFIO_DEVICE_FLAGS_RESET; >> info.flags = vdev->flags; >> @@ -260,6 +309,7 @@ static long vfio_platform_ioctl(void *device_data, >> return ret; >> >> } else if (cmd == VFIO_DEVICE_RESET) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> return vdev->reset(vdev); >> else > > I count 3 gets and 1 put, isn't the module reference count increase > showing that? vfio_platform_get_reset simply returns if the function pointer already is populated so there is no systematic ref increment. This looks like it hasn't been tested. It did testing with external and in-kernel modules through Why would we do a > get every time we want to do a reset? My doubt were about the order of probing between the vfio-platform_driver and the vfio reset module? This question was the rationale of this implementation choice. But again the actual ref count increment is devised to be done once on the first entry point (iotcl or internal release) Do one get when the device is > registered or opened and one put when the device is unregistered or > closed. We don't want erratic userspace behavior that the reset > property of a device can come and go. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@linaro.org (Eric Auger) Date: Thu, 14 May 2015 10:57:24 +0200 Subject: [PATCH 4/5] VFIO: platform: populate reset function according to compat In-Reply-To: <1431541996.3625.55.camel@redhat.com> References: <1431008843-28411-1-git-send-email-eric.auger@linaro.org> <1431008843-28411-5-git-send-email-eric.auger@linaro.org> <1431541996.3625.55.camel@redhat.com> Message-ID: <55546374.7000208@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 05/13/2015 08:33 PM, Alex Williamson wrote: > On Thu, 2015-05-07 at 16:27 +0200, Eric Auger wrote: >> Add the reset function lookup according to the device compat >> string. This lookup is added at different places: >> - on VFIO_DEVICE_GET_INFO >> - on VFIO_DEVICE_RESET >> - on device release >> >> A reference to the module implementing the reset function is taken >> on first reset function lookup and released on vfio platform device >> release. >> >> Signed-off-by: Eric Auger >> --- >> drivers/vfio/platform/vfio_platform_common.c | 50 ++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >> index 0d10018..bd7e44c 100644 >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -28,6 +28,52 @@ LIST_HEAD(reset_list); >> >> static DEFINE_MUTEX(driver_lock); >> >> +static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (!strcmp(iter->compat, compat) && >> + try_module_get(iter->owner)) >> + return iter->reset; >> + } >> + >> + return ERR_PTR(-ENODEV); > > return NULL imo ok > >> +} >> + >> +static vfio_platform_reset_fn_t vfio_platform_get_reset( >> + struct vfio_platform_device *vdev) >> +{ >> + struct device *dev = vdev->get_device(vdev); >> + const char *compat_str_array[2]; >> + vfio_platform_reset_fn_t reset; >> + int ret; >> + >> + if (!IS_ERR_OR_NULL(vdev->reset)) >> + return vdev->reset; >> + >> + ret = device_property_read_string_array(dev, "compatible", >> + compat_str_array, 2); >> + if (!ret) >> + return ERR_PTR(ret); >> + >> + reset = vfio_platform_lookup_reset(compat_str_array[0]); >> + return reset; > > Something got allocated into compat_str_array and gets leaked here. is there any allocation? device_property_read_string_array does not return -ENOMEM. > >> +} >> + >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (iter->reset == vdev->reset) { >> + module_put(iter->owner); >> + vdev->reset = NULL; >> + return; >> + } >> + } >> +} >> + >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >> { >> int cnt = 0, i; >> @@ -103,10 +149,12 @@ static void vfio_platform_release(void *device_data) >> mutex_lock(&driver_lock); >> >> if (!(--vdev->refcnt)) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->reset(vdev); >> vfio_platform_regions_cleanup(vdev); >> vfio_platform_irq_cleanup(vdev); >> + vfio_platform_put_reset(vdev); >> } >> >> mutex_unlock(&driver_lock); >> @@ -164,6 +212,7 @@ static long vfio_platform_ioctl(void *device_data, >> if (info.argsz < minsz) >> return -EINVAL; >> >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->flags |= VFIO_DEVICE_FLAGS_RESET; >> info.flags = vdev->flags; >> @@ -260,6 +309,7 @@ static long vfio_platform_ioctl(void *device_data, >> return ret; >> >> } else if (cmd == VFIO_DEVICE_RESET) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> return vdev->reset(vdev); >> else > > I count 3 gets and 1 put, isn't the module reference count increase > showing that? vfio_platform_get_reset simply returns if the function pointer already is populated so there is no systematic ref increment. This looks like it hasn't been tested. It did testing with external and in-kernel modules through Why would we do a > get every time we want to do a reset? My doubt were about the order of probing between the vfio-platform_driver and the vfio reset module? This question was the rationale of this implementation choice. But again the actual ref count increment is devised to be done once on the first entry point (iotcl or internal release) Do one get when the device is > registered or opened and one put when the device is unregistered or > closed. We don't want erratic userspace behavior that the reset > property of a device can come and go. >