Message-ID: <20191120172026.GE3279@e121166-lin.cambridge.arm.com>
Date:   Wed, 20 Nov 2019 17:20:26 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Dexuan Cui <decui@...rosoft.com>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        sashal@...nel.org, bhelgaas@...gle.com,
        linux-hyperv@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, mikelley@...rosoft.com,
        Alexander.Levin@...rosoft.com
Subject: Re: [PATCH v2 2/4] PCI: hv: Add the support of hibernation

On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote:
> Implement the suspend/resume callbacks.
> 
> We must make sure there is no pending work items before we call
> vmbus_close().

Where ? Why ? Imagine a developer reading this log to try to understand
why you made this change, do you really think this commit log is
informative in its current form ?

I am not asking a book but this is a significant feature please make
an effort to explain it (I can update the log for you but please
write one and I shall do it).

Lorenzo

> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> ---
> 
> Changes in v2: this patch is a simple merge of 2 previous smaller patches,
> accordign to the suggestion of Lorenzo Pieralisi <lorenzo.pieralisi@....com>.
> 
>  drivers/pci/controller/pci-hyperv.c | 107 +++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 65f18f840ce9..e71eb6e0bfdd 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -455,6 +455,7 @@ enum hv_pcibus_state {
>  	hv_pcibus_init = 0,
>  	hv_pcibus_probed,
>  	hv_pcibus_installed,
> +	hv_pcibus_removing,
>  	hv_pcibus_removed,
>  	hv_pcibus_maximum
>  };
> @@ -1681,6 +1682,23 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
>  
>  	spin_lock_irqsave(&hbus->device_list_lock, flags);
>  
> +	/*
> +	 * Clear the memory enable bit, in case it's already set. This occurs
> +	 * in the suspend path of hibernation, where the device is suspended,
> +	 * resumed and suspended again: see hibernation_snapshot() and
> +	 * hibernation_platform_enter().
> +	 *
> +	 * If the memory enable bit is already set, Hyper-V sliently ignores
> +	 * the below BAR updates, and the related PCI device driver can not
> +	 * work, because reading from the device register(s) always returns
> +	 * 0xFFFFFFFF.
> +	 */
> +	list_for_each_entry(hpdev, &hbus->children, list_entry) {
> +		_hv_pcifront_read_config(hpdev, PCI_COMMAND, 2, &command);
> +		command &= ~PCI_COMMAND_MEMORY;
> +		_hv_pcifront_write_config(hpdev, PCI_COMMAND, 2, command);
> +	}
> +
>  	/* Pick addresses for the BARs. */
>  	do {
>  		list_for_each_entry(hpdev, &hbus->children, list_entry) {
> @@ -2107,6 +2125,12 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
>  	unsigned long flags;
>  	bool pending_dr;
>  
> +	if (hbus->state == hv_pcibus_removing) {
> +		dev_info(&hbus->hdev->device,
> +			 "PCI VMBus BUS_RELATIONS: ignored\n");
> +		return;
> +	}
> +
>  	dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
>  	if (!dr_wrk)
>  		return;
> @@ -2223,11 +2247,19 @@ static void hv_eject_device_work(struct work_struct *work)
>   */
>  static void hv_pci_eject_device(struct hv_pci_dev *hpdev)
>  {
> +	struct hv_pcibus_device *hbus = hpdev->hbus;
> +	struct hv_device *hdev = hbus->hdev;
> +
> +	if (hbus->state == hv_pcibus_removing) {
> +		dev_info(&hdev->device, "PCI VMBus EJECT: ignored\n");
> +		return;
> +	}
> +
>  	hpdev->state = hv_pcichild_ejecting;
>  	get_pcichild(hpdev);
>  	INIT_WORK(&hpdev->wrk, hv_eject_device_work);
> -	get_hvpcibus(hpdev->hbus);
> -	queue_work(hpdev->hbus->wq, &hpdev->wrk);
> +	get_hvpcibus(hbus);
> +	queue_work(hbus->wq, &hpdev->wrk);
>  }
>  
>  /**
> @@ -3107,6 +3139,75 @@ static int hv_pci_remove(struct hv_device *hdev)
>  	return ret;
>  }
>  
> +static int hv_pci_suspend(struct hv_device *hdev)
> +{
> +	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> +	enum hv_pcibus_state old_state;
> +	int ret;
> +
> +	tasklet_disable(&hdev->channel->callback_event);
> +
> +	/* Change the hbus state to prevent new work items. */
> +	old_state = hbus->state;
> +	if (hbus->state == hv_pcibus_installed)
> +		hbus->state = hv_pcibus_removing;
> +
> +	tasklet_enable(&hdev->channel->callback_event);
> +
> +	if (old_state != hv_pcibus_installed)
> +		return -EINVAL;
> +
> +	flush_workqueue(hbus->wq);
> +
> +	ret = hv_pci_bus_exit(hdev, true);
> +	if (ret)
> +		return ret;
> +
> +	vmbus_close(hdev->channel);
> +
> +	return 0;
> +}
> +
> +static int hv_pci_resume(struct hv_device *hdev)
> +{
> +	struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> +	enum pci_protocol_version_t version[1];
> +	int ret;
> +
> +	hbus->state = hv_pcibus_init;
> +
> +	ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
> +			 hv_pci_onchannelcallback, hbus);
> +	if (ret)
> +		return ret;
> +
> +	/* Only use the version that was in use before hibernation. */
> +	version[0] = pci_protocol_version;
> +	ret = hv_pci_protocol_negotiation(hdev, version, 1);
> +	if (ret)
> +		goto out;
> +
> +	ret = hv_pci_query_relations(hdev);
> +	if (ret)
> +		goto out;
> +
> +	ret = hv_pci_enter_d0(hdev);
> +	if (ret)
> +		goto out;
> +
> +	ret = hv_send_resources_allocated(hdev);
> +	if (ret)
> +		goto out;
> +
> +	prepopulate_bars(hbus);
> +
> +	hbus->state = hv_pcibus_installed;
> +	return 0;
> +out:
> +	vmbus_close(hdev->channel);
> +	return ret;
> +}
> +
>  static const struct hv_vmbus_device_id hv_pci_id_table[] = {
>  	/* PCI Pass-through Class ID */
>  	/* 44C4F61D-4444-4400-9D52-802E27EDE19F */
> @@ -3121,6 +3222,8 @@ static struct hv_driver hv_pci_drv = {
>  	.id_table	= hv_pci_id_table,
>  	.probe		= hv_pci_probe,
>  	.remove		= hv_pci_remove,
> +	.suspend	= hv_pci_suspend,
> +	.resume		= hv_pci_resume,
>  };
>  
>  static void __exit exit_hv_pci_drv(void)
> -- 
> 2.19.1
> 

Powered by blists - more mailing lists