Re: [PATCH] PCI: dwc: Separate MSI out to different controller

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jan 15, 2025 at 08:32:15AM +0000, Daniel Tsai wrote:
> From: Tsai Sung-Fu <danielsftsai@google.com>
> 
> Setup the struct irq_affinity at EP side, and passing that as 1 of the
> function parameter when endpoint calls pci_alloc_irq_vectors_affinity,
> this could help to setup non-default irq_affinity for target irq (end up
> in irq_desc->irq_common_data.affinity), and we can make use of that to
> separate msi vector out to bind to other msi controller.
> 
> In current design, we have 8 msi controllers, and each of them own up to
> 32 msi vectors, layout as below
> 
> msi_controller0 <- msi_vector0 ~ 31
> msi_controller1 <- msi_vector32 ~ 63
> msi_controller2 <- msi_vector64 ~ 95
> .
> .
> .
> msi_controller7 <- msi_vector224 ~ 255
> 
> dw_pcie_irq_domain_alloc is allocating msi vector number in a contiguous
> fashion, that would end up those allocated msi vectors all handled by
> the same msi_controller, which all of them would have irq affinity in
> equal. To separate out to different CPU, we need to distribute msi
> vectors to different msi_controller, which require to allocate the msi
> vector in a stride fashion.
> 
> To do that, the CL make use the cpumask_var_t setup by the endpoint,
> compare against to see if the affinities are the same, if they are,
> bind to msi controller which previously allocated msi vector goes to, if
> they are not, assign to new msi controller.

It's crunch time right now getting ready for the merge window, but
some superficial things you can address when you get more detailed
feedback later:

Add "()" after function names.

s/EP/Endpoint/ at least the first time it's used
s/msi/MSI/ in English text
s/irq/IRQ/
s/the CL make use/use/ ("CL" looks like a Google-ism)

> +	 * All IRQs on a given controller will use the same parent interrupt,
> +	 * and therefore the same CPU affinity. We try to honor any CPU spreading
> +	 * requests by assigning distinct affinity masks to distinct vectors.
> +	 * So instead of always allocating the msi vectors in a contiguous fashion,
> +	 * the algo here honor whoever comes first can bind the msi controller to
> +	 * its irq affinity mask, or compare its cpumask against
> +	 * currently recorded to decide if binding to this msi controller.

Wrap comment to fit in 80 columns like the rest of the file.

s/msi/MSI/
s/irq/IRQ/

> +		 * no msi controller matches, we would error return (no space) and
> +		 * clear those previously allocated bit, because all those msi vectors
> +		 * didn't really successfully allocated, so we shouldn't occupied that
> +		 * position in the bitmap in case other endpoint may still make use of
> +		 * those. An extra step when choosing to not allocate in contiguous
> +		 * fashion.

Similar.

Capitalize beginning of sentence.

Some of these are not quite sentences or have grammatical issues.

Bjorn