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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230315133730.GB98059@thinkpad>
Date:   Wed, 15 Mar 2023 19:07:30 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Johan Hovold <johan@...nel.org>
Cc:     will@...nel.org, joro@...tes.org, robin.murphy@....com,
        andersson@...nel.org, johan+linaro@...nel.org, steev@...i.org,
        linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iommu/arm-smmu-qcom: Rework the logic finding the
 bypass quirk

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

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