Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2430049f-bcb3-466b-fbed-03b4cd0e895a@amd.com>
Date:   Wed, 14 Sep 2022 20:16:09 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH v2 02/13] iommu/amd: Drop unnecessary checks in
 amd_iommu_attach_device()
Content-Language: en-US
To:     Nicolin Chen <nicolinc@nvidia.com>, joro@8bytes.org,
        suravee.suthikulpanit@amd.com, will@kernel.org,
        robin.murphy@arm.com
Cc:     jgg@nvidia.com, kevin.tian@intel.com, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20220914051451.10304-1-nicolinc@nvidia.com>
From:   Vasant Hegde <vasant.hegde@amd.com>
In-Reply-To: <20220914051451.10304-1-nicolinc@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Yng0OFFaWVd1UHZvSzhIV0RRSEY2WUtlSzJwc1BSU3FRVEVmVGZ6ZndWWE5T?=
 =?utf-8?B?UDZlVFRyR1BQdWlQeHFQWDZMek9DQzBDV21ndllHZ0E2L0Y3M2ViOGtQMlZE?=
 =?utf-8?B?QWRVYVR0NUI3VE11WnUwOW54OXRiV1NBNmpTbUdac2lQUDFWWmlTSFA2Zllv?=
 =?utf-8?B?MFRySUl4bnpzcmpXanpManovYWNVZXJYSHRVM0syRVlJWEkyNm8vWjVWTXlB?=
 =?utf-8?B?SlZPcFFHcWVHVms4MjVrTDI3R1NEMHAzdFhHdDhhQTJpRWMxeEhwMFNreHQ2?=
 =?utf-8?B?NkkzNFd0T3Y5L0tsS0hZZzNJcGcxY2EzWFpJTmJLY3ZpNDNZSDlHd1M1TUxt?=
 =?utf-8?B?bTBTeFc5bzBRUWJtK3kvaFdUZnRyUTN3R0tRMVVaNk52VjRyVDNpU3hIREVR?=
 =?utf-8?B?YWNpa2xCajAwN0t1YXZpTDRTeW93aHVLYXZocG1tY0Rxd0luaEpINWZiQkI5?=
 =?utf-8?B?ZlVvVm9aU3lVbzMxZmk3QmxEOGhTejEyVzl3cDhYVzNOS29vOHlyS2hnTmVo?=
 =?utf-8?B?QzcvN0tvRGxlbkhXcm1LYjZvc3Z1ZjVzRVRyQU1uUU5UTTFZaHJlQmE4VStC?=
 =?utf-8?B?M0ppbVhtMG5kNmhOd0RORm1WUWdjZmlBUmxwalAyZ21qUkxkRlVuSTlSSzlT?=
 =?utf-8?B?Q1htWWhZTytUR2lGM1MrWlFNdm05TDBkc1lhWFhpUGZrczhNb09YYlExcU4x?=
 =?utf-8?B?alNSdFVPb2NhQTNRUXg0bGJXT1ZtR01ua2daMGN1TzBPVkt4NFdNSTBmSUxy?=
 =?utf-8?B?SGF1Wk1BZDJqM2NOL25VajVkaHIyenZ3Y3JlWXVrR3RKdkVydHNqTS9ZQkVz?=
 =?utf-8?B?NUJ4MnVpQ0VpVWs3YzUwWFZvaWJmUGJqS2U1MDdSVm12YXpyOVZwaWhJcldy?=
 =?utf-8?B?dVZBck52Z2VhSUpqbTJaRXhxcnpkdGRIRWpHWEROWGltYkM5aWMrc0R3dEYr?=
 =?utf-8?B?Y3JQWHcvUy9KN1hCdnpNdi9lSm1qSlBFS2JscThNRENUOEhaNXlCUGczekVu?=
 =?utf-8?B?S0ZyYVVoUmZSQVE2LzBGZmY3QnNKSnhDUjJuMmNIa1FBdDlGR1M2ZU0rRk1j?=
 =?utf-8?B?T3laWWJ4cStjVWc2V2xQa1hzVksvNWFPcWE2ZlN4Ylk1RmRvUkRBNk5ZbjNo?=
 =?utf-8?B?bzZIYk1NYmVkZ1NybkJLcXRqblhBR0J3S2EwWFJ1YTByazJVOUlaL0NWdEZB?=
 =?utf-8?B?YkZWSEdWdHhvTmhsT0szYUVmeG1KYjlaQW1nUjREcG43TjhzbEY4WGpzMFdG?=
 =?utf-8?B?M0NqOVBzU1RReXNqR0VaM0tGR3BPcmVTazU4NGJiU1VpbEEwRm1CWXFJYWZL?=
 =?utf-8?B?aDhhYklXN1RlT1R1NE9PeWp3VEljTXJNWjNISEpKTThrQXJlMXFTWE5lYmdN?=
 =?utf-8?B?blFvMVNtNXQ3Y1FmcklFQldhUHYzOHE2K2lmTmJhWFlTZWowK1hJY2JBSVFB?=
 =?utf-8?B?djRITXVOT2doamlkNFk1ZnVBbnpoQ3IvMXJ6ejJJQXgwY28wS25GNmxmdFBT?=
 =?utf-8?B?dnJrSjhKa1ZNSE9KTHdJTWFkVjFSc3hrRjhCSGxSOURZZVNHQ1JFKzEyQmxi?=
 =?utf-8?B?YzlqTTRtYlRPQ2JtQ1dwNTZ2M2s1eG8vM2xPWUd0d2VYSUFFOEhNSGtWSkpn?=
 =?utf-8?B?TzNFYVN4a2V0SS9WUzRzMU4zWGFVZVZwNEJFbE45OExHZ1hvbGlkcjlIekVO?=
 =?utf-8?B?UFh5ZCtSK0hZdmk0WWpFL1EwTUlwU1FVTC9aamRCUXdqdm1jYlNnaWNSV05m?=
 =?utf-8?B?TDdaN2dmT2pIaWcvWktnWXNnY1lhRGZrYkpTak5melc2ZDg4WWNJRVpFRG8x?=
 =?utf-8?B?S3ZoQVdaNlJBRUFCWkJTbUFLVHhnZFhGNjdWY3ByZG1TSHI4Nk9VNnBMMExG?=
 =?utf-8?B?TWJPNk9HcDVENUhMcEVJYWxjQWlQYUk3dWVnNitkNHFpd3krczFkWUl6anZm?=
 =?utf-8?B?T05tTGNvaGZIck9kS0x5VDBtNU5URzNWcStpMVVWbmFkMWF4ci9TMjBGaXRQ?=
 =?utf-8?B?Wlc0OTVDaklYbm5ydC9MbkpvaGtpbmZIMms0czRaUXhITnRRQU01VkxzQXFG?=
 =?utf-8?B?NHhRYS91Mm1TTndxZm10TXZ3TERDRk9nSEk3ZDgvRnoxNFp4bjB6U1NpbDlw?=
 =?utf-8?Q?6GNfFYerk/bGoMdHL1u84/HxN?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c4131a3-f253-476f-7412-08da965fe344
