Re: [RFC PATCH v1 1/3] PCI: vmd: Clean up vmd_enable_domain function

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Oct 25, 2024 at 05:01:51PM +0200, Szymon Durawa wrote:
> This function is too long and needs to be shortened to make it more readable.

Use the actual function name here instead of "This function".

Include "()" after function names, including in the subject line.

> +enum vmd_resource {
> +	VMD_RES_CFGBAR = 0,
> +	VMD_RES_MBAR_1, /*VMD Resource MemBAR 1 */
> +	VMD_RES_MBAR_2, /*VMD Resource MemBAR 2 */

Add space after "/*".

> @@ -132,10 +144,10 @@ struct vmd_dev {
>  	struct vmd_irq_list	*irqs;
>  
>  	struct pci_sysdata	sysdata;
> -	struct resource		resources[3];
> +	struct resource		resources[VMD_RES_COUNT];
>  	struct irq_domain	*irq_domain;
> -	struct pci_bus		*bus;
> -	u8			busn_start;
> +	struct pci_bus		*bus[VMD_BUS_COUNT];
> +	u8			busn_start[VMD_BUS_COUNT];

This looks like a combination of things that don't logically need to
be in the same patch, e.g., adding VMD_RES_COUNT (basically cosmetic),
converting "bus" from a scalar to an array (possibly new
functionality?)

> -static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> +static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>  {
> -	struct pci_sysdata *sd = &vmd->sysdata;
> -	struct resource *res;
> +	struct resource *res = &vmd->dev->resource[VMD_CFGBAR];
> +
> +	vmd->resources[VMD_RES_CFGBAR] = (struct resource){
> +		.name = "VMD CFGBAR",
> +		.start = vmd->busn_start[VMD_BUS_0],
> +		.end = vmd->busn_start[VMD_BUS_0] +
> +		       (resource_size(res) >> 20) - 1,
> +		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> +	};
> +}

This might need to be split into several patches, each of which moves
some code from vmd_enable_domain() into a helper function, to make
this easily reviewable.  I think generally these are simple, obvious
changes, but when they're all collected together in a single patch,
it's hard to tell that.

Bjorn