On Tue, Nov 21, 2023 at 04:06:15PM +0000, Robin Murphy wrote: > > Obviously. I rejected that right away because of how incredibly > > wrongly layered and hacky it is to do something like that. > > What, and dressing up the fundamental layering violation by baking it even > further into the API flow, while still not actually fixing it or any of its > *other* symptoms, is somehow better? It puts the locks and the controlled data in the right place, in the right layer. > Ultimately, this series is still basically doing the same thing my patch > does - extending the scope of the existing iommu_probe_device_lock hack to > cover fwspec creation. A hack is a hack, so frankly I'd rather it be simple > and obvious and look like one, and being easy to remove again is an obvious > bonus too. Not entirely, as I've said this series removes the use of the dev->iommu during fwspec creation. This is different than just extending the lock. > > So, I would move toward having the driver's probe invoke a helper like: > > > > iommu_of_xlate(dev, fwspec, &per_fwnode_function, &args); > > > > Which generates the same list of (fwnode_handle, of_phandle_args) that > > was passed to of_xlate today, but is ordered sensibly within the > > sequence of probe for what many drivers seem to want to do. > > Grep for of_xlate. It is a standard and well-understood callback pattern for > a subsystem to parse a common DT binding and pass a driver-specific > specifier to a driver to interpret. Or maybe you just have a peculiar > definition of what you think "micro-optimisation" means? :/ Don't know about other subsystems, here is making a mess. Look at what the drivers are doing. The two SMMU drivers are sort of sane, but everything else has coded half their pobe into of_xlate. It doesn't make alot of sense. > > So, it is not so much that that the idea of of_xlate goes away, but > > the specific op->of_xlate does, it gets shifted into a helper that > > invokes the same function in a more logical spot. > > I'm curious how you imagine an IOMMU driver's ->probe function could be > called *before* parsing the firmware to work out what, if any, IOMMU, and > thus driver, a device is associated with. You've jumped ahead here, I'm talking about removing ops->of_xlate. With everything kept the same as today this means we'd scan the FW description twice. Once to find the ops and once inside the driver to parse it. When I say micro-optimization this is what I mean - structuring the code to try to avoid multiple-scans of the FW table. Once the drivers are self-parsing I see there are two paths to keep it at one FW parse: 1) Have the core code parse and cache common cases in the iommu_fwspec. Driver then pulls out the common cases from the iommu_fwspec and reparsed in uncommon cases. 2) Accept we have only a single ops in all real systems and just invoke the driver to parse it. That parse will cache enough information to decide if EPROBE_DEFER is needed. In either case the drivers would call the same APIs and have the same logic. The choice is just infrastructure-side stuff. It is a different view than trying to do everything up front, but I'm not seeing that it is so differently efficient as to be a performance problem. On the plus side it looks to make the drivers alot simpler and more logical. > And then every driver has to have > identical boilerplate to go off and parse the generic "iommus" binding... > which is the whole damn reason for *not* going down that route and instead > using an of_xlate mechanism in the first place. Let's not guess. I've attached below a sketch conversion for apple-dart. Diffstat says it *saves* 1 line. But also we fix several bugs! - iommu_of_xlate() will check for NULL fwspec and reject the bus probe path - iommu_of_xlate() can match the iommu's from the iommu list and check that the OF actually points only to iommus with this driver's ops (missed sanity check) - The of parsing machinery already computes the iommu_driver but throws it out. This forces all of the drivers to do their own thing. Pass it in and thus apple_dart_of_xlate() doesn't need to mess around with of_find_device_by_node() and we fix the bug where it leaks the reference on the struct device (woops!) - We don't leak the cfg allocation that apple_dart_of_xlate() did on various error paths. All error paths logically free in probe. We don't have to think about what happens if of_xlate is called twice for the same args on error/reprobe paths. Many drivers follow this pattern of apple-dart and would end up like this. Drivers that just need the u32 array would be simpler, more like: struct iommu_driver *instance; unsigned int num_ids; instance = iommu_of_get_iommu(..., &num_ids); if (IS_ERR(instance)) return ERR_CAST(instance); cfg = kzalloc(struct_size(cfg, sids, num_ids), GFP_KERNEL); if (!cfg) return -ENOMEM; iommu_of_read_u32_ids(..., &cfg->sids); [..] return instance; I haven't sketched *every* driver yet, but I've sketched enough to be confident. Robin, I know it is not what you have in your head, but you should stop with the insults and be more open to other perspectives. > > The per-device data can be allocated at the top of probe and passed > > through args to fix the lifetime bugs. > > > > It is pretty simple to do. > > I believe the kids these days would say "Say you don't understand the code > without saying you don't understand the code." I think you are too fixated on what you have in your mind. It will take me a bit but I will come with a series to move all the FW parsing into probe along this model. Once done it is trivial to make bus probe work like it should. Regarding this series, I don't really see a problem. There is no "concrete" or anything like that. > > Not quite. This decouples two unrelated things into seperate > > concerns. It is not so much about the concurrency but removing the > > abuse of dev->iommu by code that has no need to touch it at all. > > Sorry, the "abuse" of storing IOMMU-API-specific data in the place we > intentionally created to consolidate all the IOMMU-API-specific data > into? The global state should not be filled until the driver is probed. It is an abuse to use a global instead of an on-stack variable when building it. Publishing only fully initialized data to global visibility is concurrency 101. :( Jason diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c index ee05f4824bfad1..476938722460b8 100644 --- a/drivers/iommu/apple-dart.c +++ b/drivers/iommu/apple-dart.c @@ -721,19 +721,29 @@ static struct iommu_domain apple_dart_blocked_domain = { static struct iommu_device *apple_dart_probe_device(struct device *dev) { - struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev); + struct apple_dart_master_cfg *cfg; struct apple_dart_stream_map *stream_map; + int ret; int i; + cfg = kzalloc(sizeof(*cfg), GFP_KERNEL); if (!cfg) - return ERR_PTR(-ENODEV); + return ERR_PTR(-ENOMEM); + ret = iommu_of_xlate(dev_iommu_fwspec_get(dev), &apple_dart_iommu_ops, + 1, &apple_dart_of_xlate, cfg); + if (ret) + goto err_free; for_each_stream_map(i, cfg, stream_map) device_link_add( dev, stream_map->dart->dev, DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER); + dev_iommu_priv_set(dev, cfg); return &cfg->stream_maps[0].dart->iommu; +err_free: + kfree(cfg); + return ret; } static void apple_dart_release_device(struct device *dev) @@ -777,25 +787,15 @@ static void apple_dart_domain_free(struct iommu_domain *domain) kfree(dart_domain); } -static int apple_dart_of_xlate(struct device *dev, struct of_phandle_args *args) +static int apple_dart_of_xlate(struct iommu_device *iommu, + struct of_phandle_args *args, void *priv) { - struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev); - struct platform_device *iommu_pdev = of_find_device_by_node(args->np); - struct apple_dart *dart = platform_get_drvdata(iommu_pdev); - struct apple_dart *cfg_dart; - int i, sid; + struct apple_dart *dart = container_of(iommu, struct apple_dart, iommu); + struct apple_dart_master_cfg *cfg = priv; + struct apple_dart *cfg_dart = cfg->stream_maps[0].dart; + int sid = args->args[0]; + int i; - if (args->args_count != 1) - return -EINVAL; - sid = args->args[0]; - - if (!cfg) - cfg = kzalloc(sizeof(*cfg), GFP_KERNEL); - if (!cfg) - return -ENOMEM; - dev_iommu_priv_set(dev, cfg); - - cfg_dart = cfg->stream_maps[0].dart; if (cfg_dart) { if (cfg_dart->supports_bypass != dart->supports_bypass) return -EINVAL; @@ -980,7 +980,6 @@ static const struct iommu_ops apple_dart_iommu_ops = { .probe_device = apple_dart_probe_device, .release_device = apple_dart_release_device, .device_group = apple_dart_device_group, - .of_xlate = apple_dart_of_xlate, .def_domain_type = apple_dart_def_domain_type, .get_resv_regions = apple_dart_get_resv_regions, .pgsize_bitmap = -1UL, /* Restricted during dart probe */