Skip to contentSkip to conversation

PCI: hv: cleanup patches

PATCH Applied Stephen Hemminger 2 participants 5 messages May 23, 2018 – May 24, 2018 PCI

lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

cover PCI: hv: cleanup patches

These are minor code cleanups found while reviewing and implementing other things in Hyper-V PCI host driver.

Stephen Hemminger (3):
PCI: hv: remove unused reason for refcount handler
PCI: hv: convert remove_lock to refcount
PCI: hv: use list_for_each_entry

 drivers/pci/host/pci-hyperv.c | 105 ++++++++++++----------------------
 1 file changed, 37 insertions(+), 68 deletions(-)
Signature · 1 line
2.17.0
author
#1 lore raw

PATCH 1/3 PCI: hv: remove unused reason for refcount handler

The get/put functions were taking a reason code. This appears to be a debug infrastructure that is no longer used.

Move the functions to start of file to eliminate need for forward declaration. Forward declarations are discouraged on Linux.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/pci/host/pci-hyperv.c | 70 +++++++++++++----------------------
 1 file changed, 26 insertions(+), 44 deletions(-)
drivers/pci/host/pci-hyperv.c +26 -44
index 50cdefe3f6d3..505453e23c22 100644
@@ -488,17 +488,6 @@ enum hv_pcichild_state {
 	hv_pcichild_maximum
 };
 
