From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Auger Subject: Re: [PATCH V13 09/10] vfio, platform: add support for ACPI while detecting the reset driver Date: Mon, 1 Feb 2016 17:08:26 +0100 Message-ID: <56AF82FA.7000303@linaro.org> References: <1454106914-15857-1-git-send-email-okaya@codeaurora.org> <1454106914-15857-10-git-send-email-okaya@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wm0-f41.google.com ([74.125.82.41]:36956 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753829AbcBAQIs (ORCPT ); Mon, 1 Feb 2016 11:08:48 -0500 Received: by mail-wm0-f41.google.com with SMTP id l66so78584757wml.0 for ; Mon, 01 Feb 2016 08:08:48 -0800 (PST) In-Reply-To: <1454106914-15857-10-git-send-email-okaya@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Sinan Kaya , dmaengine@vger.kernel.org, marc.zyngier@arm.com, mark.rutland@arm.com, timur@codeaurora.org, devicetree@vger.kernel.org, cov@codeaurora.org, vinod.koul@intel.com, jcm@redhat.com Cc: shankerd@codeaurora.org, vikrams@codeaurora.org, agross@codeaurora.org, arnd@arndb.de, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Baptiste Reynal Hi Sinan, On 01/29/2016 11:35 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. > > For ACPI systems, ACPI HID needs to match and compat in the registered > reset > driver needs to match for ACPI reset driver loading to work. Don't really get the sentence. For ACPI systems, a registered reset function is selected if its associated ACPI HID matches the device ACPI HID? > > For OF based systems, DT compatible string needs to match and compat in the > registered reset driver needs to match for DT reset driver loading to work. same here I added Baptiste who is vfio platform driver sub-system maintainer. On my side I tested with of amd xgbe and I don't observe any regression. Best Regards Eric > > Signed-off-by: Sinan Kaya > --- > .../vfio/platform/reset/vfio_platform_amdxgbe.c | 3 +- > .../platform/reset/vfio_platform_calxedaxgmac.c | 3 +- > drivers/vfio/platform/vfio_platform_common.c | 80 +++++++++++++++++++--- > drivers/vfio/platform/vfio_platform_private.h | 41 ++++++----- > 4 files changed, 96 insertions(+), 31 deletions(-) > > diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > index d4030d0..cc5b4fa 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > @@ -119,7 +119,8 @@ 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); > > 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..0e57529 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > @@ -77,7 +77,8 @@ 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); > > 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..6927e05 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; > @@ -51,11 +60,12 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, > > static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > { > - vdev->reset = vfio_platform_lookup_reset(vdev->compat, > - &vdev->reset_module); > + vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid, > + &vdev->reset_module); > if (!vdev->reset) { > request_module("vfio-reset:%s", vdev->compat); > vdev->reset = vfio_platform_lookup_reset(vdev->compat, > + vdev->acpihid, > &vdev->reset_module); > } > } > @@ -541,6 +551,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); > + > + if (!adev) > + return -EINVAL; > + > + vdev->acpihid = acpi_device_hid(adev); > + if (!vdev->acpihid) { > + pr_err("VFIO: cannot find ACPI HID for %s\n", > + vdev->name); > + return -EINVAL; > + } > + 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 -EINVAL; > + } > + return 0; > +} > + > int vfio_platform_probe_common(struct vfio_platform_device *vdev, > struct device *dev) > { > @@ -550,14 +600,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); > @@ -602,13 +652,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..32feba3 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,30 @@ 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 module_vfio_reset_handler(compat, acpihid, reset) \ > +MODULE_ALIAS("vfio-reset:" compat); \ > +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 */ > From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@linaro.org (Eric Auger) Date: Mon, 1 Feb 2016 17:08:26 +0100 Subject: [PATCH V13 09/10] vfio, platform: add support for ACPI while detecting the reset driver In-Reply-To: <1454106914-15857-10-git-send-email-okaya@codeaurora.org> References: <1454106914-15857-1-git-send-email-okaya@codeaurora.org> <1454106914-15857-10-git-send-email-okaya@codeaurora.org> Message-ID: <56AF82FA.7000303@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Sinan, On 01/29/2016 11:35 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. > > For ACPI systems, ACPI HID needs to match and compat in the registered > reset > driver needs to match for ACPI reset driver loading to work. Don't really get the sentence. For ACPI systems, a registered reset function is selected if its associated ACPI HID matches the device ACPI HID? > > For OF based systems, DT compatible string needs to match and compat in the > registered reset driver needs to match for DT reset driver loading to work. same here I added Baptiste who is vfio platform driver sub-system maintainer. On my side I tested with of amd xgbe and I don't observe any regression. Best Regards Eric > > Signed-off-by: Sinan Kaya > --- > .../vfio/platform/reset/vfio_platform_amdxgbe.c | 3 +- > .../platform/reset/vfio_platform_calxedaxgmac.c | 3 +- > drivers/vfio/platform/vfio_platform_common.c | 80 +++++++++++++++++++--- > drivers/vfio/platform/vfio_platform_private.h | 41 ++++++----- > 4 files changed, 96 insertions(+), 31 deletions(-) > > diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > index d4030d0..cc5b4fa 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c > @@ -119,7 +119,8 @@ 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); > > 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..0e57529 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > @@ -77,7 +77,8 @@ 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); > > 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..6927e05 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; > @@ -51,11 +60,12 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, > > static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > { > - vdev->reset = vfio_platform_lookup_reset(vdev->compat, > - &vdev->reset_module); > + vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid, > + &vdev->reset_module); > if (!vdev->reset) { > request_module("vfio-reset:%s", vdev->compat); > vdev->reset = vfio_platform_lookup_reset(vdev->compat, > + vdev->acpihid, > &vdev->reset_module); > } > } > @@ -541,6 +551,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); > + > + if (!adev) > + return -EINVAL; > + > + vdev->acpihid = acpi_device_hid(adev); > + if (!vdev->acpihid) { > + pr_err("VFIO: cannot find ACPI HID for %s\n", > + vdev->name); > + return -EINVAL; > + } > + 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 -EINVAL; > + } > + return 0; > +} > + > int vfio_platform_probe_common(struct vfio_platform_device *vdev, > struct device *dev) > { > @@ -550,14 +600,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); > @@ -602,13 +652,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..32feba3 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,30 @@ 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 module_vfio_reset_handler(compat, acpihid, reset) \ > +MODULE_ALIAS("vfio-reset:" compat); \ > +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 */ >