diff mbox series

[12/20] genirq/msi: Make descriptor freeing domain aware

Message ID 20221111132706.726275059@linutronix.de
State New
Headers
Series [01/20] genirq/msi: Move IRQ_DOMAIN_MSI_NOMASK_QUIRK to MSI flags |

Commit Message

Thomas Gleixner Nov. 11, 2022, 1:56 p.m. UTC 
  Change the descriptor free functions to take a domain id to prepare for the
upcoming multi MSI domain per device support.

To avoid changing and extending the interfaces over and over use an core
internal control struct and hand the pointer through the various functions.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h |   22 ++++++++++++++++---
 kernel/irq/msi.c    |   60 ++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 68 insertions(+), 14 deletions(-)

Comments

Tian, Kevin Nov. 18, 2022, 8:17 a.m. UTC | #1 
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
>  int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
>  			       struct msi_desc *init_desc);
>  /**
> - * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
> default domain
> + * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
> default irqdomain
> + *

belong to last patch

> +/**
> + * struct msi_ctrl - MSI internal management control structure
> + * @domid:	ID of the domain on which management operations should
> be done
> + * @first:	First (hardware) slot index to operate on
> + * @last:	Last (hardware) slot index to operate on
> + */
> +struct msi_ctrl {
> +	unsigned int			domid;
> +	unsigned int			first;
> +	unsigned int			last;
> +};
> +

this really contains the range information. what about msi_range and
then msi_range_valid()?

> +static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
>  {
>  	struct xarray *xa = &dev->msi.data->__store;
>  	struct msi_desc *desc;
>  	unsigned long idx;
> +	int base;
> +
> +	lockdep_assert_held(&dev->msi.data->mutex);
> 
> -	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
> MSI_MAX_INDEX))
> +	if (!msi_ctrl_valid(dev, ctrl))
>  		return;
> 
> -	lockdep_assert_held(&dev->msi.data->mutex);
> +	base = msi_get_domain_base_index(dev, ctrl->domid);
> +	if (base < 0)
> +		return;

What about putting domid checks in msi_ctrl_valid() then here could
be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.

domid is part of msi_ctrl. then it sound reasonable to validate it
together with first/last.
Thomas Gleixner Nov. 18, 2022, 12:22 p.m. UTC | #2 
On Fri, Nov 18 2022 at 08:17, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>>  /**
>> - * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
>> default domain
>> + * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
>> default irqdomain
>> + *
>
> belong to last patch

Yes.

>> +/**
>> + * struct msi_ctrl - MSI internal management control structure
>> + * @domid:	ID of the domain on which management operations should
>> be done
>> + * @first:	First (hardware) slot index to operate on
>> + * @last:	Last (hardware) slot index to operate on
>> + */
>> +struct msi_ctrl {
>> +	unsigned int			domid;
>> +	unsigned int			first;
>> +	unsigned int			last;
>> +};
>> +
>
> this really contains the range information. what about msi_range and
> then msi_range_valid()?

It's range plus domain id and later it gains nirqs. So its awkward in
any case.

>> +static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
>>  {
>>  	struct xarray *xa = &dev->msi.data->__store;
>>  	struct msi_desc *desc;
>>  	unsigned long idx;
>> +	int base;
>> +
>> +	lockdep_assert_held(&dev->msi.data->mutex);
>> 
>> -	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
>> MSI_MAX_INDEX))
>> +	if (!msi_ctrl_valid(dev, ctrl))
>>  		return;
>> 
>> -	lockdep_assert_held(&dev->msi.data->mutex);
>> +	base = msi_get_domain_base_index(dev, ctrl->domid);
>> +	if (base < 0)
>> +		return;
>
> What about putting domid checks in msi_ctrl_valid() then here could
> be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.
>
> domid is part of msi_ctrl. then it sound reasonable to validate it
> together with first/last.

Let me look at that.
Thomas Gleixner Nov. 18, 2022, 1:10 p.m. UTC | #3 
On Fri, Nov 18 2022 at 13:22, Thomas Gleixner wrote:
> On Fri, Nov 18 2022 at 08:17, Kevin Tian wrote:
>>> -	lockdep_assert_held(&dev->msi.data->mutex);
>>> +	base = msi_get_domain_base_index(dev, ctrl->domid);
>>> +	if (base < 0)
>>> +		return;
>>
>> What about putting domid checks in msi_ctrl_valid() then here could
>> be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.

I need the base domain index w/o the ctrl thing too.

>> domid is part of msi_ctrl. then it sound reasonable to validate it
>> together with first/last.
>
> Let me look at that.

So I kept it as is, but renamed msi_ctrl_valid() to
msi_ctrl_range_valid().

