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: <2a103a6d-af9f-456d-eda4-f1e15f8367b6@linux.intel.com>
Date:   Fri, 14 Jul 2023 14:00:07 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Cc:     baolu.lu@linux.intel.com, Jason Gunthorpe <jgg@nvidia.com>,
        Will Deacon <will@kernel.org>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "Yu, Fenghua" <fenghua.yu@intel.com>,
        "Luck, Tony" <tony.luck@intel.com>
Subject: Re: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma
 domain
Content-Language: en-US
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        Joerg Roedel <joro@8bytes.org>,
        Jean-Philippe Brucker <jean-philippe@linaro.com>,
        Robin Murphy <robin.murphy@arm.com>
References: <20230712163355.3177511-1-jacob.jun.pan@linux.intel.com>
 <20230712163355.3177511-7-jacob.jun.pan@linux.intel.com>
 <BN9PR11MB5276C14341AC7CE6D352948F8C37A@BN9PR11MB5276.namprd11.prod.outlook.com>
 <87e78302-9f69-8db1-8a0a-b40e1f29d9c6@linux.intel.com>
 <BN9PR11MB5276E0C6F1063E581F29BB978C34A@BN9PR11MB5276.namprd11.prod.outlook.com>
From:   Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <BN9PR11MB5276E0C6F1063E581F29BB978C34A@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/7/14 11:50, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, July 14, 2023 11:34 AM
>>
>> On 2023/7/13 15:56, Tian, Kevin wrote:
>>>> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>>> Sent: Thursday, July 13, 2023 12:34 AM
>>>>
>>>>
>>>> -	/*
>>>> -	 * Should never reach here until we add support for attaching
>>>> -	 * non-SVA domain to a pasid.
>>>> -	 */
>>>> -	WARN_ON(1);
>>>> +	dmar_domain = to_dmar_domain(domain);
>>>> +	spin_lock_irqsave(&dmar_domain->lock, flags);
>>>> +	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
>>>> {
>>>> +		if (curr->dev == dev && curr->pasid == pasid) {
>>>> +			list_del(&curr->link_domain);
>>>> +			dev_pasid = curr;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +	spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>>
>>>
>>> what about no matching dev_pasid is find?
>>
>> kfree() can handle this gracefully.
> 
> but what about domain_detach_iommu()? Is it correct to adjust
> the refcnting when there is no matching dev_pasid?

You are right.

Logically, if we get a valid domain for a pasid, we should have a
dev_pasid allocated for it. Perhaps, adding a check in the code will
make the code more readable?

Best regards,
baolu