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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 29 Jun 2019 10:08:31 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Megha Dey <megha.dey@...ux.intel.com>
cc:     bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, marc.zyngier@....com,
        ashok.raj@...el.com, jacob.jun.pan@...ux.intel.com,
        megha.dey@...el.com
Subject: Re: [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