Re: [PATCH v3] iommu/arm-smmu-qcom: Rework the logic finding the bypass quirk

[Johan Hovold]

On Wed, Mar 15, 2023 at 01:29:58PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 15, 2023 at 08:37:32AM +0100, Johan Hovold wrote:

> > > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > > +{
> > > +	u32 smr;
> > > +	int i;
> > > +
> > > +	/*
> > > +	 * Limit the number of stream matching groups to 128 as the ARM SMMU architecture
> > > +	 * specification defines NUMSMRG (Number of Stream Mapping Register Groups) in the
> > > +	 * range of 0-127, but some Qcom platforms emulate more stream mapping groups. And
> > > +	 * those groups don't exhibit the same behavior as the architecture supported ones.
> > > +	 */
> > 
> > Please fix your editor so that it wraps lines at 80 columns, which is
> > still the preferred (soft) limit.
> > 
> 
> If exceeding 80 columns end up making the comment more readable (fewer lines),
> then why should we limit ourselves?

Exceeding 80 column for comments does generally not improve readability.

That part of the coding standard has do to with not adding excessive
line breaks to *code*, where it can sometimes impact readability.

> > > +	if (smmu->num_mapping_groups > 128) {
> > > +		dev_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");
> > 
> > dev_notice() should do since there's nothing a user can do about this.
> > 
> 
> Ok.
> 
> > > +		smmu->num_mapping_groups = 128;
> > > +	}
> > 
> > So this hunk is really all that is needed to make the current quirk
> > detection work on sc8280xp. Why not simply stick with the current logic
> > and use the last group until there is a need for anything more?
> > 
> 
> No! What if the bootloader had set up mapping for 128 groups? In that case
> we'll overwrite the last group. It is still required to find the valid group
> and use it for quirk detection. If no group is available, we'll skip it.

Yes, but that's also entirely hypothetical (and could perhaps also be
handled by adding a warning for now).

If you want to rework the quirk handling for this you should at least do
so in a separate patch as it is arguably a separate change from fixing
the current quirk detection for newer SoCs by capping the number of
groups (a minimal fix that could be backported).

Johan