Thanks,

        tglx
diff mbox series

Patch

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -277,7 +277,8 @@  static inline void msi_desc_set_iommu_co
 int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
 			       struct msi_desc *init_desc);
 /**
- * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the default domain
+ * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the default irqdomain
+ *
  * @dev:	Pointer to the device for which the descriptor is allocated
  * @init_desc:	Pointer to an MSI descriptor to initialize the new descriptor
  *
@@ -288,10 +289,25 @@  static inline int msi_insert_msi_desc(st
 	return msi_domain_insert_msi_desc(dev, MSI_DEFAULT_DOMAIN, init_desc);
 }
 
-void msi_free_msi_descs_range(struct device *dev, unsigned int first_index, unsigned int last_index);
+void msi_domain_free_msi_descs_range(struct device *dev, unsigned int domid,
+				     unsigned int first, unsigned int last);
+
+/**
+ * msi_free_msi_descs_range - Free a range of MSI descriptors of a device
+ *			      in the default irqdomain
+ *
+ * @dev:	Device for which to free the descriptors
+ * @first:	Index to start freeing from (inclusive)
+ * @last:	Last index to be freed (inclusive)
+ */
+static inline void msi_free_msi_descs_range(struct device *dev, unsigned int first,
+					    unsigned int last)
+{
+	msi_domain_free_msi_descs_range(dev, MSI_DEFAULT_DOMAIN, first, last);
+}
 
 /**
- * msi_free_msi_descs - Free MSI descriptors of a device
+ * msi_free_msi_descs - Free all MSI descriptors of a device in the default irqdomain
  * @dev:	Device to free the descriptors
  */
 static inline void msi_free_msi_descs(struct device *dev)
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -19,6 +19,18 @@ 
 
 #include "internals.h"
 
+/**
+ * struct msi_ctrl - MSI internal management control structure
+ * @domid:	ID of the domain on which management operations should be done
+ * @first:	First (hardware) slot index to operate on
+ * @last:	Last (hardware) slot index to operate on
+ */
+struct msi_ctrl {
+	unsigned int			domid;
+	unsigned int			first;
+	unsigned int			last;
+};
+
 static inline int msi_sysfs_create_group(struct device *dev);
 
 /* Invalid XA index which is outside of any searchable range */
@@ -189,25 +201,32 @@  static bool msi_desc_match(struct msi_de
 	return false;
 }
 
-/**
- * msi_free_msi_descs_range - Free MSI descriptors of a device
- * @dev:		Device to free the descriptors
- * @first_index:	Index to start freeing from
- * @last_index:		Last index to be freed
- */
-void msi_free_msi_descs_range(struct device *dev, unsigned int first_index,
-			      unsigned int last_index)
+static bool msi_ctrl_valid(struct device *dev, struct msi_ctrl *ctrl)
+{
+	if (WARN_ON_ONCE(ctrl->first > ctrl->last ||
+			 ctrl->first >= MSI_MAX_INDEX ||
+			 ctrl->last >= MSI_MAX_INDEX))
+		return false;
+	return true;
+}
+
+static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
 {
 	struct xarray *xa = &dev->msi.data->__store;
 	struct msi_desc *desc;
 	unsigned long idx;
+	int base;
+
+	lockdep_assert_held(&dev->msi.data->mutex);
 
-	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >= MSI_MAX_INDEX))
+	if (!msi_ctrl_valid(dev, ctrl))
 		return;
 
-	lockdep_assert_held(&dev->msi.data->mutex);
+	base = msi_get_domain_base_index(dev, ctrl->domid);
+	if (base < 0)
+		return;
 
-	xa_for_each_range(xa, idx, desc, first_index, last_index) {
+	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
 		xa_erase(xa, idx);
 
 		/* Leak the descriptor when it is still referenced */
@@ -217,6 +236,25 @@  void msi_free_msi_descs_range(struct dev
 	}
 }
 
+/**
+ * msi_domain_free_msi_descs_range - Free a range of MSI descriptors of a device in an irqdomain
+ * @dev:	Device for which to free the descriptors
+ * @domid:	Id of the domain to operate on
+ * @first:	Index to start freeing from (inclusive)
+ * @last:	Last index to be freed (inclusive)
+ */
+void msi_domain_free_msi_descs_range(struct device *dev, unsigned int domid,
+				     unsigned int first, unsigned int last)
+{
+	struct msi_ctrl ctrl = {
+		.domid	= domid,
+		.first	= first,
+		.last	= last,
+	};
+
+	msi_domain_free_descs(dev, &ctrl);
+}
+
 void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	*msg = entry->msg;