On 2023/11/22 1:00, Jason Gunthorpe wrote: > On Tue, Nov 21, 2023 at 03:47:48PM +0900, Hector Martin wrote: >>> Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is >>> returned fwspec will be freed and dev->iommu->fwspec will be NULL >>> here. >>> >>> In the NULL case it does a 'bus probe' with a NULL fwspec and all the >>> fwspec drivers return immediately from their probe functions. >>> >>> Did I miss something? >> >> apple_dart is not a fwspec driver and doesn't do that :-) > > It implements of_xlate that makes it a driver using the fwspec probe > path. > > The issue is in apple-dart. Its logic for avoiding bus probe vs > fwspec probe is not correct. > > It does: > > static int apple_dart_of_xlate(struct device *dev, struct of_phandle_args *args) > { > [..] > dev_iommu_priv_set(dev, cfg); > > > Then: > > 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_stream_map *stream_map; > int i; > > if (!cfg) > return ERR_PTR(-ENODEV); > > Which leaks the cfg memory on rare error cases and wrongly allows the > driver to probe without a fwspec, which I think is what you are > hitting. > > It should be > > if (!dev_iommu_fwspec_get(dev) || !cfg) > return ERR_PTR(-ENODEV); > > To ensure the driver never probes on the bus path. > > Clearing the dev->iommu in the core code has the side effect of > clearing (and leaking) the cfg which would hide this issue. Aha! Yes, that makes it work with only the first change. I'll throw the apple-dart fix into our tree (and submit it once I get to clearing out the branch; the affected consumer driver isn't upstream yet so this isn't particularly urgent). - Hector