RE: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly

[Michael Kelley]

From: Wei Hu <weh@xxxxxxxxxxxxx>  Sent: Wednesday, May 6, 2020 6:22 AM
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Sent: Wednesday, May 6, 2020 7:10 PM
> > To: Wei Hu <weh@xxxxxxxxxxxxx>
> > Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> > <haiyangz@xxxxxxxxxxxxx>; Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>;
> > wei.liu@xxxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-
> > hyperv@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Dexuan Cui <decui@xxxxxxxxxxxxx>; Michael Kelley
> > <mikelley@xxxxxxxxxxxxx>
> > 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:
> > > Hi Lorenzo,
> > >
> > > Thanks for your review. Please see my comments inline.
> > >
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > > > Sent: Tuesday, May 5, 2020 11:03 PM
> > > > To: Wei Hu <weh@xxxxxxxxxxxxx>
> > > > Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> > > > <haiyangz@xxxxxxxxxxxxx>; Stephen Hemminger
> > > > <sthemmin@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; robh@xxxxxxxxxx;
> > > > bhelgaas@xxxxxxxxxx; linux- hyperv@xxxxxxxxxxxxxxx;
> > > > linux-pci@xxxxxxxxxxxxxxx; linux- kernel@xxxxxxxxxxxxxxx; Dexuan Cui
> > > > <decui@xxxxxxxxxxxxx>; Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > > > 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:
> > > > > 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.
> > > >
> > > > 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