X-MS-Exchange-CrossTenant-AuthSource: IA1PR12MB6043.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Sep 2022 14:46:20.6882
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: saMSe1KskcoaOd96ANrqqY7Y5EAp2jhsGb/yfjKJDlxWj9TN8QMl+W6EuaDM6boGFmJ2cQEJRCdtz6BgORRbTg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6659
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/14/2022 10:44 AM, Nicolin Chen wrote:
> The same checks are done in amd_iommu_probe_device(). If any of them fails
> there, then the device won't get a group, so there's no way for it to even
> reach amd_iommu_attach_device any more.
> 
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Looks good to me.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

> ---
>  drivers/iommu/amd/iommu.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 828672a46a3d..930d9946b9f7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2135,21 +2135,13 @@ static void amd_iommu_detach_device(struct iommu_domain *dom,
>  static int amd_iommu_attach_device(struct iommu_domain *dom,
>  				   struct device *dev)
>  {
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	struct protection_domain *domain = to_pdomain(dom);
> -	struct iommu_dev_data *dev_data;
> -	struct amd_iommu *iommu;
> +	struct amd_iommu *iommu = rlookup_amd_iommu(dev);
>  	int ret;
>  
> -	if (!check_device(dev))
> -		return -EINVAL;
> -
> -	dev_data = dev_iommu_priv_get(dev);
>  	dev_data->defer_attach = false;
>  
> -	iommu = rlookup_amd_iommu(dev);
> -	if (!iommu)
> -		return -EINVAL;
> -
>  	if (dev_data->domain)
>  		detach_device(dev);
>