Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

[Thierry Reding]

On Mon, Oct 05, 2020 at 04:28:53PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 14:15, Thierry Reding пишет:
> > On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
> >> 05.10.2020 12:53, Thierry Reding пишет:
> >>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> >>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>>>>>  static int tegra_smmu_of_xlate(struct device *dev,
> >>>>>>  			       struct of_phandle_args *args)
> >>>>>>  {
> >>>>>> +	struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> >>>>>> +	struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> >>>>>>  	u32 id = args->args[0];
> >>>>>>  
> >>>>>> +	of_node_put(args->np);
> >>>>>> +
> >>>>>> +	if (!mc || !mc->smmu)
> >>>>>> +		return -EPROBE_DEFER;
> >>>>> platform_get_drvdata(NULL) will crash.
> >>>>>
> >>>>
> >>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> >>>
> >>> How so? It's technically possible for the iommus property to reference a
> >>> device tree node for which no platform device will ever be created, in
> >>> which case of_find_device_by_node() will return NULL. That's very
> >>> unlikely and perhaps worth just crashing on to make sure it gets fixed
> >>> immediately.
> >>
> >> The tegra_smmu_ops are registered from the SMMU driver itself and MC
> >> driver sets platform data before SMMU is initialized, hence device is
> >> guaranteed to exist and mc can't be NULL.
> > 
> > Yes, but that assumes that args->np points to the memory controller's
> > device tree node. It's obviously a mistake to do this, but I don't think
> > anyone will prevent you from doing this:
> > 
> > 	iommus = <&{/chosen} 0>;
> > 
> > In that case, since no platform device is created for the /chosen node,
> > iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
> 
> But then Tegra SMMU isn't associated with the device's IOMMU path, and
> thus, tegra_smmu_of_xlate() won't be invoked for this device.

Indeed, you're right! It used to be that ops were assigned to the bus
without any knowledge about the specific instances that might exist, but
nowadays there's struct iommu_device which properly encapsulates all of
that, so yeah, I don't think this can ever be NULL.

Although that makes me wonder why we aren't going one step further and
pass struct iommu_device * into ->of_xlate(), which would avoid the need
for us to do the look up once more.

Thierry

Attachment: signature.asc
Description: PGP signature