lkml.org 
[lkml]   [2019]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC V1 RESEND 5/6] PCI/MSI: Free MSI-X resources by group
Megha,

On Fri, 21 Jun 2019, Megha Dey wrote:
> +static int free_msi_irqs_grp(struct pci_dev *dev, int group_id)
> +{

> +
> + for_each_pci_msi_entry(entry, dev) {
> + if (entry->group_id == group_id && entry->irq)
> + for (i = 0; i < entry->nvec_used; i++)
> + BUG_ON(irq_has_action(entry->irq + i));

BUG_ON is wrong here. This can and must be handled gracefully.

> + }
> +
> + pci_msi_teardown_msi_irqs_grp(dev, group_id);
> +
> + list_for_each_entry_safe(entry, tmp, msi_list, list) {
> + if (entry->group_id == group_id) {
> + clear_bit(entry->msi_attrib.entry_nr, dev->entry);
> + list_del(&entry->list);
> + free_msi_entry(entry);
> + }
> + }
> +
> + list_for_each_entry_safe(msix_sysfs_entry, tmp_msix, pci_msix, list) {
> + if (msix_sysfs_entry->group_id == group_id) {

Again. Proper group management makes all of that just straight forward and
not yet another special case.

> + msi_attrs = msix_sysfs_entry->msi_irq_group->attrs;
>
> +static void pci_msix_shutdown_grp(struct pci_dev *dev, int group_id)
> +{
> + struct msi_desc *entry;
> + int grp_present = 0;
> +
> + if (pci_dev_is_disconnected(dev)) {
> + dev->msix_enabled = 0;

Huch? What's that? I can't figure out why this is needed and of course it
completely lacks a comment explaining this.

> + return;
> + }
> +
> + /* Return the device with MSI-X masked as initial states */
> + for_each_pci_msi_entry(entry, dev) {
> + if (entry->group_id == group_id) {
> + /* Keep cached states to be restored */
> + __pci_msix_desc_mask_irq(entry, 1);
> + grp_present = 1;
> + }
> + }
> +
> + if (!grp_present) {
> + pci_err(dev, "Group to be disabled not present\n");
> + return;

So you print an error and silently return

> + }
> +}
> +
> +int pci_disable_msix_grp(struct pci_dev *dev, int group_id)
> +{
> + int num_vecs;
> +
> + if (!pci_msi_enable || !dev)
> + return -EINVAL;
> +
> + pci_msix_shutdown_grp(dev, group_id);
> + num_vecs = free_msi_irqs_grp(dev, group_id);

just to call in another function which has to do the same group_id lookup
muck again.

> +
> + return num_vecs;
> +}
> +EXPORT_SYMBOL(pci_disable_msix_grp);

Why is this exposed ?

Thanks,

tglx

\
 
 \ /
  Last update: 2019-06-29 10:09    [W:0.201 / U:2.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site