On 2023/11/19 17:10, Hector Martin wrote:
> On 2023/11/15 23:05, Jason Gunthorpe wrote:
>> Allow fwspec to exist independently from the dev->iommu by providing
>> functions to allow allocating and freeing the raw struct iommu_fwspec.
>>
>> Reflow the existing paths to call the new alloc/dealloc functions.
>>
>> Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
>> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>> ---
>> drivers/iommu/iommu.c | 82 ++++++++++++++++++++++++++++++++-----------
>> include/linux/iommu.h | 11 +++++-
>> 2 files changed, 72 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 18a82a20934d53..86bbb9e75c7e03 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev)
>> struct dev_iommu *param = dev->iommu;
>>
>> dev->iommu = NULL;
>> - if (param->fwspec) {
>> - fwnode_handle_put(param->fwspec->iommu_fwnode);
>> - kfree(param->fwspec);
>> - }
>> + if (param->fwspec)
>> + iommu_fwspec_dealloc(param->fwspec);
>> kfree(param);
>> }
>>
>> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>> return ops;
>> }
>>
>> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
>> + struct device *dev,
>> + struct fwnode_handle *iommu_fwnode)
>> +{
>> + const struct iommu_ops *ops;
>> +
>> + if (fwspec->iommu_fwnode) {
>> + /*
>> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
>> + * case of multiple iommus for one device they must point to the
>> + * same driver, checked via same ops.
>> + */
>> + ops = iommu_ops_from_fwnode(iommu_fwnode);
>
> This carries over a related bug from the original code: If a device has
> two IOMMUs and the first one probes but the second one defers, ops will
> be NULL here and the check will fail with EINVAL.
>
> Adding a check for that case here fixes it:
>
> if (!ops)
> return driver_deferred_probe_check_state(dev);
>
> With that, for the whole series:
>
> Tested-by: Hector Martin <marcan@xxxxxxxxx>
>
> I can't specifically test for the probe races the series intends to fix
> though, since that bug we only hit extremely rarely. I'm just testing
> that nothing breaks.
Actually no, this fix is not sufficient. If the first IOMMU is ready
then the xlate path allocates dev->iommu, which then
__iommu_probe_device takes as a sign that all IOMMUs are ready and does
the device init. Then when the xlate comes along again after suceeding
with the second IOMMU, __iommu_probe_device sees the device is already
in a group and never initializes the second IOMMU, leaving the device
with only one IOMMU.
This patch fixes it, but honestly, at this point I have no idea how to
"properly" fix this. There is *way* too much subtlety in this whole
codepath.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2477dec29740..2e4baf0572e7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2935,6 +2935,12 @@ int iommu_fwspec_of_xlate(struct iommu_fwspec
*fwspec, struct device *dev,
int ret;
ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
+ if (ret == -EPROBE_DEFER) {
+ mutex_lock(&iommu_probe_device_lock);
+ if (dev->iommu)
+ dev_iommu_free(dev);
+ mutex_unlock(&iommu_probe_device_lock);
+ }
if (ret)
return ret;
>
>> + if (fwspec->ops != ops)
>> + return -EINVAL;
>> + return 0;
>> + }
>> +
>> + if (!fwspec->ops) {
>> + ops = iommu_ops_from_fwnode(iommu_fwnode);
>> + if (!ops)
>> + return driver_deferred_probe_check_state(dev);
>> + fwspec->ops = ops;
>> + }
>> +
>> + of_node_get(to_of_node(iommu_fwnode));
>> + fwspec->iommu_fwnode = iommu_fwnode;
>> + return 0;
>> +}
>> +
>> +struct iommu_fwspec *iommu_fwspec_alloc(void)
>> +{
>> + struct iommu_fwspec *fwspec;
>> +
>> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>> + if (!fwspec)
>> + return ERR_PTR(-ENOMEM);
>> + return fwspec;
>> +}
>> +
>> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec)
>> +{
>> + if (!fwspec)
>> + return;
>> +
>> + if (fwspec->iommu_fwnode)
>> + fwnode_handle_put(fwspec->iommu_fwnode);
>> + kfree(fwspec);
>> +}
>> +
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>> const struct iommu_ops *ops)
>> {
>> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> + int ret;
>>
>> if (fwspec)
>> return ops == fwspec->ops ? 0 : -EINVAL;
>> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>> if (!dev_iommu_get(dev))
>> return -ENOMEM;
>>
>> - fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>> - if (!fwspec)
>> - return -ENOMEM;
>> + fwspec = iommu_fwspec_alloc();
>> + if (IS_ERR(fwspec))
>> + return PTR_ERR(fwspec);
>>
>> - of_node_get(to_of_node(iommu_fwnode));
>> - fwspec->iommu_fwnode = iommu_fwnode;
>> fwspec->ops = ops;
>> + ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
>> + if (ret) {
>> + iommu_fwspec_dealloc(fwspec);
>> + return ret;
>> + }
>> +
>> dev_iommu_fwspec_set(dev, fwspec);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(iommu_fwspec_init);
>>
>> -void iommu_fwspec_free(struct device *dev)
>> -{
>> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> -
>> - if (fwspec) {
>> - fwnode_handle_put(fwspec->iommu_fwnode);
>> - kfree(fwspec);
>> - dev_iommu_fwspec_set(dev, NULL);
>> - }
>> -}
>> -EXPORT_SYMBOL_GPL(iommu_fwspec_free);
>>
>> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
>> {
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index e98a4ca8f536b7..c7c68cb59aa4dc 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -813,9 +813,18 @@ struct iommu_sva {
>> struct iommu_domain *domain;
>> };
>>
>> +struct iommu_fwspec *iommu_fwspec_alloc(void);
>> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec);
>> +
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>> const struct iommu_ops *ops);
>> -void iommu_fwspec_free(struct device *dev);
>> +static inline void iommu_fwspec_free(struct device *dev)
>> +{
>> + if (!dev->iommu)
>> + return;
>> + iommu_fwspec_dealloc(dev->iommu->fwspec);
>> + dev->iommu->fwspec = NULL;
>> +}
>> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>> const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>>
>
> - Hector
>
>
- Hector
