| Message ID | 1457451209-21462-1-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/8/2016 10:33 AM, Sinan Kaya wrote: > + int rc = 0; > + I should have initialized this to -1. > + 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); > +
On Tue, Mar 08, 2016 at 10:37:34AM -0500, Sinan Kaya wrote: > On 3/8/2016 10:33 AM, Sinan Kaya wrote: > > + int rc = 0; > > + > > I should have initialized this to -1. > -1 is a magic number which stands for -EPERM which is wrong. Use -ENODEV. regards, dan carpenter
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 <eric.auger@linaro.org> (device tree only) > Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > .../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 <linux/device.h> > +#include <linux/acpi.h> > #include <linux/iommu.h> > #include <linux/module.h> > #include <linux/mutex.h> > @@ -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 */ >
On 3/10/2016 2:58 AM, Dan Carpenter wrote: > -1 is a magic number which stands for -EPERM which is wrong. Use > -ENODEV. OK. done.
On 3/10/2016 5:11 AM, Eric Auger wrote: > 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 <eric.auger@linaro.org> (device tree only) >> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> .../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. alright, I'll follow that. >> >> 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 <linux/device.h> >> +#include <linux/acpi.h> >> #include <linux/iommu.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> @@ -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. I was trying to do some error handling here. If a driver does not have a reset implementation, we are letting it go right now. I think we need to make reset driver a requirement. Mark Rutland reminded me that I need a reset driver. I did not think about it during implementation and I have not seen any warnings like you said. >> { >> - 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); >> - } >> >> - 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. Well, I haven't seen that warning during testing. I was trying to be more proactive. I'm fine removing these checks but not having a reset driver needs a really big warning here. > > Best Regards > > Eric >>
Hi Sinan, On 03/10/2016 06:13 PM, Sinan Kaya wrote: > On 3/10/2016 5:11 AM, Eric Auger wrote: >> 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 <eric.auger@linaro.org> (device tree only) >>> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>> --- >>> .../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. > > alright, I'll follow that. > >>> >>> 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 <linux/device.h> >>> +#include <linux/acpi.h> >>> #include <linux/iommu.h> >>> #include <linux/module.h> >>> #include <linux/mutex.h> >>> @@ -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. > > I was trying to do some error handling here. If a driver does not have a > reset implementation, we are letting it go right now. > > I think we need to make reset driver a requirement. Mark Rutland reminded me > that I need a reset driver. > > I did not think about it during implementation and I have not seen any > warnings like you said. the warning currently is emitted on vfio_platform_open/release: dev_warn(vdev->device, "no reset function found!\n"); > >>> { >>> - 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); >>> - } > > >>> >>> - 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. > > Well, I haven't seen that warning during testing. I was trying to be more > proactive. Thank you for that. Personally I agree to proceed with the proposed change, ie. forbid vfio platform driver to be used if there is no reset function found but this should be in a separate patch with explicit commit mesg and you should remove dev_warn & related dead code. Best Regards Eric > > I'm fine removing these checks but not having a reset driver needs a really > big warning here. > >> >> Best Regards >> >> Eric >>> > > >
On Thu, 10 Mar 2016 12:13:28 -0500 Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/10/2016 5:11 AM, Eric Auger wrote: > > 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 <eric.auger@linaro.org> (device tree only) > >> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) > >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > >> --- > >> .../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(-) [...] > >> - 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. > > Well, I haven't seen that warning during testing. I was trying to be more > proactive. > > I'm fine removing these checks but not having a reset driver needs a really > big warning here. This code really needs to be moved to a separate patch, but I agree overall: a reset function should be made mandatory - I cannot imagine trusting the HW to be left in a sane state after being used by a guest. Thanks, M.
On 3/10/2016 10:48 PM, Eric Auger wrote: >> Well, I haven't seen that warning during testing. I was trying to be more >> > proactive. > Thank you for that. Personally I agree to proceed with the proposed > change, ie. forbid vfio platform driver to be used if there is no reset > function found but this should be in a separate patch with explicit > commit mesg and you should remove dev_warn & related dead code. OK. I'm getting read to post another series and this will be an independent change.
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"); 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 <linux/device.h> +#include <linux/acpi.h> #include <linux/iommu.h> #include <linux/module.h> #include <linux/mutex.h> @@ -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) { - 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); + + 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; + } 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 */