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

[Manivannan Sadhasivam]

On Wed, Mar 15, 2023 at 09:38:42AM +0100, Johan Hovold wrote:
> 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.
> 

It all depends on the perspective/preference... But I can limit to 80 columns
here.

> 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).
> 

Using a separate patch makes sense to me. Will do that in v4.

Thanks,
Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்