Re: [PATCH] arm-smmu: check for generic bindings first

[Stefano Stabellini]

On Wed, 28 Aug 2019, Robin Murphy wrote:
> On 28/08/2019 18:38, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > 
> > Today, the arm-smmu driver checks for mmu-masters on device tree, the
> > legacy property, if it is absent it assumes that we are using the new
> > bindings. If it is present it assumes that we are using the legacy
> > bindings. All arm-smmus need to use the same bindings: legacy or new.
> > 
> > There are two issues with this:
> > 
> > - we are not actually checking for the new bindings explicitly
> > It would be better to have an explicit check for the new bindings rather
> > than assuming we must be using the new if the old are not there.
> 
> The legacy binding is the special case, though. Isn't it perfectly logical to
> check for the special case, and assume the normal case otherwise? ;)

I don't know ... all cases are special and all cases are normal
depending on who you ask or when. The normal case of today can be the
special case of tomorrow and vice versa.


> > - old and new bindings cannot coexist
> > It would be nice to be able to provide both old and new bindings so
> > that software that hasn't been updated yet is still able to get IOMMU
> > information from the legacy bindings while at the same time newer
> > software can get the latest information via the new bindings. (Xen has
> > not been updated to use the new binding yet for instance.) The current
> > code breaks under these circumstances because if the old bindings are
> > present, the new are not even checked.
> 
> FWIW that was a deliberate design decision. We didn't want to see DTs with
> both bindings at once - the legacy binding support remains only for the sake
> of DTBs deployed in fossilised firmware that can't/won't ever be updated. And
> I'm pretty sure the intervening 3 years have only reinforced that position.
> Only a couple more months now until it will have been deprecated for longer
> than it wasn't :)

This way the people that would be willing to put in extra effort to
maintain compatibility wouldn't be able to achieve the goal.


> Do note that "mmu-masters" isn't just deprecated for giggles either - the
> binding made some flawed assumptions, and there are Stream ID topologies that
> it cannot possibly describe, which do exist in the real world.
>
> > This patch changes the scheme by checking for #iommu-cells, which is
> > only present with the new bindings, before checking for mmu-masters.
> > The new bindings are always favored when present. All SMMUs still need
> > to use the same bindings: mix-and-matching new and old bindings between
> > different SMMUs is not allowed.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > ---
> > 
> > Let me know if you'd like me to turn the two using_*_binding variables
> > into a single one.
> > 
> > Also, please note that this is not meant as an excuse not to get Xen
> > updated to use the new binding.
> 
> Fair enough, but conversely, what makes it Linux's responsibility to handle?
> If Xen wants to deal with funky hybrid DTs then why shouldn't it be Xen's job
> to just filter deprecated properties out of whatever it presents to Linux?

Yes, Xen does it just fine. But if you boot Linux with the same DT but
without Xen, problems occur.

The code looked suboptimal to me because of the reasons I listed in the
commit message. I thought it was done this way by "mistake". However, I
now realize that if it is written this way on purpose, then I have a
very little chance of convincing you to change it.


 
> >   drivers/iommu/arm-smmu.c | 22 +++++++++++++---------
> >   1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 64977c131ee6..79b518ff215c 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -2118,7 +2118,7 @@ static int arm_smmu_device_dt_probe(struct
> > platform_device *pdev,
> >   {
> >   	const struct arm_smmu_match_data *data;
> >   	struct device *dev = &pdev->dev;
> > -	bool legacy_binding;
> > +	bool legacy_binding, generic_binding;
> >     	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> >   				 &smmu->num_global_irqs)) {
> > @@ -2132,16 +2132,20 @@ static int arm_smmu_device_dt_probe(struct
> > platform_device *pdev,
> >     	parse_driver_options(smmu);
> >   -	legacy_binding = of_find_property(dev->of_node, "mmu-masters", NULL);
> > -	if (legacy_binding && !using_generic_binding) {
> > -		if (!using_legacy_binding)
> > -			pr_notice("deprecated \"mmu-masters\" DT property in
> > use; DMA API support unavailable\n");
> > -		using_legacy_binding = true;
> > -	} else if (!legacy_binding && !using_legacy_binding) {
> > +	generic_binding = of_find_property(dev->of_node, "#iommu-cells",
> > NULL);
> > +	if (generic_binding && !using_legacy_binding) {
> >   		using_generic_binding = true;
> >   	} else {
> > -		dev_err(dev, "not probing due to mismatched DT properties\n");
> > -		return -ENODEV;
> > +		legacy_binding = of_find_property(dev->of_node, "mmu-masters",
> > +						  NULL);
> > +		if (legacy_binding && !using_generic_binding) {
> > +			if (!using_legacy_binding)
> > +				pr_notice("deprecated \"mmu-masters\" DT
> > property in use; DMA API support unavailable\n");
> > +			using_legacy_binding = true;
> > +		} else {
> > +			dev_err(dev, "not probing due to mismatched DT
> > properties\n");
> > +			return -ENODEV;
> > +		}
> >   	}
> >     	if (of_dma_is_coherent(dev->of_node))
> > 
>