From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965751AbcCJKNe (ORCPT ); Thu, 10 Mar 2016 05:13:34 -0500 Received: from mail-pf0-f174.google.com ([209.85.192.174]:36849 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965672AbcCJKMg (ORCPT ); Thu, 10 Mar 2016 05:12:36 -0500 Subject: Re: [PATCH 1/2] vfio, platform: add support for ACPI while detecting the reset driver To: Sinan Kaya , kvm@vger.kernel.org, timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com References: <1457451209-21462-1-git-send-email-okaya@codeaurora.org> Cc: shankerd@codeaurora.org, vikrams@codeaurora.org, marc.zyngier@arm.com, mark.rutland@arm.com, devicetree@vger.kernel.org, vinod.koul@intel.com, agross@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Baptiste Reynal , Alex Williamson , Dan Carpenter , Arnd Bergmann , linux-kernel@vger.kernel.org From: Eric Auger Message-ID: <56E14858.9050401@linaro.org> Date: Thu, 10 Mar 2016 11:11:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1457451209-21462-1-git-send-email-okaya@codeaurora.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sinan, On 03/08/2016 04:33 PM, Sinan Kaya wrote: > The code is using the compatible DT string to associate a reset driver with > the actual device itself. The compatible string does not exist on ACPI > based systems. HID is the unique identifier for a device driver instead. > The change allows a driver to register with DT compatible string or ACPI > HID and then match the object with one of these conditions. > > Rules for loading the reset driver are as follow: > - ACPI HID needs match for ACPI systems > - DT compat needs to match for OF systems > > Tested-by: Eric Auger (device tree only) > Tested-by: Shanker Donthineni (ACPI only) > Signed-off-by: Sinan Kaya > --- > .../vfio/platform/reset/vfio_platform_amdxgbe.c | 4 +- > .../platform/reset/vfio_platform_calxedaxgmac.c | 4 +- > drivers/vfio/platform/vfio_platform_common.c | 112 +++++++++++++++++---- > drivers/vfio/platform/vfio_platform_private.h | 43 ++++---- > 4 files changed, 125 insertions(+), 38 deletions(-) > > diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > index d4030d0..170dcf5 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > @@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev) > return 0; > } > > -module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset); > +module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL, > + vfio_platform_amdxgbe_reset); > +VFIO_PLATFORM_MODULE_ALIAS("amd,xgbe-seattle-v1a"); Looks the other overridden MODULE_ALIAS have a naming like MODULE_ALIAS_something? what about MODULE_ALIAS_VFIO_PLATFORM_RESET? not very compact but maybe closer to what it stands for. > > MODULE_VERSION("0.1"); > MODULE_LICENSE("GPL v2"); > diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > index e3d3d94..635c6e0 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > @@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) > return 0; > } > > -module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset); > +module_vfio_reset_handler("calxeda,hb-xgmac", NULL, > + vfio_platform_calxedaxgmac_reset); > +VFIO_PLATFORM_MODULE_ALIAS("calxeda,hb-xgmac"); > > MODULE_VERSION(DRIVER_VERSION); > MODULE_LICENSE("GPL v2"); > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 418cdd9..c758e72 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -13,6 +13,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -31,14 +32,22 @@ static LIST_HEAD(reset_list); > static DEFINE_MUTEX(driver_lock); > > static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, > - struct module **module) > + const char *acpihid, struct module **module) > { > struct vfio_platform_reset_node *iter; > vfio_platform_reset_fn_t reset_fn = NULL; > > mutex_lock(&driver_lock); > list_for_each_entry(iter, &reset_list, link) { > - if (!strcmp(iter->compat, compat) && > + if (acpihid && iter->acpihid && > + !strcmp(iter->acpihid, acpihid) && > + try_module_get(iter->owner)) { > + *module = iter->owner; > + reset_fn = iter->reset; > + break; > + } > + if (compat && iter->compat && > + !strcmp(iter->compat, compat) && > try_module_get(iter->owner)) { > *module = iter->owner; > reset_fn = iter->reset; > @@ -49,15 +58,30 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, > return reset_fn; > } > > -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) What is the point returning a value here? See my comment at the end. > { > - vdev->reset = vfio_platform_lookup_reset(vdev->compat, > - &vdev->reset_module); > - if (!vdev->reset) { > - request_module("vfio-reset:%s", vdev->compat); > - vdev->reset = vfio_platform_lookup_reset(vdev->compat, > - &vdev->reset_module); > - } > + int rc = 0; > + > + vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid, > + &vdev->reset_module); > + if (vdev->reset) > + return 0; > + > + if (vdev->acpihid) > + rc = request_module("vfio-reset:%s", vdev->acpihid); > + > + if (rc && vdev->compat) > + rc = request_module("vfio-reset:%s", vdev->compat); > + > + if (rc) > + return rc; > + > + vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid, > + &vdev->reset_module); > + if (vdev->reset) > + return 0; > + > + return -ENODEV; > } > > static void vfio_platform_put_reset(struct vfio_platform_device *vdev) > @@ -541,6 +565,46 @@ static const struct vfio_device_ops vfio_platform_ops = { > .mmap = vfio_platform_mmap, > }; > > +#ifdef CONFIG_ACPI > +int vfio_platform_probe_acpi(struct vfio_platform_device *vdev, > + struct device *dev) > +{ > + struct acpi_device* adev = ACPI_COMPANION(dev); checkpatch reports: ERROR: "foo* bar" should be "foo *bar" > + > + if (!adev) > + return -ENODEV; > + > + vdev->acpihid = acpi_device_hid(adev); > + if (!vdev->acpihid) { > + pr_err("VFIO: cannot find ACPI HID for %s\n", > + vdev->name); > + return -ENODEV; > + } > + return 0; > +} > +#else > +int vfio_platform_probe_acpi(struct vfio_platform_device *vdev, > + struct device *dev) > +{ > + return -EINVAL; > +} > +#endif > + > +int vfio_platform_probe_of(struct vfio_platform_device *vdev, > + struct device *dev) > +{ > + int ret; > + > + ret = device_property_read_string(dev, "compatible", > + &vdev->compat); > + if (ret) { > + pr_err("VFIO: cannot retrieve compat for %s\n", > + vdev->name); > + return ret; > + } > + return 0; > +} > + > int vfio_platform_probe_common(struct vfio_platform_device *vdev, > struct device *dev) > { > @@ -550,14 +614,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > if (!vdev) > return -EINVAL; > > - ret = device_property_read_string(dev, "compatible", &vdev->compat); > - if (ret) { > - pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name); > - return -EINVAL; > - } > + ret = vfio_platform_probe_acpi(vdev, dev); > + if (ret) > + ret = vfio_platform_probe_of(vdev, dev); > > - vdev->device = dev; > + if (ret) > + return ret; > > + vdev->device = dev; > group = iommu_group_get(dev); > if (!group) { > pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); > @@ -570,7 +634,11 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > return ret; > } > > - vfio_platform_get_reset(vdev); > + ret = vfio_platform_get_reset(vdev); > + if (ret) { > + iommu_group_put(group); > + return ret; > + } This change is not related to your commit message. Also here you change the use model of VFIO platform and forbid any usage if no reset module is available, right? I don't think this is something we discussed and I think it removes some flexibility. Currently a warning is emitted in case we don't have a reset function. Best Regards Eric > > mutex_init(&vdev->igate); > > @@ -602,13 +670,21 @@ void __vfio_platform_register_reset(struct vfio_platform_reset_node *node) > EXPORT_SYMBOL_GPL(__vfio_platform_register_reset); > > void vfio_platform_unregister_reset(const char *compat, > + const char *acpihid, > vfio_platform_reset_fn_t fn) > { > struct vfio_platform_reset_node *iter, *temp; > > mutex_lock(&driver_lock); > list_for_each_entry_safe(iter, temp, &reset_list, link) { > - if (!strcmp(iter->compat, compat) && (iter->reset == fn)) { > + if (acpihid && iter->acpihid && > + !strcmp(iter->acpihid, acpihid) && (iter->reset == fn)) { > + list_del(&iter->link); > + break; > + } > + > + if (compat && iter->compat && > + !strcmp(iter->compat, compat) && (iter->reset == fn)) { > list_del(&iter->link); > break; > } > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index 42816dd..20787f7 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -58,6 +58,7 @@ struct vfio_platform_device { > struct mutex igate; > struct module *parent_module; > const char *compat; > + const char *acpihid; > struct module *reset_module; > struct device *device; > > @@ -79,6 +80,7 @@ typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev); > struct vfio_platform_reset_node { > struct list_head link; > char *compat; > + char *acpihid; > struct module *owner; > vfio_platform_reset_fn_t reset; > }; > @@ -98,27 +100,32 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev, > > extern void __vfio_platform_register_reset(struct vfio_platform_reset_node *n); > extern void vfio_platform_unregister_reset(const char *compat, > + const char *acpihid, > vfio_platform_reset_fn_t fn); > -#define vfio_platform_register_reset(__compat, __reset) \ > -static struct vfio_platform_reset_node __reset ## _node = { \ > - .owner = THIS_MODULE, \ > - .compat = __compat, \ > - .reset = __reset, \ > -}; \ > + > +#define vfio_platform_register_reset(__compat, __acpihid, __reset) \ > +static struct vfio_platform_reset_node __reset ## _node = { \ > + .owner = THIS_MODULE, \ > + .compat = __compat, \ > + .acpihid = __acpihid, \ > + .reset = __reset, \ > +}; \ > __vfio_platform_register_reset(&__reset ## _node) > > -#define module_vfio_reset_handler(compat, reset) \ > -MODULE_ALIAS("vfio-reset:" compat); \ > -static int __init reset ## _module_init(void) \ > -{ \ > - vfio_platform_register_reset(compat, reset); \ > - return 0; \ > -}; \ > -static void __exit reset ## _module_exit(void) \ > -{ \ > - vfio_platform_unregister_reset(compat, reset); \ > -}; \ > -module_init(reset ## _module_init); \ > +#define VFIO_PLATFORM_MODULE_ALIAS(name) \ > + MODULE_ALIAS("vfio-reset:" name) > + > +#define module_vfio_reset_handler(compat, acpihid, reset) \ > +static int __init reset ## _module_init(void) \ > +{ \ > + vfio_platform_register_reset(compat, acpihid, reset); \ > + return 0; \ > +}; \ > +static void __exit reset ## _module_exit(void) \ > +{ \ > + vfio_platform_unregister_reset(compat, acpihid, reset); \ > +}; \ > +module_init(reset ## _module_init); \ > module_exit(reset ## _module_exit) > > #endif /* VFIO_PLATFORM_PRIVATE_H */ >