[PATCH] iommu/arm-smmu-v3: fixed check process for disable_bypass module parameter

[ishii.shuuichir at fujitsu.com]

Hi, Robin,
Thank you for your comments.

> >   	/* Enable the SMMU interface, or ensure bypass */
> > -	if (!bypass || disable_bypass) {
> > +	if (!bypass && disable_bypass) {
> 
> This change looks obviously wrong - if bypass is false here then we definitely
> want to enable the SMMU, so disable_bypass is irrelevant. It shouldn't even be
> possible to get here with bypass==true under ACPI, since
> arm_smmu_device_acpi_probe() cannot fail :/

Sorry, my understanding of the meaning of the disable_bypass module parameter 
and the process of setting GBPA_ABORT was insufficient.

I misunderstood that the disable_bypass module parameter is used to simply 
bypass (disable) SMMU (SMMU_CR0.SMMUEN == 0 and SMMU_GBPA.ABORT == 0).
Forget about the fixes in this patch.

Although our understanding was lacking, 
we thought it would be a good idea to have a module parameter that simply disables SMMU,
so we were considering a fix.

Best regards, 
Shuuichirou.

> -----Original Message-----
> From: Robin Murphy <robin.murphy at arm.com>
> Sent: Wednesday, August 3, 2022 6:26 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir at fujitsu.com>;
> will at kernel.org; joro at 8bytes.org; thunder.leizhen at huawei.com; jgg at ziepe.ca;
> tglx at linutronix.de; chenxiang66 at hisilicon.com; christophe.jaillet at wanadoo.fr;
> john.garry at huawei.com; baolu.lu at linux.intel.com;
> linux-arm-kernel at lists.infradead.org; iommu at lists.linux.dev;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH] iommu/arm-smmu-v3: fixed check process for
> disable_bypass module parameter
> 
> On 2022-08-03 00:42, Shuuichirou Ishii wrote:
> > The current process does not enable the bypass setting regardless of
> > the value of the disable_bypass module parameter when ACPI is enabled,
> > so the value of the disable_bypass module parameter has been corrected
> > so that it is handled correctly.
> >
> > Signed-off-by: Shuuichirou Ishii <ishii.shuuichir at fujitsu.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 88817a3376ef..256d7b2a83a7 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3396,7 +3396,7 @@ static int arm_smmu_device_reset(struct
> arm_smmu_device *smmu, bool bypass)
> >   		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
> >
> >   	/* Enable the SMMU interface, or ensure bypass */
> > -	if (!bypass || disable_bypass) {
> > +	if (!bypass && disable_bypass) {
> 
> This change looks obviously wrong - if bypass is false here then we definitely
> want to enable the SMMU, so disable_bypass is irrelevant. It shouldn't even be
> possible to get here with bypass==true under ACPI, since
> arm_smmu_device_acpi_probe() cannot fail :/
> Robin.
> 
> >   		enables |= CR0_SMMUEN;
> >   	} else {
> >   		ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);