Re: [RFC V1 RESEND 5/6] PCI/MSI: Free MSI-X resources by group

[Thomas Gleixner]

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