| Message ID | 1454106914-15857-10-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sinan,
[auto build test ERROR on vfio/next]
[also build test ERROR on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/dma-add-Qualcomm-Technologies-HIDMA-driver/20160130-064551
base: https://github.com/awilliam/linux-vfio.git next
config: arm64-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
drivers/vfio/platform/vfio_platform_common.c: In function 'vfio_platform_probe_acpi':
>> drivers/vfio/platform/vfio_platform_common.c:558:9: error: invalid initializer
struct acpi_device adev = ACPI_COMPANION(dev);
^
>> drivers/vfio/platform/vfio_platform_common.c:560:6: error: wrong type argument to unary exclamation mark
if (!adev)
^
>> drivers/vfio/platform/vfio_platform_common.c:563:18: error: incompatible type for argument 1 of 'acpi_device_hid'
vdev->acpihid = acpi_device_hid(adev);
^
In file included from include/linux/acpi.h:41:0,
from drivers/vfio/platform/vfio_platform_common.c:16:
include/acpi/acpi_bus.h:253:13: note: expected 'struct acpi_device *' but argument is of type 'struct acpi_device'
const char *acpi_device_hid(struct acpi_device *device);
^
vim +558 drivers/vfio/platform/vfio_platform_common.c
552 };
553
554 #ifdef CONFIG_ACPI
555 int vfio_platform_probe_acpi(struct vfio_platform_device *vdev,
556 struct device *dev)
557 {
> 558 struct acpi_device adev = ACPI_COMPANION(dev);
559
> 560 if (!adev)
561 return -EINVAL;
562
> 563 vdev->acpihid = acpi_device_hid(adev);
564 if (!vdev->acpihid) {
565 pr_err("VFIO: cannot find ACPI HID for %s\n",
566 vdev->name);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Eric, Alex, Antonios; On 1/29/2016 5: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. > > 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. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > .../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 <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; > @@ -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 */ > If we put aside the forgotten pointer in acpi_device assignment, (I had a merge conflict and I screwed it up at the last moment), Can you review the following two patches? https://lkml.org/lkml/2016/1/29/679 https://lkml.org/lkml/2016/1/29/677 These patches seem to be in your area. I was relying on the maintainer list to pull you into the review but for some reason your emails didn't show up. Sinan
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 <okaya@codeaurora.org> > --- > .../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 <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; > @@ -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 */ >
On 2/1/2016 11:08 AM, Eric Auger wrote: >> 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? Old commit message. I did an internal review before posting the patch. The first version of the patch was a hack. I simplified the code but forgot to update the commit message. Now, the rule is simple. - ACPI HID needs match for ACPI systems - DT compat needs to match for OF systems as expected. I'll rephrase for the next version. >> > >> > 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. Thanks, we should ask Baptiste to get his email into the Maintainer file list. > > On my side I tested with of amd xgbe and I don't observe any regression. > > Best Regards Can I add your tested by?
On 02/01/2016 05:16 PM, Sinan Kaya wrote: > On 2/1/2016 11:08 AM, Eric Auger wrote: >>> 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? > > Old commit message. I did an internal review before posting the patch. The first > version of the patch was a hack. I simplified the code but forgot to update the > commit message. > > Now, the rule is simple. > > - ACPI HID needs match for ACPI systems > - DT compat needs to match for OF systems > > as expected. I'll rephrase for the next version. > >>>> >>>> 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. > > Thanks, we should ask Baptiste to get his email into the Maintainer file list. I think it is: ./scripts/get_maintainer.pl 0001-vfio-platform-add-support-for-ACPI-while-detecting-t.patch Baptiste Reynal <b.reynal@virtualopensystems.com> (maintainer:VFIO PLATFORM DRIVER,commit_signer:1/5=20%) Alex Williamson <alex.williamson@redhat.com> (maintainer:VFIO DRIVER,commit_signer:2/3=67%,commit_signer:4/5=80%) ../.. > >> >> On my side I tested with of amd xgbe and I don't observe any regression. >> >> Best Regards > > Can I add your tested by? Well to make things clear I did not test the ACPI part. I just can tell this does not bring any regression on the of part. But I am not against if you don't find anybody else and you tested the ACPI part ;-) Best Regards Eric >
On 2/1/2016 11:29 AM, Eric Auger wrote: >> hanks, we should ask Baptiste to get his email into the Maintainer file list. > I think it is: > ./scripts/get_maintainer.pl > 0001-vfio-platform-add-support-for-ACPI-while-detecting-t.patch > > Baptiste Reynal <b.reynal@virtualopensystems.com> (maintainer:VFIO > PLATFORM DRIVER,commit_signer:1/5=20%) > Alex Williamson <alex.williamson@redhat.com> (maintainer:VFIO > DRIVER,commit_signer:2/3=67%,commit_signer:4/5=80%) > ../.. Strange, I'll reset my tree. >> > >>> >> >>> >> On my side I tested with of amd xgbe and I don't observe any regression. >>> >> >>> >> Best Regards >> > >> > Can I add your tested by? > Well to make things clear I did not test the ACPI part. I just can tell > this does not bring any regression on the of part. But I am not against > if you don't find anybody else and you tested the ACPI part ;-) > How about? of-tested-by: ... Shanker and I tested ACPI before pushing the patches to the list and we also posted the corresponding QEMU patches as well to the qemu-devel maillist. http://patchwork.ozlabs.org/patch/575878/ I'll ask him to add his acpi-tested-by for the ACPI part. > Best Regards
Sinan Kaya wrote: > of-tested-by: ... > > Shanker and I tested ACPI before pushing the patches to the list and we also > posted the corresponding QEMU patches as well to the qemu-devel maillist. > > http://patchwork.ozlabs.org/patch/575878/ > > I'll ask him to add his acpi-tested-by for the ACPI part. I would rather not see a tested-by variant, and instead add comments to the line, like this: Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only) Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) That way, people can still grep for "^Tested-by:".
On 2/1/2016 11:49 AM, Timur Tabi wrote: > Sinan Kaya wrote: >> of-tested-by: ... >> >> Shanker and I tested ACPI before pushing the patches to the list and we also >> posted the corresponding QEMU patches as well to the qemu-devel maillist. >> >> http://patchwork.ozlabs.org/patch/575878/ >> >> I'll ask him to add his acpi-tested-by for the ACPI part. > > I would rather not see a tested-by variant, and instead add comments to the line, like this: > > Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only) > Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only) > > That way, people can still grep for "^Tested-by:". > will do.
Hi Baptiste, Alex; On 2/1/2016 11:29 AM, Eric Auger wrote: >>> I added Baptiste who is vfio platform driver sub-system maintainer. >> > >> > Thanks, we should ask Baptiste to get his email into the Maintainer file list. > I think it is: > ./scripts/get_maintainer.pl > 0001-vfio-platform-add-support-for-ACPI-while-detecting-t.patch Any comments on these two reviews? https://lkml.org/lkml/2016/1/29/679 https://lkml.org/lkml/2016/1/29/677 Sinan
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 <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; @@ -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 */
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. 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. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- .../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(-)