Re: [PATCH 1/1 V3] AMD Family15h Model10-1Fh erratum 746 Workaround

[Borislav Petkov]

On Thu, Jan 24, 2013 at 06:30:15AM -0600, suravee.suthikulpanit@xxxxxxx wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> 
> Changes in V3:
> 	* Add proper commit message
> 	* Change logic to avoid unnecessary indentaion
> 
> Changes in V2:
> 	* Fix logic that check the processor model.
> 	* Clear write enable bit after apply workaround
> 	* Change function name 

Changelogs need to be after the diffstat, see below. They should be
part of the commit message only when they contain important information
pertaining to the fix itself and those pieces which are needed for patch
tracking and not to be in changelog should go after the "---" so that
git doesn't pick them up in the commit message.

> Commit Message:
> Erratum 746: IOMMU Logging May Stall Transaction
> Workaround:  BIOS should disable L2B micellaneous clock gating by setting 
>              L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b

This commit message is kinda laconic: it says it all for AMD folk who
stare at RGs all the time but not to normal users. I've attached a
corrected version of this patch so that you can see what I mean. And
I've taken the text from the RG and made it a bit more readable so that
I can piss off Eric again :-).

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

This patch is adding trailing whitespace:

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: AMD Family15h Model10-1Fh erratum 746 Workaround
/w/kernel/linux-2.6/.git/rebase-apply/patch:18: trailing whitespace.
        if ((boot_cpu_data.x86 != 0x15) || 
/w/kernel/linux-2.6/.git/rebase-apply/patch:22: trailing whitespace.

/w/kernel/linux-2.6/.git/rebase-apply/patch:24: trailing whitespace.
        pci_read_config_dword(iommu->dev, 0xf4, &value); 
/w/kernel/linux-2.6/.git/rebase-apply/patch:48: trailing whitespace.

warning: 4 lines add whitespace errors.

You can catch stuff like that when running your patch through
scripts/checkpatch.pl:

ERROR: trailing whitespace
#36: FILE: drivers/iommu/amd_iommu_init.c:987:
+^Iif ((boot_cpu_data.x86 != 0x15) || $

ERROR: trailing whitespace
#40: FILE: drivers/iommu/amd_iommu_init.c:991:
+^I$

ERROR: trailing whitespace
#42: FILE: drivers/iommu/amd_iommu_init.c:993:
+^Ipci_read_config_dword(iommu->dev, 0xf4, &value); $

ERROR: trailing whitespace
#66: FILE: drivers/iommu/amd_iommu_init.c:1206:
+^I$

total: 4 errors, 2 warnings, 46 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
      scripts/cleanfile

Your patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 81837b0..934b017 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -975,6 +975,38 @@ static void __init free_iommu_all(void)
>  }
>  
>  /*
> + * AMD family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations)
> + * Workaround:
> + *     BIOS should disable L2B micellaneous clock gating by setting
> + *     L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b
> + */
> +static void __init amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
> +{
> +	u32 value;
> +
> +	if ((boot_cpu_data.x86 != 0x15) || 
> +	    (boot_cpu_data.x86_model < 0x10) ||
> +	    (boot_cpu_data.x86_model > 0x1f))
> +		return;
> +	
> +	pci_write_config_dword(iommu->dev, 0xf0, 0x90);
> +	pci_read_config_dword(iommu->dev, 0xf4, &value); 
> +
> +	if (value & BIT(2))
> +		return;
> +
> +	/* Select NB indirect register 0x90 and enable writing */
> +	pci_write_config_dword(iommu->dev, 0xf0, 0x90 | (1 << 8));
> +
> +	pci_write_config_dword(iommu->dev, 0xf4, value | 0x4);
> +	pr_info("AMD-Vi: Applying erratum 746 workaround for IOMMU at %s\n",
> +	 dev_name(&iommu->dev->dev));

This should be aligned with "AMD-Vi... " above.

> +
> +	/* Clear the enable writing bit */
> +	pci_write_config_dword(iommu->dev, 0xf0, 0x90);
> +}
> +
> +/*
>   * This function clues the initialization function for one IOMMU
>   * together and also allocates the command buffer and programs the
>   * hardware. It does NOT enable the IOMMU. This is done afterwards.
> @@ -1171,6 +1203,8 @@ static int iommu_init_pci(struct amd_iommu *iommu)
>  		for (i = 0; i < 0x83; i++)
>  			iommu->stored_l2[i] = iommu_read_l2(iommu, i);
>  	}
> +	
> +	amd_iommu_erratum_746_workaround(iommu);
>  
>  	return pci_enable_device(iommu->dev);

Ok, here it is, take a look.

---