[PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove

[PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove

[Dexuan Cui]

1. We don't really need such a big on-stack buffer when sending
the teardown_packet: vmbus_sendpacket() here only uses
sizeof(struct pci_message).

2. In the hot-remove case (PCI_EJECT), after we send PCI_EJECTION_COMPLETE
to the host, the host will send a RESCIND_CHANNEL message to us and the
host won't access the per-channel ringbuffer any longer, so we needn't
send PCI_RESOURCES_RELEASED/PCI_BUS_D0EXIT to the host, and we shouldn't
expect the host's completion message of PCI_BUS_D0EXIT, which will never
come.

3. We should send PCI_BUS_D0EXIT after hv_send_resources_released().

Signed-off-by: Dexuan Cui <decui@microsoft.com>
CC: Jake Oshins <jakeo@microsoft.com>
Cc: KY Srinivasan <kys@microsoft.com>
CC: Haiyang Zhang <haiyangz@microsoft.com>
CC: Vitaly Kuznetsov <vkuznets@redhat.com>

---

I made the patch based on discussions with Jake Oshins and others.

 drivers/pci/host/pci-hyperv.c | 53 +++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 93ed64a..7590ad0 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -2266,24 +2266,32 @@ static int hv_pci_probe(struct hv_device *hdev,
 	return ret;
 }
 
-/**
- * hv_pci_remove() - Remove routine for this VMBus channel
- * @hdev:	VMBus's tracking struct for this root PCI bus
- *
- * Return: 0 on success, -errno on failure
- */
-static int hv_pci_remove(struct hv_device *hdev)
+static void hv_pci_bus_exit(struct hv_device *hdev)
 {
-	int ret;
-	struct hv_pcibus_device *hbus;
-	union {
+	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
+	struct {
 		struct pci_packet teardown_packet;
-		u8 buffer[0x100];
+		u8 buffer[sizeof(struct pci_message)];
 	} pkt;
 	struct pci_bus_relations relations;
 	struct hv_pci_compl comp_pkt;
+	int ret;
 
-	hbus = hv_get_drvdata(hdev);
+	/*
+	 * After the host sends the RESCIND_CHANNEL message, it doesn't
+	 * access the per-channel ringbuffer any longer.
+	 */
+	if (hdev->channel->rescind)
+		return;
+
+	/* Delete any children which might still exist. */
+	memset(&relations, 0, sizeof(relations));
+	hv_pci_devices_present(hbus, &relations);
+
+	ret = hv_send_resources_released(hdev);
+	if (ret)
+		dev_err(&hdev->device,
+			"Couldn't send resources released packet(s)\n");
 
 	memset(&pkt.teardown_packet, 0, sizeof(pkt.teardown_packet));
 	init_completion(&comp_pkt.host_event);
@@ -2298,7 +2306,19 @@ static int hv_pci_remove(struct hv_device *hdev)
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (!ret)
 		wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ);
+}
+
+/**
+ * hv_pci_remove() - Remove routine for this VMBus channel
+ * @hdev:	VMBus's tracking struct for this root PCI bus
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static int hv_pci_remove(struct hv_device *hdev)
+{
+	struct hv_pcibus_device *hbus;
 
+	hbus = hv_get_drvdata(hdev);
 	if (hbus->state == hv_pcibus_installed) {
 		/* Remove the bus from PCI's point of view. */
 		pci_lock_rescan_remove();
@@ -2307,17 +2327,10 @@ static int hv_pci_remove(struct hv_device *hdev)
 		pci_unlock_rescan_remove();
 	}
 
-	ret = hv_send_resources_released(hdev);
-	if (ret)
-		dev_err(&hdev->device,
-			"Couldn't send resources released packet(s)\n");
+	hv_pci_bus_exit(hdev);
 
 	vmbus_close(hdev->channel);
 
-	/* Delete any children which might still exist. */
-	memset(&relations, 0, sizeof(relations));
-	hv_pci_devices_present(hbus, &relations);
-
 	iounmap(hbus->cfg_addr);
 	hv_free_config_window(hbus);
 	pci_free_resource_list(&hbus->resources_for_children);
-- 
2.7.4

RE: [PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove

[Jake Oshins]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Wednesday, November 9, 2016 11:19 PM
> To: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
> devel@linuxdriverproject.org
> Cc: gregkh@linuxfoundation.org; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Jake Oshins <jakeo@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>; Vitaly Kuznetsov
> <vkuznets@redhat.com>; jasowang@redhat.com; apw@canonical.com;
> olaf@aepfle.de; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove
> 
> 1. We don't really need such a big on-stack buffer when sending
> the teardown_packet: vmbus_sendpacket() here only uses
> sizeof(struct pci_message).
> 
> 2. In the hot-remove case (PCI_EJECT), after we send
> PCI_EJECTION_COMPLETE
> to the host, the host will send a RESCIND_CHANNEL message to us and the
> host won't access the per-channel ringbuffer any longer, so we needn't
> send PCI_RESOURCES_RELEASED/PCI_BUS_D0EXIT to the host, and we
> shouldn't
> expect the host's completion message of PCI_BUS_D0EXIT, which will never
> come.
> 
> 3. We should send PCI_BUS_D0EXIT after hv_send_resources_released().
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> CC: Jake Oshins <jakeo@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> ---
> 
> I made the patch based on discussions with Jake Oshins and others.
> 
>  drivers/pci/host/pci-hyperv.c | 53 +++++++++++++++++++++++++++--------
> --------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 93ed64a..7590ad0 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -2266,24 +2266,32 @@ static int hv_pci_probe(struct hv_device *hdev,
>  	return ret;
>  }
> 
> -/**
> - * hv_pci_remove() - Remove routine for this VMBus channel
> - * @hdev:	VMBus's tracking struct for this root PCI bus
> - *
> - * Return: 0 on success, -errno on failure
> - */
> -static int hv_pci_remove(struct hv_device *hdev)
> +static void hv_pci_bus_exit(struct hv_device *hdev)
>  {
> -	int ret;
> -	struct hv_pcibus_device *hbus;
> -	union {
> +	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> +	struct {
>  		struct pci_packet teardown_packet;
> -		u8 buffer[0x100];
> +		u8 buffer[sizeof(struct pci_message)];
>  	} pkt;
>  	struct pci_bus_relations relations;
>  	struct hv_pci_compl comp_pkt;
> +	int ret;
> 
> -	hbus = hv_get_drvdata(hdev);
> +	/*
> +	 * After the host sends the RESCIND_CHANNEL message, it doesn't
> +	 * access the per-channel ringbuffer any longer.
> +	 */
> +	if (hdev->channel->rescind)
> +		return;
> +
> +	/* Delete any children which might still exist. */
> +	memset(&relations, 0, sizeof(relations));
> +	hv_pci_devices_present(hbus, &relations);
> +
> +	ret = hv_send_resources_released(hdev);
> +	if (ret)
> +		dev_err(&hdev->device,
> +			"Couldn't send resources released packet(s)\n");
> 
>  	memset(&pkt.teardown_packet, 0, sizeof(pkt.teardown_packet));
>  	init_completion(&comp_pkt.host_event);
> @@ -2298,7 +2306,19 @@ static int hv_pci_remove(struct hv_device *hdev)
> 
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  	if (!ret)
>  		wait_for_completion_timeout(&comp_pkt.host_event, 10 *
> HZ);
> +}
> +
> +/**
> + * hv_pci_remove() - Remove routine for this VMBus channel
> + * @hdev:	VMBus's tracking struct for this root PCI bus
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int hv_pci_remove(struct hv_device *hdev)
> +{
> +	struct hv_pcibus_device *hbus;
> 
> +	hbus = hv_get_drvdata(hdev);
>  	if (hbus->state == hv_pcibus_installed) {
>  		/* Remove the bus from PCI's point of view. */
>  		pci_lock_rescan_remove();
> @@ -2307,17 +2327,10 @@ static int hv_pci_remove(struct hv_device
> *hdev)
>  		pci_unlock_rescan_remove();
>  	}
> 
> -	ret = hv_send_resources_released(hdev);
> -	if (ret)
> -		dev_err(&hdev->device,
> -			"Couldn't send resources released packet(s)\n");
> +	hv_pci_bus_exit(hdev);
> 
>  	vmbus_close(hdev->channel);
> 
> -	/* Delete any children which might still exist. */
> -	memset(&relations, 0, sizeof(relations));
> -	hv_pci_devices_present(hbus, &relations);
> -
>  	iounmap(hbus->cfg_addr);
>  	hv_free_config_window(hbus);
>  	pci_free_resource_list(&hbus->resources_for_children);
> --
> 2.7.4

This looks fine to me.

-- Jake Oshins

RE: [PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove

[KY Srinivasan]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Wednesday, November 9, 2016 11:19 PM
> To: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
> devel@linuxdriverproject.org
> Cc: gregkh@linuxfoundation.org; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Jake Oshins <jakeo@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>; Vitaly Kuznetsov
> <vkuznets@redhat.com>; jasowang@redhat.com; apw@canonical.com;
> olaf@aepfle.de; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/3] PCI: hv: fix hv_pci_remove() for hot-remove
> 
> 1. We don't really need such a big on-stack buffer when sending
> the teardown_packet: vmbus_sendpacket() here only uses
> sizeof(struct pci_message).
> 
> 2. In the hot-remove case (PCI_EJECT), after we send
> PCI_EJECTION_COMPLETE
> to the host, the host will send a RESCIND_CHANNEL message to us and the
> host won't access the per-channel ringbuffer any longer, so we needn't
> send PCI_RESOURCES_RELEASED/PCI_BUS_D0EXIT to the host, and we
> shouldn't
> expect the host's completion message of PCI_BUS_D0EXIT, which will never
> come.
> 
> 3. We should send PCI_BUS_D0EXIT after hv_send_resources_released().
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> CC: Jake Oshins <jakeo@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: Vitaly Kuznetsov <vkuznets@redhat.com>


Thanks Dexuan.

Acked-by: K. Y. Srinivasan <kys@microsoft.com>
> 
> ---
> 
> I made the patch based on discussions with Jake Oshins and others.
> 
>  drivers/pci/host/pci-hyperv.c | 53 +++++++++++++++++++++++++++--------
> --------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 93ed64a..7590ad0 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -2266,24 +2266,32 @@ static int hv_pci_probe(struct hv_device *hdev,
>  	return ret;
>  }
> 
> -/**
> - * hv_pci_remove() - Remove routine for this VMBus channel
> - * @hdev:	VMBus's tracking struct for this root PCI bus
> - *
> - * Return: 0 on success, -errno on failure
> - */
> -static int hv_pci_remove(struct hv_device *hdev)
> +static void hv_pci_bus_exit(struct hv_device *hdev)
>  {
> -	int ret;
> -	struct hv_pcibus_device *hbus;
> -	union {
> +	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> +	struct {
>  		struct pci_packet teardown_packet;
> -		u8 buffer[0x100];
> +		u8 buffer[sizeof(struct pci_message)];
>  	} pkt;
>  	struct pci_bus_relations relations;
>  	struct hv_pci_compl comp_pkt;
> +	int ret;
> 
> -	hbus = hv_get_drvdata(hdev);
> +	/*
> +	 * After the host sends the RESCIND_CHANNEL message, it doesn't
> +	 * access the per-channel ringbuffer any longer.
> +	 */
> +	if (hdev->channel->rescind)
> +		return;
> +
> +	/* Delete any children which might still exist. */
> +	memset(&relations, 0, sizeof(relations));
> +	hv_pci_devices_present(hbus, &relations);
> +
> +	ret = hv_send_resources_released(hdev);
> +	if (ret)
> +		dev_err(&hdev->device,
> +			"Couldn't send resources released packet(s)\n");
> 
>  	memset(&pkt.teardown_packet, 0, sizeof(pkt.teardown_packet));
>  	init_completion(&comp_pkt.host_event);
> @@ -2298,7 +2306,19 @@ static int hv_pci_remove(struct hv_device *hdev)
> 
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  	if (!ret)
>  		wait_for_completion_timeout(&comp_pkt.host_event, 10 *
> HZ);
> +}
> +
> +/**
> + * hv_pci_remove() - Remove routine for this VMBus channel
> + * @hdev:	VMBus's tracking struct for this root PCI bus
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int hv_pci_remove(struct hv_device *hdev)
> +{
> +	struct hv_pcibus_device *hbus;
> 
> +	hbus = hv_get_drvdata(hdev);
>  	if (hbus->state == hv_pcibus_installed) {
>  		/* Remove the bus from PCI's point of view. */
>  		pci_lock_rescan_remove();
> @@ -2307,17 +2327,10 @@ static int hv_pci_remove(struct hv_device
> *hdev)
>  		pci_unlock_rescan_remove();
>  	}
> 
> -	ret = hv_send_resources_released(hdev);
> -	if (ret)
> -		dev_err(&hdev->device,
> -			"Couldn't send resources released packet(s)\n");
> +	hv_pci_bus_exit(hdev);
> 
>  	vmbus_close(hdev->channel);
> 
> -	/* Delete any children which might still exist. */
> -	memset(&relations, 0, sizeof(relations));
> -	hv_pci_devices_present(hbus, &relations);
> -
>  	iounmap(hbus->cfg_addr);
>  	hv_free_config_window(hbus);
>  	pci_free_resource_list(&hbus->resources_for_children);
> --
> 2.7.4