| Message ID | 20211126232734.531194050@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | genirq/msi, PCI/MSI: Spring cleaning - Part 2 | expand |
On Sat, Nov 27, 2021 at 02:22:33AM +0100, Thomas Gleixner wrote: > In preparation for dynamic handling of MSI-X interrupts provide a new set > of MSI descriptor accessor functions and iterators. They are benefitial per > se as they allow to cleanup quite some code in various MSI domain > implementations. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > include/linux/msi.h | 58 ++++++++++++++++++++++++++++ > kernel/irq/msi.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 165 insertions(+) > > +++ b/include/linux/msi.h > @@ -140,6 +140,18 @@ struct msi_desc { > struct pci_msi_desc pci; > }; > > +/* > + * Filter values for the MSI descriptor iterators and accessor functions. > + */ > +enum msi_desc_filter { > + /* All descriptors */ > + MSI_DESC_ALL, > + /* Descriptors which have no interrupt associated */ > + MSI_DESC_NOTASSOCIATED, > + /* Descriptors which have an interrupt associated */ > + MSI_DESC_ASSOCIATED, > +}; > + > /** > * msi_device_data - MSI per device data > * @lock: Spinlock to protect register access > @@ -148,6 +160,8 @@ struct msi_desc { > * @platform_data: Platform-MSI specific data > * @list: List of MSI descriptors associated to the device > * @mutex: Mutex protecting the MSI list > + * @__next: Cached pointer to the next entry for iterators > + * @__filter: Cached descriptor filter > */ > struct msi_device_data { > raw_spinlock_t lock; > @@ -156,6 +170,8 @@ struct msi_device_data { > struct platform_msi_priv_data *platform_data; > struct list_head list; > struct mutex mutex; > + struct msi_desc *__next; > + enum msi_desc_filter __filter; > }; > > int msi_setup_device_data(struct device *dev); > @@ -193,6 +209,48 @@ static inline unsigned int msi_get_virq( > void msi_lock_descs(struct device *dev); > void msi_unlock_descs(struct device *dev); > > +struct msi_desc *__msi_first_desc(struct device *dev, enum msi_desc_filter filter, unsigned int base_index); > +struct msi_desc *msi_next_desc(struct device *dev); > + > +/** > + * msi_first_desc - Get the first MSI descriptor associated to the device > + * @dev: Device to search > + */ > +static inline struct msi_desc *msi_first_desc(struct device *dev) > +{ > + return __msi_first_desc(dev, MSI_DESC_ALL, 0); > +} > + > + > +/** > + * msi_for_each_desc_from - Iterate the MSI descriptors from a given index > + * > + * @desc: struct msi_desc pointer used as iterator > + * @dev: struct device pointer - device to iterate > + * @filter: Filter for descriptor selection > + * @base_index: MSI index to iterate from > + * > + * Notes: > + * - The loop must be protected with a msi_lock_descs()/msi_unlock_descs() > + * pair. > + * - It is safe to remove a retrieved MSI descriptor in the loop. > + */ > +#define msi_for_each_desc_from(desc, dev, filter, base_index) \ > + for ((desc) = __msi_first_desc((dev), (filter), (base_index)); (desc); \ > + (desc) = msi_next_desc((dev))) Given this ends up as an xarray it feels really weird that there is a hidden shared __next/__iter_idx instead of having the caller provide the index storage as is normal for xa operations. I understand why that isn't desirable at this patch where the storage would have to be a list_head pointer, but still, seems like an odd place to end up at the end of the series. eg add index here unused and then the last patch uses it instead of __iter_idx. Also, I don't understand why filter was stored in the dev and not passed into msi_next_desc() in the macro here? Jason
On Sat, Nov 27 2021 at 21:00, Jason Gunthorpe wrote: > On Sat, Nov 27, 2021 at 02:22:33AM +0100, Thomas Gleixner wrote: >> + * Notes: >> + * - The loop must be protected with a msi_lock_descs()/msi_unlock_descs() >> + * pair. >> + * - It is safe to remove a retrieved MSI descriptor in the loop. >> + */ >> +#define msi_for_each_desc_from(desc, dev, filter, base_index) \ >> + for ((desc) = __msi_first_desc((dev), (filter), (base_index)); (desc); \ >> + (desc) = msi_next_desc((dev))) > > Given this ends up as an xarray it feels really weird that there is a > hidden shared __next/__iter_idx instead of having the caller provide > the index storage as is normal for xa operations. > > I understand why that isn't desirable at this patch where the storage > would have to be a list_head pointer, but still, seems like an odd > place to end up at the end of the series. > > eg add index here unused and then the last patch uses it instead of > __iter_idx. TBH, didn't think about doing just that. OTH, given the experience of looking at the creative mess people create, this was probably also a vain attempt to make it harder in the future. > Also, I don't understand why filter was stored in the dev and not > passed into msi_next_desc() in the macro here? No real reason. I probably just stored it along with the rest. Lemme try that index approach. Thanks, tglx
Jason, On Sun, Nov 28 2021 at 20:22, Thomas Gleixner wrote: > On Sat, Nov 27 2021 at 21:00, Jason Gunthorpe wrote: >> On Sat, Nov 27, 2021 at 02:22:33AM +0100, Thomas Gleixner wrote: >> I understand why that isn't desirable at this patch where the storage >> would have to be a list_head pointer, but still, seems like an odd >> place to end up at the end of the series. >> >> eg add index here unused and then the last patch uses it instead of >> __iter_idx. > > TBH, didn't think about doing just that. OTH, given the experience of > looking at the creative mess people create, this was probably also a > vain attempt to make it harder in the future. > >> Also, I don't understand why filter was stored in the dev and not >> passed into msi_next_desc() in the macro here? > > No real reason. I probably just stored it along with the rest. Lemme try > that index approach. After looking at all the call sites again, there is no real usage for this local index variable. If anything needs the index of a descriptor then it's available in the descriptor itself. That won't change because the low level message write code needs the index too and the only accessible storage there is msi_desc. So the "gain" would be to have a pointless 'unsigned long index;' variable at all usage sites. What for? The usage sites should not have to care about the storage details of a facility they are using. So it might look odd in the first place, but at the end it's conveniant and does not put any restrictions on changing the underlying mechanics. Thanks, tglx
On Mon, Nov 29, 2021 at 10:26:11AM +0100, Thomas Gleixner wrote: > Jason, > > On Sun, Nov 28 2021 at 20:22, Thomas Gleixner wrote: > > On Sat, Nov 27 2021 at 21:00, Jason Gunthorpe wrote: > >> On Sat, Nov 27, 2021 at 02:22:33AM +0100, Thomas Gleixner wrote: > >> I understand why that isn't desirable at this patch where the storage > >> would have to be a list_head pointer, but still, seems like an odd > >> place to end up at the end of the series. > >> > >> eg add index here unused and then the last patch uses it instead of > >> __iter_idx. > > > > TBH, didn't think about doing just that. OTH, given the experience of > > looking at the creative mess people create, this was probably also a > > vain attempt to make it harder in the future. > > > >> Also, I don't understand why filter was stored in the dev and not > >> passed into msi_next_desc() in the macro here? > > > > No real reason. I probably just stored it along with the rest. Lemme try > > that index approach. > > After looking at all the call sites again, there is no real usage for > this local index variable. > > If anything needs the index of a descriptor then it's available in the > descriptor itself. That won't change because the low level message write > code needs the index too and the only accessible storage there is > msi_desc. Oh, that makes it simpler, just use the current desc->index as the input to the xa_for_each_start() and then there should be no need of hidden state? > What for? The usage sites should not have to care about the storage > details of a facility they are using. Generally for_each things shouldn't have hidden state that prevents them from being nested. It is just an unexpected design pattern.. Jason
Jason, On Mon, Nov 29 2021 at 10:01, Jason Gunthorpe wrote: > On Mon, Nov 29, 2021 at 10:26:11AM +0100, Thomas Gleixner wrote: >> After looking at all the call sites again, there is no real usage for >> this local index variable. >> >> If anything needs the index of a descriptor then it's available in the >> descriptor itself. That won't change because the low level message write >> code needs the index too and the only accessible storage there is >> msi_desc. > > Oh, that makes it simpler, just use the current desc->index as the > input to the xa_for_each_start() and then there should be no need of > hidden state? That works for alloc, but on free that's going to end up badly. >> What for? The usage sites should not have to care about the storage >> details of a facility they are using. > > Generally for_each things shouldn't have hidden state that prevents > them from being nested. It is just an unexpected design pattern.. I'm not seeing any sensible use case for: msi_for_each_desc(dev) msi_for_each_desc(dev) If that ever comes forth, I'm happy to debate this further :) Thanks, tglx
--- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -140,6 +140,18 @@ struct msi_desc { struct pci_msi_desc pci; }; +/* + * Filter values for the MSI descriptor iterators and accessor functions. + */ +enum msi_desc_filter { + /* All descriptors */ + MSI_DESC_ALL, + /* Descriptors which have no interrupt associated */ + MSI_DESC_NOTASSOCIATED, + /* Descriptors which have an interrupt associated */ + MSI_DESC_ASSOCIATED, +}; + /** * msi_device_data - MSI per device data * @lock: Spinlock to protect register access @@ -148,6 +160,8 @@ struct msi_desc { * @platform_data: Platform-MSI specific data * @list: List of MSI descriptors associated to the device * @mutex: Mutex protecting the MSI list + * @__next: Cached pointer to the next entry for iterators + * @__filter: Cached descriptor filter */ struct msi_device_data { raw_spinlock_t lock; @@ -156,6 +170,8 @@ struct msi_device_data { struct platform_msi_priv_data *platform_data; struct list_head list; struct mutex mutex; + struct msi_desc *__next; + enum msi_desc_filter __filter; }; int msi_setup_device_data(struct device *dev); @@ -193,6 +209,48 @@ static inline unsigned int msi_get_virq( void msi_lock_descs(struct device *dev); void msi_unlock_descs(struct device *dev); +struct msi_desc *__msi_first_desc(struct device *dev, enum msi_desc_filter filter, unsigned int base_index); +struct msi_desc *msi_next_desc(struct device *dev); + +/** + * msi_first_desc - Get the first MSI descriptor associated to the device + * @dev: Device to search + */ +static inline struct msi_desc *msi_first_desc(struct device *dev) +{ + return __msi_first_desc(dev, MSI_DESC_ALL, 0); +} + + +/** + * msi_for_each_desc_from - Iterate the MSI descriptors from a given index + * + * @desc: struct msi_desc pointer used as iterator + * @dev: struct device pointer - device to iterate + * @filter: Filter for descriptor selection + * @base_index: MSI index to iterate from + * + * Notes: + * - The loop must be protected with a msi_lock_descs()/msi_unlock_descs() + * pair. + * - It is safe to remove a retrieved MSI descriptor in the loop. + */ +#define msi_for_each_desc_from(desc, dev, filter, base_index) \ + for ((desc) = __msi_first_desc((dev), (filter), (base_index)); (desc); \ + (desc) = msi_next_desc((dev))) + +/** + * msi_for_each_desc - Iterate the MSI descriptors + * + * @desc: struct msi_desc pointer used as iterator + * @dev: struct device pointer - device to iterate + * @filter: Filter for descriptor selection + * + * See msi_for_each_desc_from()for further information. + */ +#define msi_for_each_desc(desc, dev, filter) \ + msi_for_each_desc_from(desc, dev, filter, 0) + /* Helpers to hide struct msi_desc implementation details */ #define msi_desc_to_dev(desc) ((desc)->dev) #define dev_to_msi_list(dev) (&(dev)->msi.data->list) --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -142,10 +142,117 @@ void msi_unlock_descs(struct device *dev { if (WARN_ON_ONCE(!dev->msi.data)) return; + /* Clear the next pointer which was cached by the iterator */ + dev->msi.data->__next = NULL; mutex_unlock(&dev->msi.data->mutex); } EXPORT_SYMBOL_GPL(msi_unlock_descs); +static bool msi_desc_match(struct msi_desc *desc, enum msi_desc_filter filter) +{ + switch (filter) { + case MSI_DESC_ALL: + return true; + case MSI_DESC_NOTASSOCIATED: + return !desc->irq; + case MSI_DESC_ASSOCIATED: + return !!desc->irq; + } + WARN_ON_ONCE(1); + return false; +} + +static struct msi_desc *msi_find_first_desc(struct device *dev, enum msi_desc_filter filter, + unsigned int base_index) +{ + struct msi_desc *desc; + + list_for_each_entry(desc, dev_to_msi_list(dev), list) { + if (desc->msi_index < base_index) + continue; + if (msi_desc_match(desc, filter)) + return desc; + } + return NULL; +} + +/** + * __msi_first_desc - Get the first MSI descriptor of a device + * @dev: Device to operate on + * @filter: Descriptor state filter + * @base_index: MSI index to start from for range based operations + * + * Must be called with the MSI descriptor mutex held, i.e. msi_lock_descs() + * must be invoked before the call. + * + * Return: Pointer to the first MSI descriptor matching the search + * criteria, NULL if none found. + */ +struct msi_desc *__msi_first_desc(struct device *dev, enum msi_desc_filter filter, + unsigned int base_index) +{ + struct msi_desc *desc; + + if (WARN_ON_ONCE(!dev->msi.data)) + return NULL; + + lockdep_assert_held(&dev->msi.data->mutex); + + /* Invalidate a previous invocation within the same lock section */ + dev->msi.data->__next = NULL; + + desc = msi_find_first_desc(dev, filter, base_index); + if (desc) { + dev->msi.data->__next = list_next_entry(desc, list); + dev->msi.data->__filter = filter; + } + return desc; +} +EXPORT_SYMBOL_GPL(__msi_first_desc); + +static struct msi_desc *__msi_next_desc(struct device *dev, enum msi_desc_filter filter, + struct msi_desc *from) +{ + struct msi_desc *desc = from; + + list_for_each_entry_from(desc, dev_to_msi_list(dev), list) { + if (msi_desc_match(desc, filter)) + return desc; + } + return NULL; +} + +/** + * msi_next_desc - Get the next MSI descriptor of a device + * @dev: Device to operate on + * + * The first invocation of msi_next_desc() has to be preceeded by a + * successful incovation of __msi_first_desc(). Consecutive invocations are + * only valid if the previous one was successful. All these operations have + * to be done within the same MSI mutex held region. + * + * Return: Pointer to the next MSI descriptor matching the search + * criteria, NULL if none found. + */ +struct msi_desc *msi_next_desc(struct device *dev) +{ + struct msi_device_data *data = dev->msi.data; + struct msi_desc *desc; + + if (WARN_ON_ONCE(!data)) + return NULL; + + lockdep_assert_held(&data->mutex); + + if (!data->__next) + return NULL; + + desc = __msi_next_desc(dev, data->__filter, data->__next); + dev->msi.data->__next = desc ? list_next_entry(desc, list) : NULL; + return desc; +} +EXPORT_SYMBOL_GPL(msi_next_desc); + /** * __msi_get_virq - Return Linux interrupt number of a MSI interrupt * @dev: Device to operate on
In preparation for dynamic handling of MSI-X interrupts provide a new set of MSI descriptor accessor functions and iterators. They are benefitial per se as they allow to cleanup quite some code in various MSI domain implementations. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- include/linux/msi.h | 58 ++++++++++++++++++++++++++++ kernel/irq/msi.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+)