-enum hv_pcidev_ref_reason {
-	hv_pcidev_ref_invalid = 0,
-	hv_pcidev_ref_initial,
-	hv_pcidev_ref_by_slot,
-	hv_pcidev_ref_packet,
-	hv_pcidev_ref_pnp,
-	hv_pcidev_ref_childlist,
-	hv_pcidev_irqdata,
-	hv_pcidev_ref_max
-};
-
 struct hv_pci_dev {
 	/* List protected by pci_rescan_remove_lock */
 	struct list_head list_entry;

@@ -548,10 +537,17 @@ static void hv_pci_generic_compl(void *context, struct pci_response *resp,
 
 static struct hv_pci_dev *get_pcichild_wslot(struct hv_pcibus_device *hbus,
 						u32 wslot);
-static void get_pcichild(struct hv_pci_dev *hv_pcidev,
-			 enum hv_pcidev_ref_reason reason);
-static void put_pcichild(struct hv_pci_dev *hv_pcidev,
-			 enum hv_pcidev_ref_reason reason);
+
+static void get_pcichild(struct hv_pci_dev *hpdev)
+{
+	refcount_inc(&hpdev->refs);
+}
+
+static void put_pcichild(struct hv_pci_dev *hpdev)
+{
+	if (refcount_dec_and_test(&hpdev->refs))
+		kfree(hpdev);
+}
 
 static void get_hvpcibus(struct hv_pcibus_device *hv_pcibus);
 static void put_hvpcibus(struct hv_pcibus_device *hv_pcibus);

@@ -762,7 +758,7 @@ static int hv_pcifront_read_config(struct pci_bus *bus, unsigned int devfn,
 
 	_hv_pcifront_read_config(hpdev, where, size, val);
 
-	put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+	put_pcichild(hpdev);
 	return PCIBIOS_SUCCESSFUL;
 }
 

@@ -790,7 +786,7 @@ static int hv_pcifront_write_config(struct pci_bus *bus, unsigned int devfn,
 
 	_hv_pcifront_write_config(hpdev, where, size, val);
 
-	put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+	put_pcichild(hpdev);
 	return PCIBIOS_SUCCESSFUL;
 }
 

@@ -856,7 +852,7 @@ static void hv_msi_free(struct irq_domain *domain, struct msi_domain_info *info,
 	}
 
 	hv_int_desc_free(hpdev, int_desc);
-	put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+	put_pcichild(hpdev);
 }
 
 static int hv_set_affinity(struct irq_data *data, const struct cpumask *dest,

@@ -1186,13 +1182,13 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	msg->address_lo = comp.int_desc.address & 0xffffffff;
 	msg->data = comp.int_desc.data;
 
-	put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+	put_pcichild(hpdev);
 	return;
 
 free_int_desc:
 	kfree(int_desc);
 drop_reference:
-	put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+	put_pcichild(hpdev);
 return_null_message:
 	msg->address_hi = 0;
 	msg->address_lo = 0;

@@ -1508,19 +1504,6 @@ static void q_resource_requirements(void *context, struct pci_response *resp,
 	complete(&completion->host_event);
 }
 
-static void get_pcichild(struct hv_pci_dev *hpdev,
-			    enum hv_pcidev_ref_reason reason)
-{
-	refcount_inc(&hpdev->refs);
-}
-
-static void put_pcichild(struct hv_pci_dev *hpdev,
-			    enum hv_pcidev_ref_reason reason)
-{
-	if (refcount_dec_and_test(&hpdev->refs))
-		kfree(hpdev);
-}
-
 /**
  * new_pcichild_device() - Create a new child device
  * @hbus:	The internal struct tracking this root PCI bus.

@@ -1572,7 +1555,7 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
 
 	hpdev->desc = *desc;
 	refcount_set(&hpdev->refs, 1);
-	get_pcichild(hpdev, hv_pcidev_ref_childlist);
+	get_pcichild(hpdev);
 	spin_lock_irqsave(&hbus->device_list_lock, flags);
 
 	/*

@@ -1618,7 +1601,7 @@ static struct hv_pci_dev *get_pcichild_wslot(struct hv_pcibus_device *hbus,
 	list_for_each_entry(iter, &hbus->children, list_entry) {
 		if (iter->desc.win_slot.slot == wslot) {
 			hpdev = iter;
-			get_pcichild(hpdev, hv_pcidev_ref_by_slot);
+			get_pcichild(hpdev);
 			break;
 		}
 	}

@@ -1735,7 +1718,7 @@ static void pci_devices_present_work(struct work_struct *work)
 					     list_entry);
 			if (hpdev->reported_missing) {
 				found = true;
-				put_pcichild(hpdev, hv_pcidev_ref_childlist);
+				put_pcichild(hpdev);
 				list_move_tail(&hpdev->list_entry, &removed);
 				break;
 			}

@@ -1748,7 +1731,7 @@ static void pci_devices_present_work(struct work_struct *work)
 		hpdev = list_first_entry(&removed, struct hv_pci_dev,
 					 list_entry);
 		list_del(&hpdev->list_entry);
-		put_pcichild(hpdev, hv_pcidev_ref_initial);
+		put_pcichild(hpdev);
 	}
 
 	switch (hbus->state) {

@@ -1883,8 +1866,8 @@ static void hv_eject_device_work(struct work_struct *work)
 			 sizeof(*ejct_pkt), (unsigned long)&ctxt.pkt,
 			 VM_PKT_DATA_INBAND, 0);
 
-	put_pcichild(hpdev, hv_pcidev_ref_childlist);
-	put_pcichild(hpdev, hv_pcidev_ref_pnp);
+	put_pcichild(hpdev);
+	put_pcichild(hpdev);
 	put_hvpcibus(hpdev->hbus);
 }
 

@@ -1899,7 +1882,7 @@ static void hv_eject_device_work(struct work_struct *work)
 static void hv_pci_eject_device(struct hv_pci_dev *hpdev)
 {
 	hpdev->state = hv_pcichild_ejecting;
-	get_pcichild(hpdev, hv_pcidev_ref_pnp);
+	get_pcichild(hpdev);
 	INIT_WORK(&hpdev->wrk, hv_eject_device_work);
 	get_hvpcibus(hpdev->hbus);
 	queue_work(hpdev->hbus->wq, &hpdev->wrk);

@@ -1999,8 +1982,7 @@ static void hv_pci_onchannelcallback(void *context)
 						      dev_message->wslot.slot);
 				if (hpdev) {
 					hv_pci_eject_device(hpdev);
-					put_pcichild(hpdev,
-							hv_pcidev_ref_by_slot);
+					put_pcichild(hpdev);
 				}
 				break;
 

@@ -2398,7 +2380,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
 				PCI_RESOURCES_ASSIGNED2;
 			res_assigned2->wslot.slot = hpdev->desc.win_slot.slot;
 		}
-		put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+		put_pcichild(hpdev);
 
 		ret = vmbus_sendpacket(hdev->channel, &pkt->message,
 				size_res, (unsigned long)pkt,

@@ -2446,7 +2428,7 @@ static int hv_send_resources_released(struct hv_device *hdev)
 		pkt.message_type.type = PCI_RESOURCES_RELEASED;
 		pkt.wslot.slot = hpdev->desc.win_slot.slot;
 
-		put_pcichild(hpdev, hv_pcidev_ref_by_slot);
+		put_pcichild(hpdev);
 
 		ret = vmbus_sendpacket(hdev->channel, &pkt, sizeof(pkt), 0,
 				       VM_PKT_DATA_INBAND, 0);
Signature · 1 line
2.17.0
author
#2 lore raw

PATCH 2/3 PCI: hv: convert remove_lock to refcount

In reply to Stephen Hemminger

Use refcount instead of atomic for the reference counting on bus. Refcount is safer because it handles overflow correctly.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/pci/host/pci-hyperv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pci/host/pci-hyperv.c +4 -4
index 505453e23c22..19eb47f58ccb 100644
@@ -433,7 +433,7 @@ enum hv_pcibus_state {
 struct hv_pcibus_device {
 	struct pci_sysdata sysdata;
 	enum hv_pcibus_state state;
-	atomic_t remove_lock;
+	refcount_t remove_lock;
 	struct hv_device *hdev;
 	resource_size_t low_mmio_space;
 	resource_size_t high_mmio_space;

@@ -2441,12 +2441,12 @@ static int hv_send_resources_released(struct hv_device *hdev)
 
 static void get_hvpcibus(struct hv_pcibus_device *hbus)
 {
-	atomic_inc(&hbus->remove_lock);
+	refcount_inc(&hbus->remove_lock);
 }
 
 static void put_hvpcibus(struct hv_pcibus_device *hbus)
 {
-	if (atomic_dec_and_test(&hbus->remove_lock))
+	if (refcount_dec_and_test(&hbus->remove_lock))
 		complete(&hbus->remove_event);
 }
 

@@ -2490,7 +2490,7 @@ static int hv_pci_probe(struct hv_device *hdev,
 			       hdev->dev_instance.b[8] << 8;
 
 	hbus->hdev = hdev;
-	atomic_inc(&hbus->remove_lock);
+	refcount_set(&hbus->remove_lock, 1);
 	INIT_LIST_HEAD(&hbus->children);
 	INIT_LIST_HEAD(&hbus->dr_list);
 	INIT_LIST_HEAD(&hbus->resources_for_children);
Signature · 1 line
2.17.0
author
#3 lore raw

PATCH 3/3 PCI: hv: use list_for_each_entry

In reply to Stephen Hemminger

There are several places where list_for_each_entry could be used to simplify the code.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/pci/host/pci-hyperv.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
drivers/pci/host/pci-hyperv.c +7 -20
index 19eb47f58ccb..afc30dee6001 100644
@@ -1279,7 +1279,6 @@ static u64 get_bar_size(u64 bar_val)
  */
 static void survey_child_resources(struct hv_pcibus_device *hbus)
 {
-	struct list_head *iter;
 	struct hv_pci_dev *hpdev;
 	resource_size_t bar_size = 0;
 	unsigned long flags;

@@ -1305,8 +1304,7 @@ static void survey_child_resources(struct hv_pcibus_device *hbus)
 	 * for a child device are a power of 2 in size and aligned in memory,
 	 * so it's sufficient to just add them up without tracking alignment.
 	 */
-	list_for_each(iter, &hbus->children) {
-		hpdev = container_of(iter, struct hv_pci_dev, list_entry);
+	list_for_each_entry(hpdev, &hbus->children, list_entry) {
 		for (i = 0; i < 6; i++) {
 			if (hpdev->probed_bar[i] & PCI_BASE_ADDRESS_SPACE_IO)
 				dev_err(&hbus->hdev->device,

@@ -1359,7 +1357,6 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
 	resource_size_t low_base = 0;
 	resource_size_t bar_size;
 	struct hv_pci_dev *hpdev;
-	struct list_head *iter;
 	unsigned long flags;
 	u64 bar_val;
 	u32 command;

@@ -1381,9 +1378,7 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
 
 	/* Pick addresses for the BARs. */
 	do {
-		list_for_each(iter, &hbus->children) {
-			hpdev = container_of(iter, struct hv_pci_dev,
-					     list_entry);
+		list_for_each_entry(hpdev, &hbus->children, list_entry) {
 			for (i = 0; i < 6; i++) {
 				bar_val = hpdev->probed_bar[i];
 				if (bar_val == 0)

@@ -1637,7 +1632,6 @@ static void pci_devices_present_work(struct work_struct *work)
 {
 	u32 child_no;
 	bool found;
-	struct list_head *iter;
 	struct pci_function_description *new_desc;
 	struct hv_pci_dev *hpdev;
 	struct hv_pcibus_device *hbus;

@@ -1674,10 +1668,8 @@ static void pci_devices_present_work(struct work_struct *work)
 
 	/* First, mark all existing children as reported missing. */
 	spin_lock_irqsave(&hbus->device_list_lock, flags);
-	list_for_each(iter, &hbus->children) {
-			hpdev = container_of(iter, struct hv_pci_dev,
-					     list_entry);
-			hpdev->reported_missing = true;
+	list_for_each_entry(hpdev, &hbus->children, list_entry) {
+		hpdev->reported_missing = true;
 	}
 	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
 

@@ -1687,11 +1679,8 @@ static void pci_devices_present_work(struct work_struct *work)
 		new_desc = &dr->func[child_no];
 
 		spin_lock_irqsave(&hbus->device_list_lock, flags);
-		list_for_each(iter, &hbus->children) {
-			hpdev = container_of(iter, struct hv_pci_dev,
-					     list_entry);
-			if ((hpdev->desc.win_slot.slot ==
-			     new_desc->win_slot.slot) &&
+		list_for_each_entry(hpdev, &hbus->children, list_entry) {
+			if ((hpdev->desc.win_slot.slot == new_desc->win_slot.slot) &&
 			    (hpdev->desc.v_id == new_desc->v_id) &&
 			    (hpdev->desc.d_id == new_desc->d_id) &&
 			    (hpdev->desc.ser == new_desc->ser)) {

@@ -1713,9 +1702,7 @@ static void pci_devices_present_work(struct work_struct *work)
 	spin_lock_irqsave(&hbus->device_list_lock, flags);
 	do {
 		found = false;
-		list_for_each(iter, &hbus->children) {
-			hpdev = container_of(iter, struct hv_pci_dev,
-					     list_entry);
+		list_for_each_entry(hpdev, &hbus->children, list_entry) {
 			if (hpdev->reported_missing) {
 				found = true;
 				put_pcichild(hpdev);
Signature · 1 line
2.17.0
Applied
#4 lore raw
In reply to Stephen Hemminger
Quoted 10 lines · Stephen Hemminger

These are minor code cleanups found while reviewing and implementing other things in Hyper-V PCI host driver.

Stephen Hemminger (3):
PCI: hv: remove unused reason for refcount handler
PCI: hv: convert remove_lock to refcount
PCI: hv: use list_for_each_entry

 drivers/pci/host/pci-hyperv.c | 105 ++++++++++++----------------------
 1 file changed, 37 insertions(+), 68 deletions(-)

Applied to pci/hv for v4.18, thanks.

Lorenzo