| Message ID | 20211206015903.88687-5-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
On Mon, Dec 06, 2021 at 09:58:49AM +0800, Lu Baolu wrote: > Multiple platform devices may be placed in the same IOMMU group because > they cannot be isolated from each other. These devices must either be > entirely under kernel control or userspace control, never a mixture. This > checks and sets DMA ownership during driver binding, and release the > ownership during driver unbinding. > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, > the userspace framework drivers (vfio etc.) which need to manually claim > DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/platform_device.h | 1 + > drivers/base/platform.c | 30 +++++++++++++++++++++++++++++- > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 4381c34af7e0..f3926be7582f 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -210,6 +210,7 @@ struct platform_driver { > struct device_driver driver; > const struct platform_device_id *id_table; > bool prevent_deferred_probe; > + bool suppress_auto_claim_dma_owner; We now have "prevent_" and "suppress_" as prefixes. Why the difference? What is wrong with "prevent_" for your new flag? thanks, greg k-h
I really hate the amount of boilerplate code that having this in each bus type causes. Between that and the suggestion from Joerg I wonder if we could do the following again: - add new no_kernel_dma flag to struct device_driver - set this flag for the various vfio drivers - skip claiming the kernel dma ownership for those (or rather release it if the suggestion from Joerg works out)
On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: > I really hate the amount of boilerplate code that having this in each > bus type causes. +1 I liked the first version of this series better with the code near really_probe(). Can we go back to that with some device_configure_dma() wrapper condtionally called by really_probe as we discussed? > Between that and the suggestion from Joerg I wonder if we could do the > following again: > > - add new no_kernel_dma flag to struct device_driver > - set this flag for the various vfio drivers > - skip claiming the kernel dma ownership for those (or rather release > it if the suggestion from Joerg works out) v1 did exactly this. Jason
On 12/6/21 11:06 PM, Jason Gunthorpe wrote: > On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: >> I really hate the amount of boilerplate code that having this in each >> bus type causes. > +1 > > I liked the first version of this series better with the code near > really_probe(). > > Can we go back to that with some device_configure_dma() wrapper > condtionally called by really_probe as we discussed? > Are you talking about below change? diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..368f9e530515 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus->dma_configure) { ret = dev->bus->dma_configure(dev); if (ret) - goto probe_failed; + goto pinctrl_bind_failed; + + if (!drv->no_kernel_dma) { + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); + if (ret) + goto pinctrl_bind_failed; + } } ret = driver_sysfs_add(dev); @@ -660,6 +666,9 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DRIVER_NOT_BOUND, dev); + + if (dev->bus->dma_configure && !drv->no_kernel_dma) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); pinctrl_bind_failed: device_links_no_driver(dev); devres_release_all(dev); @@ -1204,6 +1213,9 @@ static void __device_release_driver(struct device *dev, struct device *parent) else if (drv->remove) drv->remove(dev); + if (dev->bus->dma_configure && !drv->no_kernel_dma) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); + device_links_driver_cleanup(dev); devres_release_all(dev); diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index a498ebcf4993..2cf7b757b28e 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -100,6 +100,7 @@ struct device_driver { const char *mod_name; /* used for built-in modules */ bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ + bool no_kernel_dma; enum probe_type probe_type; const struct of_device_id *of_match_table; Best regards, baolu
On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote: > On 12/6/21 11:06 PM, Jason Gunthorpe wrote: > > On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: > > > I really hate the amount of boilerplate code that having this in each > > > bus type causes. > > +1 > > > > I liked the first version of this series better with the code near > > really_probe(). > > > > Can we go back to that with some device_configure_dma() wrapper > > condtionally called by really_probe as we discussed? > > > > Are you talking about below change? > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 68ea1f949daa..368f9e530515 100644 > +++ b/drivers/base/dd.c > @@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct > device_driver *drv) > if (dev->bus->dma_configure) { > ret = dev->bus->dma_configure(dev); > if (ret) > - goto probe_failed; > + goto pinctrl_bind_failed; > + > + if (!drv->no_kernel_dma) { > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); > + if (ret) > + goto pinctrl_bind_failed; > + } > } Yes, the suggestion was to put everything that 'if' inside a function and then of course a matching undo function. Jason
On Tue, Dec 07, 2021 at 09:16:27AM -0400, Jason Gunthorpe wrote: > Yes, the suggestion was to put everything that 'if' inside a function > and then of course a matching undo function. Can't we simplify things even more? Do away with the DMA API owner entirely, and instead in iommu_group_set_dma_owner iterate over all devices in a group and check that they all have the no_dma_api flag set (plus a similar check on group join). With that most of the boilerplate code goes away entirely in favor of a little more work at iommu_group_set_dma_owner time.
On Tue, Dec 07, 2021 at 05:25:04AM -0800, Christoph Hellwig wrote: > On Tue, Dec 07, 2021 at 09:16:27AM -0400, Jason Gunthorpe wrote: > > Yes, the suggestion was to put everything that 'if' inside a function > > and then of course a matching undo function. > > Can't we simplify things even more? Do away with the DMA API owner > entirely, and instead in iommu_group_set_dma_owner iterate over all > devices in a group and check that they all have the no_dma_api flag > set (plus a similar check on group join). With that most of the > boilerplate code goes away entirely in favor of a little more work at > iommu_group_set_dma_owner time. Robin suggested something like this already. The locking doesn't work out, we can't nest device_lock()'s safely without ABBA deadlocks, and can't touch the dev->driver without the device_lock. Jason
On 12/7/21 9:16 PM, Jason Gunthorpe wrote: > On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote: >> On 12/6/21 11:06 PM, Jason Gunthorpe wrote: >>> On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: >>>> I really hate the amount of boilerplate code that having this in each >>>> bus type causes. >>> +1 >>> >>> I liked the first version of this series better with the code near >>> really_probe(). >>> >>> Can we go back to that with some device_configure_dma() wrapper >>> condtionally called by really_probe as we discussed? >>> >> >> Are you talking about below change? >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index 68ea1f949daa..368f9e530515 100644 >> +++ b/drivers/base/dd.c >> @@ -577,7 +577,13 @@ static int really_probe(struct device *dev, struct >> device_driver *drv) >> if (dev->bus->dma_configure) { >> ret = dev->bus->dma_configure(dev); >> if (ret) >> - goto probe_failed; >> + goto pinctrl_bind_failed; >> + >> + if (!drv->no_kernel_dma) { >> + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); >> + if (ret) >> + goto pinctrl_bind_failed; >> + } >> } > > Yes, the suggestion was to put everything that 'if' inside a function > and then of course a matching undo function. Followed your suggestion, I refactored the change like below: diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..68ca5a579eb1 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, struct device_driver *drv) return ret; } +static int device_dma_configure(struct device *dev, struct device_driver *drv) +{ + int ret; + + if (!dev->bus->dma_configure) + return 0; + + ret = dev->bus->dma_configure(dev); + if (ret) + return ret; + + if (!drv->suppress_auto_claim_dma_owner) + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); + + return ret; +} + +static void device_dma_cleanup(struct device *dev, struct device_driver *drv) +{ + if (!dev->bus->dma_configure) + return; + + if (!drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API, NULL); +} + static int really_probe(struct device *dev, struct device_driver *drv) { bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) && @@ -574,11 +600,8 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (ret) goto pinctrl_bind_failed; - if (dev->bus->dma_configure) { - ret = dev->bus->dma_configure(dev); - if (ret) - goto probe_failed; - } + if (device_dma_configure(dev, drv)) + goto pinctrl_bind_failed; ret = driver_sysfs_add(dev); if (ret) { @@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DRIVER_NOT_BOUND, dev); + + device_dma_cleanup(dev, drv); pinctrl_bind_failed: device_links_no_driver(dev); devres_release_all(dev); @@ -1204,6 +1229,7 @@ static void __device_release_driver(struct device *dev, struct device *parent) else if (drv->remove) drv->remove(dev); + device_dma_cleanup(dev, drv); device_links_driver_cleanup(dev); devres_release_all(dev); diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index a498ebcf4993..374a3c2cc10d 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -100,6 +100,7 @@ struct device_driver { const char *mod_name; /* used for built-in modules */ bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ + bool suppress_auto_claim_dma_owner; enum probe_type probe_type; const struct of_device_id *of_match_table; Further suggestions? Best regards, baolu
Hi Greg, Jason and Christoph, On 12/9/21 9:20 AM, Lu Baolu wrote: > On 12/7/21 9:16 PM, Jason Gunthorpe wrote: >> On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote: >>> On 12/6/21 11:06 PM, Jason Gunthorpe wrote: >>>> On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: >>>>> I really hate the amount of boilerplate code that having this in each >>>>> bus type causes. >>>> +1 >>>> >>>> I liked the first version of this series better with the code near >>>> really_probe(). >>>> >>>> Can we go back to that with some device_configure_dma() wrapper >>>> condtionally called by really_probe as we discussed? [...] > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 68ea1f949daa..68ca5a579eb1 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, > struct device_driver *drv) > return ret; > } > > +static int device_dma_configure(struct device *dev, struct > device_driver *drv) > +{ > + int ret; > + > + if (!dev->bus->dma_configure) > + return 0; > + > + ret = dev->bus->dma_configure(dev); > + if (ret) > + return ret; > + > + if (!drv->suppress_auto_claim_dma_owner) > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, > NULL); > + > + return ret; > +} > + > +static void device_dma_cleanup(struct device *dev, struct device_driver > *drv) > +{ > + if (!dev->bus->dma_configure) > + return; > + > + if (!drv->suppress_auto_claim_dma_owner) > + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API, > NULL); > +} > + > static int really_probe(struct device *dev, struct device_driver *drv) > { > bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) && > @@ -574,11 +600,8 @@ static int really_probe(struct device *dev, struct > device_driver *drv) > if (ret) > goto pinctrl_bind_failed; > > - if (dev->bus->dma_configure) { > - ret = dev->bus->dma_configure(dev); > - if (ret) > - goto probe_failed; > - } > + if (device_dma_configure(dev, drv)) > + goto pinctrl_bind_failed; > > ret = driver_sysfs_add(dev); > if (ret) { > @@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct > device_driver *drv) > if (dev->bus) > blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > > BUS_NOTIFY_DRIVER_NOT_BOUND, dev); > + > + device_dma_cleanup(dev, drv); > pinctrl_bind_failed: > device_links_no_driver(dev); > devres_release_all(dev); > @@ -1204,6 +1229,7 @@ static void __device_release_driver(struct device > *dev, struct device *parent) > else if (drv->remove) > drv->remove(dev); > > + device_dma_cleanup(dev, drv); > device_links_driver_cleanup(dev); > > devres_release_all(dev); > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index a498ebcf4993..374a3c2cc10d 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -100,6 +100,7 @@ struct device_driver { > const char *mod_name; /* used for built-in > modules */ > > bool suppress_bind_attrs; /* disables bind/unbind via > sysfs */ > + bool suppress_auto_claim_dma_owner; > enum probe_type probe_type; > > const struct of_device_id *of_match_table; Does this work for you? Can I work towards this in the next version? Best regards, baolu
On 12/10/21 9:23 AM, Lu Baolu wrote: > Hi Greg, Jason and Christoph, > > On 12/9/21 9:20 AM, Lu Baolu wrote: >> On 12/7/21 9:16 PM, Jason Gunthorpe wrote: >>> On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote: >>>> On 12/6/21 11:06 PM, Jason Gunthorpe wrote: >>>>> On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: >>>>>> I really hate the amount of boilerplate code that having this in each >>>>>> bus type causes. >>>>> +1 >>>>> >>>>> I liked the first version of this series better with the code near >>>>> really_probe(). >>>>> >>>>> Can we go back to that with some device_configure_dma() wrapper >>>>> condtionally called by really_probe as we discussed? > > [...] > >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index 68ea1f949daa..68ca5a579eb1 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, >> struct device_driver *drv) >> return ret; >> } >> >> +static int device_dma_configure(struct device *dev, struct >> device_driver *drv) >> +{ >> + int ret; >> + >> + if (!dev->bus->dma_configure) >> + return 0; >> + >> + ret = dev->bus->dma_configure(dev); >> + if (ret) >> + return ret; >> + >> + if (!drv->suppress_auto_claim_dma_owner) >> + ret = iommu_device_set_dma_owner(dev, >> DMA_OWNER_DMA_API, NULL); >> + >> + return ret; >> +} >> + >> +static void device_dma_cleanup(struct device *dev, struct >> device_driver *drv) >> +{ >> + if (!dev->bus->dma_configure) >> + return; >> + >> + if (!drv->suppress_auto_claim_dma_owner) >> + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); >> +} >> + >> static int really_probe(struct device *dev, struct device_driver *drv) >> { >> bool test_remove = >> IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) && >> @@ -574,11 +600,8 @@ static int really_probe(struct device *dev, >> struct device_driver *drv) >> if (ret) >> goto pinctrl_bind_failed; >> >> - if (dev->bus->dma_configure) { >> - ret = dev->bus->dma_configure(dev); >> - if (ret) >> - goto probe_failed; >> - } >> + if (device_dma_configure(dev, drv)) >> + goto pinctrl_bind_failed; >> >> ret = driver_sysfs_add(dev); >> if (ret) { >> @@ -660,6 +683,8 @@ static int really_probe(struct device *dev, struct >> device_driver *drv) >> if (dev->bus) >> blocking_notifier_call_chain(&dev->bus->p->bus_notifier, >> >> BUS_NOTIFY_DRIVER_NOT_BOUND, dev); >> + >> + device_dma_cleanup(dev, drv); >> pinctrl_bind_failed: >> device_links_no_driver(dev); >> devres_release_all(dev); >> @@ -1204,6 +1229,7 @@ static void __device_release_driver(struct >> device *dev, struct device *parent) >> else if (drv->remove) >> drv->remove(dev); >> >> + device_dma_cleanup(dev, drv); >> device_links_driver_cleanup(dev); >> >> devres_release_all(dev); >> diff --git a/include/linux/device/driver.h >> b/include/linux/device/driver.h >> index a498ebcf4993..374a3c2cc10d 100644 >> --- a/include/linux/device/driver.h >> +++ b/include/linux/device/driver.h >> @@ -100,6 +100,7 @@ struct device_driver { >> const char *mod_name; /* used for built-in >> modules */ >> >> bool suppress_bind_attrs; /* disables bind/unbind via >> sysfs */ >> + bool suppress_auto_claim_dma_owner; >> enum probe_type probe_type; >> >> const struct of_device_id *of_match_table; > > Does this work for you? Can I work towards this in the next version? A kindly ping ... Is this heading the right direction? I need your advice to move ahead. :-) Best regards, baolu
On Mon, Dec 13, 2021 at 08:50:05AM +0800, Lu Baolu wrote: > > Does this work for you? Can I work towards this in the next version? > > A kindly ping ... Is this heading the right direction? I need your > advice to move ahead. :-) I prefer this to all the duplicated code in the v3 series.. Given it touches almost every bus type that implements the dma_configure it seems appropriate. Jason
This approach looks much better to me.
Hi Greg, On 2021/12/13 8:50, Lu Baolu wrote: > On 12/10/21 9:23 AM, Lu Baolu wrote: >> Hi Greg, Jason and Christoph, >> >> On 12/9/21 9:20 AM, Lu Baolu wrote: >>> On 12/7/21 9:16 PM, Jason Gunthorpe wrote: >>>> On Tue, Dec 07, 2021 at 10:57:25AM +0800, Lu Baolu wrote: >>>>> On 12/6/21 11:06 PM, Jason Gunthorpe wrote: >>>>>> On Mon, Dec 06, 2021 at 06:36:27AM -0800, Christoph Hellwig wrote: >>>>>>> I really hate the amount of boilerplate code that having this in >>>>>>> each >>>>>>> bus type causes. >>>>>> +1 >>>>>> >>>>>> I liked the first version of this series better with the code near >>>>>> really_probe(). >>>>>> >>>>>> Can we go back to that with some device_configure_dma() wrapper >>>>>> condtionally called by really_probe as we discussed? >> >> [...] >> >>> >>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >>> index 68ea1f949daa..68ca5a579eb1 100644 >>> --- a/drivers/base/dd.c >>> +++ b/drivers/base/dd.c >>> @@ -538,6 +538,32 @@ static int call_driver_probe(struct device *dev, >>> struct device_driver *drv) >>> return ret; >>> } >>> >>> +static int device_dma_configure(struct device *dev, struct >>> device_driver *drv) >>> +{ >>> + int ret; >>> + >>> + if (!dev->bus->dma_configure) >>> + return 0; >>> + >>> + ret = dev->bus->dma_configure(dev); >>> + if (ret) >>> + return ret; >>> + >>> + if (!drv->suppress_auto_claim_dma_owner) >>> + ret = iommu_device_set_dma_owner(dev, >>> DMA_OWNER_DMA_API, NULL); >>> + >>> + return ret; >>> +} >>> + >>> +static void device_dma_cleanup(struct device *dev, struct >>> device_driver *drv) >>> +{ >>> + if (!dev->bus->dma_configure) >>> + return; >>> + >>> + if (!drv->suppress_auto_claim_dma_owner) >>> + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); >>> +} >>> + >>> static int really_probe(struct device *dev, struct device_driver *drv) >>> { >>> bool test_remove = >>> IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) && >>> @@ -574,11 +600,8 @@ static int really_probe(struct device *dev, >>> struct device_driver *drv) >>> if (ret) >>> goto pinctrl_bind_failed; >>> >>> - if (dev->bus->dma_configure) { >>> - ret = dev->bus->dma_configure(dev); >>> - if (ret) >>> - goto probe_failed; >>> - } >>> + if (device_dma_configure(dev, drv)) >>> + goto pinctrl_bind_failed; >>> >>> ret = driver_sysfs_add(dev); >>> if (ret) { >>> @@ -660,6 +683,8 @@ static int really_probe(struct device *dev, >>> struct device_driver *drv) >>> if (dev->bus) >>> >>> blocking_notifier_call_chain(&dev->bus->p->bus_notifier, >>> >>> BUS_NOTIFY_DRIVER_NOT_BOUND, dev); >>> + >>> + device_dma_cleanup(dev, drv); >>> pinctrl_bind_failed: >>> device_links_no_driver(dev); >>> devres_release_all(dev); >>> @@ -1204,6 +1229,7 @@ static void __device_release_driver(struct >>> device *dev, struct device *parent) >>> else if (drv->remove) >>> drv->remove(dev); >>> >>> + device_dma_cleanup(dev, drv); >>> device_links_driver_cleanup(dev); >>> >>> devres_release_all(dev); >>> diff --git a/include/linux/device/driver.h >>> b/include/linux/device/driver.h >>> index a498ebcf4993..374a3c2cc10d 100644 >>> --- a/include/linux/device/driver.h >>> +++ b/include/linux/device/driver.h >>> @@ -100,6 +100,7 @@ struct device_driver { >>> const char *mod_name; /* used for built-in >>> modules */ >>> >>> bool suppress_bind_attrs; /* disables bind/unbind via >>> sysfs */ >>> + bool suppress_auto_claim_dma_owner; >>> enum probe_type probe_type; >>> >>> const struct of_device_id *of_match_table; >> >> Does this work for you? Can I work towards this in the next version? > > A kindly ping ... Is this heading the right direction? I need your > advice to move ahead. :-) Can I do it like this? :-) Best regards, baolu
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 4381c34af7e0..f3926be7582f 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -210,6 +210,7 @@ struct platform_driver { struct device_driver driver; const struct platform_device_id *id_table; bool prevent_deferred_probe; + bool suppress_auto_claim_dma_owner; }; #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 125d708c0eb3..032b9f20b468 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -30,6 +30,7 @@ #include <linux/property.h> #include <linux/kmemleak.h> #include <linux/types.h> +#include <linux/iommu.h> #include "base.h" #include "power/power.h" @@ -1464,6 +1465,32 @@ int firmware_dma_configure(struct device *dev) return ret; } +static int platform_dma_configure(struct device *dev) +{ + struct platform_driver *drv = to_platform_driver(dev->driver); + int ret; + + if (!drv->suppress_auto_claim_dma_owner) { + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); + if (ret) + return ret; + } + + ret = firmware_dma_configure(dev); + if (ret && !drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); + + return ret; +} + +static void platform_dma_cleanup(struct device *dev) +{ + struct platform_driver *drv = to_platform_driver(dev->driver); + + if (!drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); +} + static const struct dev_pm_ops platform_dev_pm_ops = { SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL) USE_PLATFORM_PM_SLEEP_OPS @@ -1477,7 +1504,8 @@ struct bus_type platform_bus_type = { .probe = platform_probe, .remove = platform_remove, .shutdown = platform_shutdown, - .dma_configure = firmware_dma_configure, + .dma_configure = platform_dma_configure, + .dma_cleanup = platform_dma_cleanup, .pm = &platform_dev_pm_ops, }; EXPORT_SYMBOL_GPL(platform_bus_type);
Multiple platform devices may be placed in the same IOMMU group because they cannot be isolated from each other. These devices must either be entirely under kernel control or userspace control, never a mixture. This checks and sets DMA ownership during driver binding, and release the ownership during driver unbinding. Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, the userspace framework drivers (vfio etc.) which need to manually claim DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/platform_device.h | 1 + drivers/base/platform.c | 30 +++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)