Re: [PATCH v3 07/10] iommu: Use right way to retrieve iommu_ops

2022-02-14 Thread Lu Baolu

On 2/15/22 9:46 AM, Lu Baolu wrote:

On 2/14/22 8:49 PM, Joerg Roedel wrote:

On Mon, Feb 14, 2022 at 09:55:35AM +0800, Lu Baolu wrote:

+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+    /*
+ * Assume that valid ops must be installed if iommu_probe_device()
+ * has succeeded. The device ops are essentially for internal use
+ * within the IOMMU subsystem itself, so we should be able to trust
+ * ourselves not to misuse the helper.
+ */
+    WARN_ON(!dev || !dev->iommu || !dev->iommu->iommu_dev ||
+    !dev->iommu->iommu_dev->ops);


There is no need for this WARN_ON, the code will oops anyway when one of
the pointers checked here is NULL.



We really don't need to WARN_ON intermediate null pointers. But I would
argue that we could add a WARN() on null dev->iommu->iommu_dev->ops, so
that callers have no need to check the returned ops.


Oh, sorry! We don't need to check null ops either. That will also result
in a null pointer reference oops in the caller.

So, yes. No need for this WARN_ON().

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 07/10] iommu: Use right way to retrieve iommu_ops

2022-02-14 Thread Lu Baolu

On 2/14/22 8:49 PM, Joerg Roedel wrote:

On Mon, Feb 14, 2022 at 09:55:35AM +0800, Lu Baolu wrote:

+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+   /*
+* Assume that valid ops must be installed if iommu_probe_device()
+* has succeeded. The device ops are essentially for internal use
+* within the IOMMU subsystem itself, so we should be able to trust
+* ourselves not to misuse the helper.
+*/
+   WARN_ON(!dev || !dev->iommu || !dev->iommu->iommu_dev ||
+   !dev->iommu->iommu_dev->ops);


There is no need for this WARN_ON, the code will oops anyway when one of
the pointers checked here is NULL.



We really don't need to WARN_ON intermediate null pointers. But I would
argue that we could add a WARN() on null dev->iommu->iommu_dev->ops, so
that callers have no need to check the returned ops.

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 07/10] iommu: Use right way to retrieve iommu_ops

2022-02-14 Thread Joerg Roedel
On Mon, Feb 14, 2022 at 09:55:35AM +0800, Lu Baolu wrote:
> +static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
> +{
> + /*
> +  * Assume that valid ops must be installed if iommu_probe_device()
> +  * has succeeded. The device ops are essentially for internal use
> +  * within the IOMMU subsystem itself, so we should be able to trust
> +  * ourselves not to misuse the helper.
> +  */
> + WARN_ON(!dev || !dev->iommu || !dev->iommu->iommu_dev ||
> + !dev->iommu->iommu_dev->ops);

There is no need for this WARN_ON, the code will oops anyway when one of
the pointers checked here is NULL.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 07/10] iommu: Use right way to retrieve iommu_ops

2022-02-13 Thread Lu Baolu
The common iommu_ops is hooked to both device and domain. When a helper
has both device and domain pointer, the way to get the iommu_ops looks
messy in iommu core. This sorts out the way to get iommu_ops. The device
related helpers go through device pointer, while the domain related ones
go through domain pointer.

Signed-off-by: Lu Baolu 
Reviewed-by: Christoph Hellwig 
Reviewed-by: Jason Gunthorpe 
---
 include/linux/iommu.h | 14 ++
 drivers/iommu/iommu.c | 20 +++-
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ffa2e88f3d0..eb2684f95018 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -381,6 +381,20 @@ static inline void iommu_iotlb_gather_init(struct 
iommu_iotlb_gather *gather)
};
 }
 
+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+   /*
+* Assume that valid ops must be installed if iommu_probe_device()
+* has succeeded. The device ops are essentially for internal use
+* within the IOMMU subsystem itself, so we should be able to trust
+* ourselves not to misuse the helper.
+*/
+   WARN_ON(!dev || !dev->iommu || !dev->iommu->iommu_dev ||
+   !dev->iommu->iommu_dev->ops);
+
+   return dev->iommu->iommu_dev->ops;
+}
+
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE  1 /* Device added */
 #define IOMMU_GROUP_NOTIFY_DEL_DEVICE  2 /* Pre Device removed */
 #define IOMMU_GROUP_NOTIFY_BIND_DRIVER 3 /* Pre Driver bind */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7cf073c56036..d290d8be6133 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -833,8 +833,10 @@ static int iommu_create_device_direct_mappings(struct 
iommu_group *group,
 static bool iommu_is_attach_deferred(struct iommu_domain *domain,
 struct device *dev)
 {
-   if (domain->ops->is_attach_deferred)
-   return domain->ops->is_attach_deferred(domain, dev);
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+   if (ops->is_attach_deferred)
+   return ops->is_attach_deferred(domain, dev);
 
return false;
 }
@@ -1252,10 +1254,10 @@ int iommu_page_response(struct device *dev,
struct iommu_fault_event *evt;
struct iommu_fault_page_request *prm;
struct dev_iommu *param = dev->iommu;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
-   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 
-   if (!domain || !domain->ops->page_response)
+   if (!ops->page_response)
return -ENODEV;
 
if (!param || !param->fault_param)
@@ -1296,7 +1298,7 @@ int iommu_page_response(struct device *dev,
msg->pasid = 0;
}
 
-   ret = domain->ops->page_response(dev, evt, msg);
+   ret = ops->page_response(dev, evt, msg);
list_del(>list);
kfree(evt);
break;
@@ -1759,10 +1761,10 @@ static int __iommu_group_dma_attach(struct iommu_group 
*group)
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
 {
-   struct iommu_domain *domain = data;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-   if (domain->ops->probe_finalize)
-   domain->ops->probe_finalize(dev);
+   if (ops->probe_finalize)
+   ops->probe_finalize(dev);
 
return 0;
 }
@@ -2020,7 +2022,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 {
-   const struct iommu_ops *ops = domain->ops;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
 
if (ops->is_attach_deferred && ops->is_attach_deferred(domain, dev))
return __iommu_attach_device(domain, dev);
-- 
2.25.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu