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

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Michael Kelley <mikelley@microsoft.com>
Cc: Wei Hu <weh@microsoft.com>, KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	"robh@kernel.org" <robh@kernel.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
Date: Wed, 6 May 2020 16:21:26 +0100	[thread overview]
Message-ID: <20200506152126.GA2911@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <MW2PR2101MB1052F033623F91A0FF991DE4D7A40@MW2PR2101MB1052.namprd21.prod.outlook.com>

On Wed, May 06, 2020 at 02:55:17PM +0000, Michael Kelley wrote:

[...]

> > 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().

That's much clearer now - please add these bits of info to the commit
log, it is essential that developers can find an explanation for a
change like this one there IMO.

Overall the code changes are fine, I am not a big fan of the (void)
cast (I don't think error codes should be ignored) but it is acceptable,
in this context.

Thank you for taking some time to review the code together.

Lorenzo