linux-kernel - Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free routines

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:	Sun, 10 Jul 2016 05:47:37 +0200
From:	Christoph Hellwig <hch@....de>
To:	Alexander Gordeev <agordeev@...hat.com>
Cc:	Christoph Hellwig <hch@....de>, tglx@...utronix.de, axboe@...com,
	linux-block@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free
	routines

On Wed, Jul 06, 2016 at 10:05:45AM +0200, Alexander Gordeev wrote:
> > + pci_enable_msi, pci_enable_msi_range, pci_enable_msi_exact, pci_disable_msi,
> > + pci_msi_vec_count, pci_enable_msix_range, pci_enable_msix_exact,
> > + pci_disable_msix, pci_msix_vec_count
> 
> Description of these functions can be removed when all drivers migrated
> to the new API. Also implementation descriptions + examples would still
> be needed AFAICT.

I diagreed - if we deprecated functions the only thing that should
be mentioned is a "don't use these". 

> This function's code almost matches the existing pci_enable_msix_range()
> so pci_enable_msix_range() should be reworked instead IMHO.

That's what earlier versions of the code did.  However due to the
fact that we want to avoid over-allocating the msix_vectors array
(minor) and get the vectors count of the affinity mask right (major,
as pointed out by you last time) I had to move the allocations inside
the helpers that loop around the atctual enablement.  I didn't want
to change the function to a different version of the algorithm just
before removing them relatively soon.  But given that strong preference
for changing these simple functions instead of duplicating them I've
changed that patch to do that now.

> We do not need to keep msix_entry array, since it only needed for
> pci_irq_vector() function. But the same info could be retrieved from
> msi_desc::irq.

Indeed.  Avoiding this allocation makes these interfaces quite a bit
simpler.  It requires a few prep patches, but I think it's definitively
worth, so the next version will avoid the need for the msix_entry array.

> > +	/* use legacy irq if allowed */
> > +	if (min_vecs == 1)
> > +		return 1;
> > +	return -ENOSPC;
> 
> The original error code (in vecs) would be overridden with -ENOSPC here.

Ok, fixed.

> > +	WARN_ON_ONCE(!dev->msi_enabled && nr > 0);
> > +	return dev->irq + nr;
> 
> I think this function should check irq number existence and return the
> vector number or -EINVAL;

Ok, fixed.

> > +		unsigned int flags)
> > +{
> > +	if (min_vecs > 1)
> > +		return -ENOSPC;
> 
> In case CONFIG_PCI_MSI is unset min_vecs > 1 is -EINVAL;

Ok, fixed.