[v3] PCI: vmd: Assign a number to each VMD controller

Commit Message

brookxu.cn Sept. 15, 2021, 3:07 a.m. UTC

From: Chunguang Xu <[e-mail]>

If the system has multiple VMD controllers, the driver does not assign
a number to each controller, so when analyzing the interrupt through
/proc/interrupts, the names of all controllers are the same, which is
not very convenient for problem analysis. Here, try to assign a number
to each VMD controller.

Signed-off-by: Chunguang Xu <[e-mail]>
---

v3: Update subject line.
v2: Update the commit log.

 drivers/pci/controller/vmd.c | 58 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 13 deletions(-)

Comments

Krzysztof Wilczyński Sept. 16, 2021, 10:57 p.m. UTC | #1

Hi Xu,

Thank you for sending the patch over!

A small nitpick below, so feel free to ignore it.

[...] 
> @@ -769,28 +773,48 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	unsigned long features = (unsigned long) id->driver_data;
>  	struct vmd_dev *vmd;
> -	int err;
> +	int err = 0;
>  
> -	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
> -		return -ENOMEM;
> +	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>  
>  	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
> -	if (!vmd)
> -		return -ENOMEM;
> +	if (!vmd) {
> +		err = -ENOMEM;
> +		goto out;
> +	}

I assume that you changed the above to use the newly added "out" label to
be consistent given that you also have the other label, but since there is
no clean-up to be done here, do we need this additional label?

>  	vmd->dev = dev;
> +	vmd->instance = ida_simple_get(&vmd_instance_ida, 0, 0, GFP_KERNEL);
> +	if (vmd->instance < 0) {
> +		err = vmd->instance;
> +		goto out;
> +	}

Similarly to here to the above, no clean-up to be done, and you could just
return immediately here.

What do you think?

Also, I think we might have lost a "Reviewed-by" from Jon Derrick somewhere
along the way.  Given that you only updated the commit log and the subject
like, it probably still applies (unless Jon would like to give his seal of
approval again).

	Krzysztof

brookxu.cn Sept. 17, 2021, 1:36 a.m. UTC | #2

Krzysztof Wilczyński wrote on 2021/9/17 6:57 上午:
> Hi Xu,
> 
> Thank you for sending the patch over!
> 
> A small nitpick below, so feel free to ignore it.
> 
> [...] 
>> @@ -769,28 +773,48 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  {
>>  	unsigned long features = (unsigned long) id->driver_data;
>>  	struct vmd_dev *vmd;
>> -	int err;
>> +	int err = 0;
>>  
>> -	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
>> -		return -ENOMEM;
>> +	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>>  
>>  	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
>> -	if (!vmd)
>> -		return -ENOMEM;
>> +	if (!vmd) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
> 
> I assume that you changed the above to use the newly added "out" label to
> be consistent given that you also have the other label, but since there is
> no clean-up to be done here, do we need this additional label?
> 
>>  	vmd->dev = dev;
>> +	vmd->instance = ida_simple_get(&vmd_instance_ida, 0, 0, GFP_KERNEL);
>> +	if (vmd->instance < 0) {
>> +		err = vmd->instance;
>> +		goto out;
>> +	}
> 
> Similarly to here to the above, no clean-up to be done, and you could just
> return immediately here.
> 
> What do you think?
> 

Thanks, I think we can do this.

> Also, I think we might have lost a "Reviewed-by" from Jon Derrick somewhere
> along the way.  Given that you only updated the commit log and the subject
> like, it probably still applies (unless Jon would like to give his seal of
> approval again).
> 

Thanks, my mistake here.

> 	Krzysztof
>

Patch

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e3fcdfe..c334396 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -69,6 +69,8 @@  enum vmd_features {
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
 };
 
+static DEFINE_IDA(vmd_instance_ida);
+
 /*
  * Lock for manipulating VMD IRQ lists.
  */
@@ -119,6 +121,8 @@  struct vmd_dev {
 	struct pci_bus		*bus;
 	u8			busn_start;
 	u8			first_vec;
+	char			*name;
+	int			instance;
 };
 
 static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
@@ -599,7 +603,7 @@  static int vmd_alloc_irqs(struct vmd_dev *vmd)
 		INIT_LIST_HEAD(&vmd->irqs[i].irq_list);
 		err = devm_request_irq(&dev->dev, pci_irq_vector(dev, i),
 				       vmd_irq, IRQF_NO_THREAD,
-				       "vmd", &vmd->irqs[i]);
+				       vmd->name, &vmd->irqs[i]);
 		if (err)
 			return err;
 	}
@@ -769,28 +773,48 @@  static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	unsigned long features = (unsigned long) id->driver_data;
 	struct vmd_dev *vmd;
-	int err;
+	int err = 0;
 
-	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
-		return -ENOMEM;
+	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
-	if (!vmd)
-		return -ENOMEM;
+	if (!vmd) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	vmd->dev = dev;
+	vmd->instance = ida_simple_get(&vmd_instance_ida, 0, 0, GFP_KERNEL);
+	if (vmd->instance < 0) {
+		err = vmd->instance;
+		goto out;
+	}
+
+	vmd->name = kasprintf(GFP_KERNEL, "vmd%d", vmd->instance);
+	if (!vmd->name) {
+		err = -ENOMEM;
+		goto out_release_instance;
+	}
+
 	err = pcim_enable_device(dev);
 	if (err < 0)
-		return err;
+		goto out_release_instance;
 
 	vmd->cfgbar = pcim_iomap(dev, VMD_CFGBAR, 0);
-	if (!vmd->cfgbar)
-		return -ENOMEM;
+	if (!vmd->cfgbar) {
+		err = -ENOMEM;
+		goto out_release_instance;
+	}
 
 	pci_set_master(dev);
 	if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
-	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
-		return -ENODEV;
+	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32))) {
+		err = -ENODEV;
+		goto out_release_instance;
+	}
 
 	if (features & VMD_FEAT_OFFSET_FIRST_VECTOR)
 		vmd->first_vec = 1;
@@ -799,11 +823,17 @@  static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	pci_set_drvdata(dev, vmd);
 	err = vmd_enable_domain(vmd, features);
 	if (err)
-		return err;
+		goto out_release_instance;
 
 	dev_info(&vmd->dev->dev, "Bound to PCI domain %04x\n",
 		 vmd->sysdata.domain);
 	return 0;
+
+ out_release_instance:
+	ida_simple_remove(&vmd_instance_ida, vmd->instance);
+	kfree(vmd->name);
+ out:
+	return err;
 }
 
 static void vmd_cleanup_srcu(struct vmd_dev *vmd)
@@ -824,6 +854,8 @@  static void vmd_remove(struct pci_dev *dev)
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	vmd_remove_irq_domain(vmd);
+	ida_simple_remove(&vmd_instance_ida, vmd->instance);
+	kfree(vmd->name);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -848,7 +880,7 @@  static int vmd_resume(struct device *dev)
 	for (i = 0; i < vmd->msix_count; i++) {
 		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
 				       vmd_irq, IRQF_NO_THREAD,
-				       "vmd", &vmd->irqs[i]);
+				       vmd->name, &vmd->irqs[i]);
 		if (err)
 			return err;
 	}