Hi Jacob,
On 2020/5/14 23:57, Jacob Pan wrote:
Hi Christoph,
Thanks a lot for the reviews, comments below.
Jacob
On Wed, 13 May 2020 22:59:30 -0700
Christoph Hellwig<[email protected]> wrote:
+ if (dev_is_pci(dev)) {
+ /* VT-d supports devices with full 20 bit PASIDs
only */
+ if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX)
+ return -EINVAL;
+ } else {
+ return -ENOTSUPP;
+ }
This looks strange. Why not:
if (!dev_is_pci(dev)) {
return -ENOTSUPP;
/* VT-d supports devices with full 20 bit PASIDs only */
if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX)
return -EINVAL;
That is better, will do.
+ for_each_svm_dev(sdev, svm, dev) {
+ /*
+ * For devices with aux domains, we should
allow multiple
+ * bind calls with the same PASID and pdev.
+ */
+ if (iommu_dev_feature_enabled(dev,
IOMMU_DEV_FEAT_AUX)) {
+ sdev->users++;
+ } else {
+ dev_warn_ratelimited(dev, "Already
bound with PASID %u\n",
+ svm->pasid);
+ ret = -EBUSY;
+ }
+ goto out;
Is this intentionally a for loop that jumps out of the loop after
the first device?
The name is confusing, it is not a loop. I will change it to
find_svm_dev() and comments like this?
/*
* Find the matching device in a given SVM. The bind code ensures that
* each device can only be added to the SVM list once.
*/
#define find_svm_dev(sdev, svm, d) \
list_for_each_entry((sdev), &(svm)->devs, list) \
if ((d) != (sdev)->dev) {} else
The for_each_svm_dev() is not added by this series and is also used by
other functions. How about changing it in a separated patch?
Best regards,
baolu
