| Message ID | 8c380309f264cd0dfc73ba2ec060adc9515af2f2.1657034828.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Retire bus_set_iommu() | expand |
On 2022/7/6 01:08, Robin Murphy wrote: > /* > * Use a function instead of an array here because the domain-type is a > * bit-field, so an array would waste memory. > @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) > (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? > "(set via kernel command line)" : ""); > > + /* If the system is so broken that this fails, it will WARN anyway */ Can you please elaborate a bit on this? iommu_bus_init() still return errors. > + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) > + iommu_bus_init(iommu_buses[i]); > + > return 0; Best regards, baolu
On 2022-07-06 02:53, Baolu Lu wrote: > On 2022/7/6 01:08, Robin Murphy wrote: >> /* >> * Use a function instead of an array here because the domain-type is a >> * bit-field, so an array would waste memory. >> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) >> (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? >> "(set via kernel command line)" : ""); >> + /* If the system is so broken that this fails, it will WARN >> anyway */ > > Can you please elaborate a bit on this? iommu_bus_init() still return > errors. Indeed, it's commenting on the fact that we don't try to clean up or propagate an error value further even if it did ever manage to return one. I feared that if I strip the error handling out of iommu_bus_init() itself on the same reasoning, we'll just get constant patches from the static checker brigade trying to add it back, so it seemed like the neatest compromise to keep that decision where it's obviously in an early initcall, rather than in the helper function which can be viewed out of context. However, I'm happy to either expand this comment or go the whole way and make iommu_bus_init() return void if you think it's worthwhile. Cheers, Robin. > >> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) >> + iommu_bus_init(iommu_buses[i]); >> + >> return 0; > > Best regards, > baolu
On 2022/7/6 21:43, Robin Murphy wrote: > On 2022-07-06 02:53, Baolu Lu wrote: >> On 2022/7/6 01:08, Robin Murphy wrote: >>> /* >>> * Use a function instead of an array here because the domain-type >>> is a >>> * bit-field, so an array would waste memory. >>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) >>> (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? >>> "(set via kernel command line)" : ""); >>> + /* If the system is so broken that this fails, it will WARN >>> anyway */ >> >> Can you please elaborate a bit on this? iommu_bus_init() still return >> errors. > > Indeed, it's commenting on the fact that we don't try to clean up or > propagate an error value further even if it did ever manage to return > one. I feared that if I strip the error handling out of iommu_bus_init() > itself on the same reasoning, we'll just get constant patches from the > static checker brigade trying to add it back, so it seemed like the > neatest compromise to keep that decision where it's obviously in an > early initcall, rather than in the helper function which can be viewed > out of context. However, I'm happy to either expand this comment or go > the whole way and make iommu_bus_init() return void if you think it's > worthwhile. Thanks for the explanation. It would be helpful if the comment could be expanded. In this case, after a long time, people will not consider it an oversight. :-) Best regards, baolu > > Cheers, > Robin. > >> >>> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) >>> + iommu_bus_init(iommu_buses[i]); >>> + >>> return 0; >> >> Best regards, >> baolu >
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Wednesday, July 6, 2022 1:08 AM > > The number of bus types that the IOMMU subsystem deals with is small and > manageable, so pull that list into core code as a first step towards > cleaning up all the boilerplate bus-awareness from drivers. Calling > iommu_probe_device() before bus->iommu_ops is set will simply return > -ENODEV and not break the notifier call chain, so there should be no > harm in proactively registering all our bus notifiers at init time. > Suppose we miss a check on iommu ops in iommu_release_device(): if (!dev->iommu) <<<<<<< return; iommu_device_unlink(dev->iommu->iommu_dev, dev); ops = dev_iommu_ops(dev); ops->release_device(dev); following the rationale in patch01 a device could be removed when it's associated with a known but not registered instance.
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Thursday, July 7, 2022 8:21 AM > > On 2022/7/6 21:43, Robin Murphy wrote: > > On 2022-07-06 02:53, Baolu Lu wrote: > >> On 2022/7/6 01:08, Robin Murphy wrote: > >>> /* > >>> * Use a function instead of an array here because the domain-type > >>> is a > >>> * bit-field, so an array would waste memory. > >>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) > >>> (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? > >>> "(set via kernel command line)" : ""); > >>> + /* If the system is so broken that this fails, it will WARN > >>> anyway */ > >> > >> Can you please elaborate a bit on this? iommu_bus_init() still return > >> errors. > > > > Indeed, it's commenting on the fact that we don't try to clean up or > > propagate an error value further even if it did ever manage to return > > one. I feared that if I strip the error handling out of iommu_bus_init() > > itself on the same reasoning, we'll just get constant patches from the > > static checker brigade trying to add it back, so it seemed like the > > neatest compromise to keep that decision where it's obviously in an > > early initcall, rather than in the helper function which can be viewed > > out of context. However, I'm happy to either expand this comment or go > > the whole way and make iommu_bus_init() return void if you think it's > > worthwhile. > > Thanks for the explanation. It would be helpful if the comment could be > expanded. In this case, after a long time, people will not consider it > an oversight. :-) > I'd prefer to making iommu_bus_init() return void plus expanding the comment otherwise the question arises that if the only caller is not interested in the return value then why bother returning it in the first place.
On 2022-07-07 07:34, Tian, Kevin wrote: >> From: Baolu Lu <baolu.lu@linux.intel.com> >> Sent: Thursday, July 7, 2022 8:21 AM >> >> On 2022/7/6 21:43, Robin Murphy wrote: >>> On 2022-07-06 02:53, Baolu Lu wrote: >>>> On 2022/7/6 01:08, Robin Murphy wrote: >>>>> /* >>>>> * Use a function instead of an array here because the domain-type >>>>> is a >>>>> * bit-field, so an array would waste memory. >>>>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) >>>>> (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? >>>>> "(set via kernel command line)" : ""); >>>>> + /* If the system is so broken that this fails, it will WARN >>>>> anyway */ >>>> >>>> Can you please elaborate a bit on this? iommu_bus_init() still return >>>> errors. >>> >>> Indeed, it's commenting on the fact that we don't try to clean up or >>> propagate an error value further even if it did ever manage to return >>> one. I feared that if I strip the error handling out of iommu_bus_init() >>> itself on the same reasoning, we'll just get constant patches from the >>> static checker brigade trying to add it back, so it seemed like the >>> neatest compromise to keep that decision where it's obviously in an >>> early initcall, rather than in the helper function which can be viewed >>> out of context. However, I'm happy to either expand this comment or go >>> the whole way and make iommu_bus_init() return void if you think it's >>> worthwhile. >> >> Thanks for the explanation. It would be helpful if the comment could be >> expanded. In this case, after a long time, people will not consider it >> an oversight. :-) >> > > I'd prefer to making iommu_bus_init() return void plus expanding > the comment otherwise the question arises that if the only caller > is not interested in the return value then why bother returning it > in the first place.
On 2022-07-07 07:31, Tian, Kevin wrote: >> From: Robin Murphy <robin.murphy@arm.com> >> Sent: Wednesday, July 6, 2022 1:08 AM >> >> The number of bus types that the IOMMU subsystem deals with is small and >> manageable, so pull that list into core code as a first step towards >> cleaning up all the boilerplate bus-awareness from drivers. Calling >> iommu_probe_device() before bus->iommu_ops is set will simply return >> -ENODEV and not break the notifier call chain, so there should be no >> harm in proactively registering all our bus notifiers at init time. >> > > Suppose we miss a check on iommu ops in iommu_release_device(): > > if (!dev->iommu) <<<<<<< > return; > > iommu_device_unlink(dev->iommu->iommu_dev, dev); > > ops = dev_iommu_ops(dev); > ops->release_device(dev); > > following the rationale in patch01 a device could be removed when > it's associated with a known but not registered instance. No, because at that point the instance is only known internally to the driver. As long as it isn't erroneously returned from ->probe_device(dev), dev->iommu will remain NULL and the rest of the core code works as expected. Thanks, Robin.
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Thursday, July 7, 2022 5:59 PM > > On 2022-07-07 07:31, Tian, Kevin wrote: > >> From: Robin Murphy <robin.murphy@arm.com> > >> Sent: Wednesday, July 6, 2022 1:08 AM > >> > >> The number of bus types that the IOMMU subsystem deals with is small > and > >> manageable, so pull that list into core code as a first step towards > >> cleaning up all the boilerplate bus-awareness from drivers. Calling > >> iommu_probe_device() before bus->iommu_ops is set will simply return > >> -ENODEV and not break the notifier call chain, so there should be no > >> harm in proactively registering all our bus notifiers at init time. > >> > > > > Suppose we miss a check on iommu ops in iommu_release_device(): > > > > if (!dev->iommu) <<<<<<< > > return; > > > > iommu_device_unlink(dev->iommu->iommu_dev, dev); > > > > ops = dev_iommu_ops(dev); > > ops->release_device(dev); > > > > following the rationale in patch01 a device could be removed when > > it's associated with a known but not registered instance. > > No, because at that point the instance is only known internally to the > driver. As long as it isn't erroneously returned from > ->probe_device(dev), dev->iommu will remain NULL and the rest of the > core code works as expected. > You are correct. I overlooked dev->iommu as device_to_iommu() in patch01. As long as the device hasn't been probed or ->probe_device doesn't do bad thing then dev->iommu should be NULL in this case.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 847ad47a2dfd..514edc0eaa94 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -6,6 +6,7 @@ #define pr_fmt(fmt) "iommu: " fmt +#include <linux/amba/bus.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/kernel.h> @@ -16,11 +17,13 @@ #include <linux/export.h> #include <linux/slab.h> #include <linux/errno.h> +#include <linux/host1x_context_bus.h> #include <linux/iommu.h> #include <linux/idr.h> #include <linux/err.h> #include <linux/pci.h> #include <linux/bitops.h> +#include <linux/platform_device.h> #include <linux/property.h> #include <linux/fsl/mc.h> #include <linux/module.h> @@ -75,6 +78,7 @@ static const char * const iommu_group_resv_type_string[] = { #define IOMMU_CMD_LINE_DMA_API BIT(0) #define IOMMU_CMD_LINE_STRICT BIT(1) +static int iommu_bus_init(struct bus_type *bus); static int iommu_alloc_default_domain(struct iommu_group *group, struct device *dev); static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus, @@ -103,6 +107,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \ static LIST_HEAD(iommu_device_list); static DEFINE_SPINLOCK(iommu_device_lock); +static struct bus_type * const iommu_buses[] = { + &platform_bus_type, +#ifdef CONFIG_PCI + &pci_bus_type, +#endif +#ifdef CONFIG_ARM_AMBA + &amba_bustype, +#endif +#ifdef CONFIG_FSL_MC_BUS + &fsl_mc_bus_type, +#endif +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS + &host1x_context_device_bus_type, +#endif +}; + /* * Use a function instead of an array here because the domain-type is a * bit-field, so an array would waste memory. @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? "(set via kernel command line)" : ""); + /* If the system is so broken that this fails, it will WARN anyway */ + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) + iommu_bus_init(iommu_buses[i]); + return 0; } subsys_initcall(iommu_subsys_init); @@ -1772,35 +1796,19 @@ int bus_iommu_probe(struct bus_type *bus) return ret; } -static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops) +static int iommu_bus_init(struct bus_type *bus) { struct notifier_block *nb; int err; - nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL); + nb = kzalloc(sizeof(*nb), GFP_KERNEL); if (!nb) return -ENOMEM; nb->notifier_call = iommu_bus_notifier; - err = bus_register_notifier(bus, nb); if (err) - goto out_free; - - err = bus_iommu_probe(bus); - if (err) - goto out_err; - - - return 0; - -out_err: - /* Clean up */ - bus_for_each_dev(bus, NULL, NULL, remove_iommu_group); - bus_unregister_notifier(bus, nb); - -out_free: - kfree(nb); + kfree(nb); return err; } @@ -1833,9 +1841,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) bus->iommu_ops = ops; /* Do IOMMU specific setup for this bus-type */ - err = iommu_bus_init(bus, ops); - if (err) + err = bus_iommu_probe(bus); + if (err) { + /* Clean up */ + bus_for_each_dev(bus, NULL, NULL, remove_iommu_group); bus->iommu_ops = NULL; + } return err; }