Re: [PATCH v2 2/9] vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind
On Fri, 8 May 2020 10:20:32 +0300 Diana Craciun wrote: > The DPRC (Data Path Resource Container) device is a bus device and has > child devices attached to it. When the vfio-fsl-mc driver is probed > the DPRC is scanned and the child devices discovered and initialized. > > Signed-off-by: Bharat Bhushan > Signed-off-by: Diana Craciun > --- > drivers/vfio/fsl-mc/vfio_fsl_mc.c | 106 ++ > drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 1 + > 2 files changed, 107 insertions(+) > > diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c > b/drivers/vfio/fsl-mc/vfio_fsl_mc.c > index 8b53c2a25b32..ea301ba81225 100644 > --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c > +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c > @@ -15,6 +15,8 @@ > > #include "vfio_fsl_mc_private.h" > > +static struct fsl_mc_driver vfio_fsl_mc_driver; > + > static int vfio_fsl_mc_open(void *device_data) > { > if (!try_module_get(THIS_MODULE)) > @@ -84,6 +86,69 @@ static const struct vfio_device_ops vfio_fsl_mc_ops = { > .mmap = vfio_fsl_mc_mmap, > }; > > +static int vfio_fsl_mc_bus_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct vfio_fsl_mc_device *vdev = container_of(nb, > + struct vfio_fsl_mc_device, nb); > + struct device *dev = data; > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + struct fsl_mc_device *mc_cont = to_fsl_mc_device(mc_dev->dev.parent); > + > + if (action == BUS_NOTIFY_ADD_DEVICE && > + vdev->mc_dev == mc_cont) { > + mc_dev->driver_override = kasprintf(GFP_KERNEL, "%s", > + vfio_fsl_mc_ops.name); > + dev_info(dev, "Setting driver override for device in dprc %s\n", > + dev_name(_cont->dev)); > + } else if (action == BUS_NOTIFY_BOUND_DRIVER && > + vdev->mc_dev == mc_cont) { > + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(dev->driver); > + > + if (mc_drv && mc_drv != _fsl_mc_driver) > + dev_warn(dev, "Object %s bound to driver %s while DPRC > bound to vfio-fsl-mc\n", > + dev_name(dev), mc_drv->driver.name); > + } > + > + return 0; > +} > + > +static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev) > +{ > + struct fsl_mc_device *mc_dev = vdev->mc_dev; > + int ret = 0; > + > + /* Non-dprc devices share mc_io from parent */ > + if (!is_fsl_mc_bus_dprc(mc_dev)) { > + struct fsl_mc_device *mc_cont = > to_fsl_mc_device(mc_dev->dev.parent); > + > + mc_dev->mc_io = mc_cont->mc_io; > + return 0; > + } > + > + vdev->nb.notifier_call = vfio_fsl_mc_bus_notifier; > + ret = bus_register_notifier(_mc_bus_type, >nb); > + if (ret) > + return ret; > + > + /* open DPRC, allocate a MC portal */ > + ret = dprc_setup(mc_dev); > + if (ret < 0) { > + dev_err(_dev->dev, "Failed to setup DPRC (error = %d)\n", > ret); > + bus_unregister_notifier(_mc_bus_type, >nb); > + return ret; > + } > + > + ret = dprc_scan_container(mc_dev, false); > + if (ret < 0) { > + dev_err(_dev->dev, "Container scanning failed: %d\n", ret); > + bus_unregister_notifier(_mc_bus_type, >nb); > + dprc_cleanup(mc_dev); > + } > + > + return 0; The last error branch falls through, did you intend to return 'ret' here to capture that? Also, nit, ret doesn't need to be initialized. > +} > + > static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) > { > struct iommu_group *group; > @@ -112,9 +177,42 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device > *mc_dev) > return ret; > } > > + ret = vfio_fsl_mc_init_device(vdev); > + if (ret) { > + vfio_iommu_group_put(group, dev); > + return ret; > + } The error condition value is a bit inconsistent between vfio_fs_mc_init_device() and here, <0 vs !0. Thanks, Alex > + > return ret; > } > > +static int vfio_fsl_mc_device_remove(struct device *dev, void *data) > +{ > + struct fsl_mc_device *mc_dev; > + > + WARN_ON(!dev); > + mc_dev = to_fsl_mc_device(dev); > + if (WARN_ON(!mc_dev)) > + return -ENODEV; > + > + kfree(mc_dev->driver_override); > + mc_dev->driver_override = NULL; > + > + /* > + * The device-specific remove callback will get invoked by device_del() > + */ > + device_del(_dev->dev); > + put_device(_dev->dev); > + > + return 0; > +} > + > +static void vfio_fsl_mc_cleanup_dprc(struct fsl_mc_device *mc_dev) > +{ > + device_for_each_child(_dev->dev, NULL, vfio_fsl_mc_device_remove); > + dprc_cleanup(mc_dev); > +} > + > static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev) > { > struct
Re: [PATCH v2 2/9] vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind
Hi Diana, I love your patch! Yet something to improve: [auto build test ERROR on vfio/next] [also build test ERROR on linus/master v5.7-rc4 next-20200508] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Diana-Craciun/vfio-fsl-mc-VFIO-support-for-FSL-MC-devices/20200509-034845 base: https://github.com/awilliam/linux-vfio.git next config: i386-allyesconfig (attached as .config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/vfio/fsl-mc/vfio_fsl_mc.c: In function 'vfio_fsl_mc_bus_notifier': >> drivers/vfio/fsl-mc/vfio_fsl_mc.c:100:9: error: 'struct fsl_mc_device' has >> no member named 'driver_override' mc_dev->driver_override = kasprintf(GFP_KERNEL, "%s", ^~ drivers/vfio/fsl-mc/vfio_fsl_mc.c: In function 'vfio_fsl_mc_init_device': >> drivers/vfio/fsl-mc/vfio_fsl_mc.c:135:8: error: implicit declaration of >> function 'dprc_setup'; did you mean 'x2apic_setup'? >> [-Werror=implicit-function-declaration] ret = dprc_setup(mc_dev); ^~ x2apic_setup >> drivers/vfio/fsl-mc/vfio_fsl_mc.c:142:8: error: implicit declaration of >> function 'dprc_scan_container'; did you mean 'init_section_contains'? >> [-Werror=implicit-function-declaration] ret = dprc_scan_container(mc_dev, false); ^~~ init_section_contains >> drivers/vfio/fsl-mc/vfio_fsl_mc.c:146:3: error: implicit declaration of >> function 'dprc_cleanup'; did you mean 'pud_clear'? >> [-Werror=implicit-function-declaration] dprc_cleanup(mc_dev); ^~~~ pud_clear drivers/vfio/fsl-mc/vfio_fsl_mc.c: In function 'vfio_fsl_mc_device_remove': drivers/vfio/fsl-mc/vfio_fsl_mc.c:198:14: error: 'struct fsl_mc_device' has no member named 'driver_override' kfree(mc_dev->driver_override); ^~ drivers/vfio/fsl-mc/vfio_fsl_mc.c:199:8: error: 'struct fsl_mc_device' has no member named 'driver_override' mc_dev->driver_override = NULL; ^~ cc1: some warnings being treated as errors vim +100 drivers/vfio/fsl-mc/vfio_fsl_mc.c 88 89 static int vfio_fsl_mc_bus_notifier(struct notifier_block *nb, 90 unsigned long action, void *data) 91 { 92 struct vfio_fsl_mc_device *vdev = container_of(nb, 93 struct vfio_fsl_mc_device, nb); 94 struct device *dev = data; 95 struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); 96 struct fsl_mc_device *mc_cont = to_fsl_mc_device(mc_dev->dev.parent); 97 98 if (action == BUS_NOTIFY_ADD_DEVICE && 99 vdev->mc_dev == mc_cont) { > 100 mc_dev->driver_override = kasprintf(GFP_KERNEL, "%s", 101 vfio_fsl_mc_ops.name); 102 dev_info(dev, "Setting driver override for device in dprc %s\n", 103 dev_name(_cont->dev)); 104 } else if (action == BUS_NOTIFY_BOUND_DRIVER && 105 vdev->mc_dev == mc_cont) { 106 struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(dev->driver); 107 108 if (mc_drv && mc_drv != _fsl_mc_driver) 109 dev_warn(dev, "Object %s bound to driver %s while DPRC bound to vfio-fsl-mc\n", 110 dev_name(dev), mc_drv->driver.name); 111 } 112 113 return 0; 114 } 115 116 static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev) 117 { 118 struct fsl_mc_device *mc_dev = vdev->mc_dev; 119 int ret = 0; 120 121 /* Non-dprc devices share mc_io from parent */ 122 if (!is_fsl_mc_bus_dprc(mc_dev)) { 123 struct fsl_mc_device *mc_cont = to_fsl_mc_device(mc_dev->dev.parent); 124 125 mc_dev->mc_io = mc_cont->mc_io; 126 return 0; 127 } 128 129 vdev->nb.notifier_call = vfio_fsl_mc_bus_notifier; 130 ret = bus_register_notifier(_mc_bus_type, >nb); 131 if (ret) 132 return ret; 133 134 /* open DPRC, allocate a MC portal */ > 135 ret = dprc_setup(mc_dev); 136 if (ret < 0) { 137 dev_err(_dev->dev, "Failed to setup DPRC (error = %d)\n", ret); 138
[PATCH v2 2/9] vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind
The DPRC (Data Path Resource Container) device is a bus device and has child devices attached to it. When the vfio-fsl-mc driver is probed the DPRC is scanned and the child devices discovered and initialized. Signed-off-by: Bharat Bhushan Signed-off-by: Diana Craciun --- drivers/vfio/fsl-mc/vfio_fsl_mc.c | 106 ++ drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 1 + 2 files changed, 107 insertions(+) diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c index 8b53c2a25b32..ea301ba81225 100644 --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c @@ -15,6 +15,8 @@ #include "vfio_fsl_mc_private.h" +static struct fsl_mc_driver vfio_fsl_mc_driver; + static int vfio_fsl_mc_open(void *device_data) { if (!try_module_get(THIS_MODULE)) @@ -84,6 +86,69 @@ static const struct vfio_device_ops vfio_fsl_mc_ops = { .mmap = vfio_fsl_mc_mmap, }; +static int vfio_fsl_mc_bus_notifier(struct notifier_block *nb, + unsigned long action, void *data) +{ + struct vfio_fsl_mc_device *vdev = container_of(nb, + struct vfio_fsl_mc_device, nb); + struct device *dev = data; + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); + struct fsl_mc_device *mc_cont = to_fsl_mc_device(mc_dev->dev.parent); + + if (action == BUS_NOTIFY_ADD_DEVICE && + vdev->mc_dev == mc_cont) { + mc_dev->driver_override = kasprintf(GFP_KERNEL, "%s", + vfio_fsl_mc_ops.name); + dev_info(dev, "Setting driver override for device in dprc %s\n", +dev_name(_cont->dev)); + } else if (action == BUS_NOTIFY_BOUND_DRIVER && + vdev->mc_dev == mc_cont) { + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(dev->driver); + + if (mc_drv && mc_drv != _fsl_mc_driver) + dev_warn(dev, "Object %s bound to driver %s while DPRC bound to vfio-fsl-mc\n", +dev_name(dev), mc_drv->driver.name); + } + + return 0; +} + +static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev) +{ + struct fsl_mc_device *mc_dev = vdev->mc_dev; + int ret = 0; + + /* Non-dprc devices share mc_io from parent */ + if (!is_fsl_mc_bus_dprc(mc_dev)) { + struct fsl_mc_device *mc_cont = to_fsl_mc_device(mc_dev->dev.parent); + + mc_dev->mc_io = mc_cont->mc_io; + return 0; + } + + vdev->nb.notifier_call = vfio_fsl_mc_bus_notifier; + ret = bus_register_notifier(_mc_bus_type, >nb); + if (ret) + return ret; + + /* open DPRC, allocate a MC portal */ + ret = dprc_setup(mc_dev); + if (ret < 0) { + dev_err(_dev->dev, "Failed to setup DPRC (error = %d)\n", ret); + bus_unregister_notifier(_mc_bus_type, >nb); + return ret; + } + + ret = dprc_scan_container(mc_dev, false); + if (ret < 0) { + dev_err(_dev->dev, "Container scanning failed: %d\n", ret); + bus_unregister_notifier(_mc_bus_type, >nb); + dprc_cleanup(mc_dev); + } + + return 0; +} + static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) { struct iommu_group *group; @@ -112,9 +177,42 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) return ret; } + ret = vfio_fsl_mc_init_device(vdev); + if (ret) { + vfio_iommu_group_put(group, dev); + return ret; + } + return ret; } +static int vfio_fsl_mc_device_remove(struct device *dev, void *data) +{ + struct fsl_mc_device *mc_dev; + + WARN_ON(!dev); + mc_dev = to_fsl_mc_device(dev); + if (WARN_ON(!mc_dev)) + return -ENODEV; + + kfree(mc_dev->driver_override); + mc_dev->driver_override = NULL; + + /* +* The device-specific remove callback will get invoked by device_del() +*/ + device_del(_dev->dev); + put_device(_dev->dev); + + return 0; +} + +static void vfio_fsl_mc_cleanup_dprc(struct fsl_mc_device *mc_dev) +{ + device_for_each_child(_dev->dev, NULL, vfio_fsl_mc_device_remove); + dprc_cleanup(mc_dev); +} + static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev) { struct vfio_fsl_mc_device *vdev; @@ -124,6 +222,14 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev) if (!vdev) return -EINVAL; + if (vdev->nb.notifier_call) + bus_unregister_notifier(_mc_bus_type, >nb); + + if (is_fsl_mc_bus_dprc(mc_dev)) + vfio_fsl_mc_cleanup_dprc(vdev->mc_dev); + + mc_dev->mc_io = NULL; + vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);