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

[...]

quoted

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