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
