From: Wei Hu <redacted>  Sent: Wednesday, May 6, 2020 6:22 AM

quoted

-----Original Message-----
From: Lorenzo Pieralisi <redacted>
Sent: Wednesday, May 6, 2020 7:10 PM
To: Wei Hu <redacted>
Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
[off-list ref]; Stephen Hemminger [off-list ref];
wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
kernel@vger.kernel.org; Dexuan Cui [off-list ref]; Michael Kelley
[off-list ref]
Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to
release resource properly

On Wed, May 06, 2020 at 05:36:46AM +0000, Wei Hu wrote:

quoted

Hi Lorenzo,

Thanks for your review. Please see my comments inline.

quoted

-----Original Message-----
From: Lorenzo Pieralisi <redacted>
Sent: Tuesday, May 5, 2020 11:03 PM
To: Wei Hu <redacted>
Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
[off-list ref]; Stephen Hemminger
[off-list ref]; wei.liu@kernel.org; robh@kernel.org;
bhelgaas@google.com; linux- hyperv@vger.kernel.org;
linux-pci@vger.kernel.org; linux- kernel@vger.kernel.org; Dexuan Cui
[off-list ref]; Michael Kelley [off-list ref]
Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe
failure path to release resource properly

On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:

quoted

Some error cases in hv_pci_probe() were not handled. Fix these
error paths to release the resourses and clean up the state properly.

This patch does more than that. It adds a variable to store the
number of slots actually allocated - I presume to free only allocated on slots

on the exit path.

quoted

quoted

Two patches required I am afraid.

Well, adding this variable is needed to make the call of "(void)

hv_pci_bus_exit(hdev, true)"

I don't understand why - it is not clear from the commit log and the code,
please explain it since it is not obvious.

Hv_pci_bus_exit() calls hv_send_resources_released() to release all child resources.
These child resources were allocated in hv_send_resources_allocated().
Hv_send_resources_allocated() could fail in the middle, leaving some child resources
allocated and rest not. Without adding this variable to record the highest slot number that
resource has been successfully allocated, calling hv_send_resources_released() could
cause spurious resource release requests being sent to hypervisor.

This had been fine since hv_pci_bus_exit() was never called in error path before this patch
was
introduced. To add this call to clean the pci state in the error path, we need to know the
starting
point in child device that resource has not been allocated. Hence this variable
is used in hv_send_resources_allocated() to record this point and in
hv_send_resource_released() to start deallocating child resources.

I can add to the commit log if you are fine with this explanation.

FWIW, I think of this patch as follows:

In some error cases in hv_pci_probe(), allocated resources are not
freed.  Fix this by adding a field to keep track of the high water mark
for slots that have resources allocated to them.  In case of an error, this
high water mark is used to know which slots have resources that
must be released.  Since slots are numbered starting with zero, a
value of -1 indicates no slots have been allocated resources.  There
may be unused slots in the range between slot 0 and the high
water mark slot, but these slots are already ignored by the existing code
in the allocate and release loops with the call to get_pcichild_wslot().

Michael