Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

iommu: Replace device_lock() with group->mutex

device_lock() was used in iommu_group_store_type() to prevent the
devices in an iommu group from being attached by any device driver.
On the other hand, in order to avoid lock race between group->mutex
and device_lock(), it limited the usage scenario to the singleton
groups.

We already have the DMA ownership scheme to avoid driver attachment
and group->mutex ensures that device ops are always valid, there's
no need for device_lock() anymore. Remove device_lock() and the
singleton group limitation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Link: https://lore.kernel.org/r/20230322064956.263419-6-baolu.lu@linux.intel.com
Signed-off-by: Joerg Roedel <jroedel@suse.de>

authored by

Lu Baolu and committed by
Joerg Roedel
49a22aae 33793748

+18 -63
+18 -63
drivers/iommu/iommu.c
··· 2956 2956 goto out; 2957 2957 } 2958 2958 2959 - /* We can bring up a flush queue without tearing down the domain */ 2960 - if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) { 2961 - ret = iommu_dma_init_fq(prev_dom); 2962 - if (!ret) 2963 - prev_dom->type = IOMMU_DOMAIN_DMA_FQ; 2964 - goto out; 2965 - } 2966 - 2967 2959 /* Sets group->default_domain to the newly allocated domain */ 2968 2960 ret = iommu_group_alloc_default_domain(dev->bus, group, type); 2969 2961 if (ret) ··· 2988 2996 * transition. Return failure if this isn't met. 2989 2997 * 2990 2998 * We need to consider the race between this and the device release path. 2991 - * device_lock(dev) is used here to guarantee that the device release path 2999 + * group->mutex is used here to guarantee that the device release path 2992 3000 * will not be entered at the same time. 2993 3001 */ 2994 3002 static ssize_t iommu_group_store_type(struct iommu_group *group, ··· 3015 3023 else 3016 3024 return -EINVAL; 3017 3025 3018 - /* 3019 - * Lock/Unlock the group mutex here before device lock to 3020 - * 1. Make sure that the iommu group has only one device (this is a 3021 - * prerequisite for step 2) 3022 - * 2. Get struct *dev which is needed to lock device 3023 - */ 3024 3026 mutex_lock(&group->mutex); 3025 - if (iommu_group_device_count(group) != 1) { 3027 + /* We can bring up a flush queue without tearing down the domain. */ 3028 + if (req_type == IOMMU_DOMAIN_DMA_FQ && 3029 + group->default_domain->type == IOMMU_DOMAIN_DMA) { 3030 + ret = iommu_dma_init_fq(group->default_domain); 3031 + if (!ret) 3032 + group->default_domain->type = IOMMU_DOMAIN_DMA_FQ; 3026 3033 mutex_unlock(&group->mutex); 3027 - pr_err_ratelimited("Cannot change default domain: Group has more than one device\n"); 3028 - return -EINVAL; 3034 + 3035 + return ret ?: count; 3029 3036 } 3030 3037 3031 - /* Since group has only one device */ 3038 + /* Otherwise, ensure that device exists and no driver is bound. */ 3039 + if (list_empty(&group->devices) || group->owner_cnt) { 3040 + mutex_unlock(&group->mutex); 3041 + return -EPERM; 3042 + } 3043 + 3032 3044 grp_dev = list_first_entry(&group->devices, struct group_device, list); 3033 3045 dev = grp_dev->dev; 3034 - get_device(dev); 3035 3046 3036 - /* 3037 - * Don't hold the group mutex because taking group mutex first and then 3038 - * the device lock could potentially cause a deadlock as below. Assume 3039 - * two threads T1 and T2. T1 is trying to change default domain of an 3040 - * iommu group and T2 is trying to hot unplug a device or release [1] VF 3041 - * of a PCIe device which is in the same iommu group. T1 takes group 3042 - * mutex and before it could take device lock assume T2 has taken device 3043 - * lock and is yet to take group mutex. Now, both the threads will be 3044 - * waiting for the other thread to release lock. Below, lock order was 3045 - * suggested. 3046 - * device_lock(dev); 3047 - * mutex_lock(&group->mutex); 3048 - * iommu_change_dev_def_domain(); 3049 - * mutex_unlock(&group->mutex); 3050 - * device_unlock(dev); 3051 - * 3052 - * [1] Typical device release path 3053 - * device_lock() from device/driver core code 3054 - * -> bus_notifier() 3055 - * -> iommu_bus_notifier() 3056 - * -> iommu_release_device() 3057 - * -> ops->release_device() vendor driver calls back iommu core code 3058 - * -> mutex_lock() from iommu core code 3059 - */ 3060 - mutex_unlock(&group->mutex); 3061 - 3062 - /* Check if the device in the group still has a driver bound to it */ 3063 - device_lock(dev); 3064 - if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ && 3065 - group->default_domain->type == IOMMU_DOMAIN_DMA)) { 3066 - pr_err_ratelimited("Device is still bound to driver\n"); 3067 - ret = -EBUSY; 3068 - goto out; 3069 - } 3070 - 3071 - mutex_lock(&group->mutex); 3072 3047 ret = iommu_change_dev_def_domain(group, dev, req_type); 3048 + 3073 3049 /* 3074 3050 * Release the mutex here because ops->probe_finalize() call-back of 3075 3051 * some vendor IOMMU drivers calls arm_iommu_attach_device() which ··· 3048 3088 3049 3089 /* Make sure dma_ops is appropriatley set */ 3050 3090 if (!ret) 3051 - iommu_group_do_probe_finalize(dev, group->default_domain); 3052 - ret = ret ?: count; 3091 + __iommu_group_dma_finalize(group); 3053 3092 3054 - out: 3055 - device_unlock(dev); 3056 - put_device(dev); 3057 - 3058 - return ret; 3093 + return ret ?: count; 3059 3094 } 3060 3095 3061 3096 static bool iommu_is_default_domain(struct iommu_group *group)