RE: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state

[Wei Hu]

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Sent: Tuesday, May 5, 2020 11: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 2/2] PCI: hv: Retry PCI bus D0 entry when the first
> attempt failed with invalid device state
> 
> On Fri, May 01, 2020 at 01:37:28PM +0800, Wei Hu wrote:
> > In the case of kdump, the PCI device was not cleanly shut down before
> > the kdump kernel starts. This causes the initial attempt of entering
> > D0 state in the kdump kernel to fail with invalid device state
> > returned from Hyper-V host.
> > When this happens, explicitly call PCI bus exit and retry to enter the
> > D0 state.
> >
> > Signed-off-by: Wei Hu <weh@xxxxxxxxxxxxx>
> > ---
> >    v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
> >    Bjorn Helgaas
> >
> >  drivers/pci/controller/pci-hyperv.c | 40
> > +++++++++++++++++++++++++++--
> >  1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c
> > b/drivers/pci/controller/pci-hyperv.c
> > index e6fac0187722..92092a47d3af 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -2739,6 +2739,8 @@ static void hv_free_config_window(struct
> hv_pcibus_device *hbus)
> >  	vmbus_free_mmio(hbus->mem_config->start,
> PCI_CONFIG_MMIO_LENGTH);  }
> >
> > +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
> > +
> >  /**
> >   * hv_pci_enter_d0() - Bring the "bus" into the D0 power state
> >   * @hdev:	VMBus's tracking struct for this root PCI bus
> > @@ -2751,8 +2753,10 @@ static int hv_pci_enter_d0(struct hv_device
> *hdev)
> >  	struct pci_bus_d0_entry *d0_entry;
> >  	struct hv_pci_compl comp_pkt;
> >  	struct pci_packet *pkt;
> > +	bool retry = true;
> >  	int ret;
> >
> > +enter_d0_retry:
> >  	/*
> >  	 * Tell the host that the bus is ready to use, and moved into the
> >  	 * powered-on state.  This includes telling the host which region @@
> > -2779,6 +2783,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> >  	if (ret)
> >  		goto exit;
> >
> > +	/*
> > +	 * In certain case (Kdump) the pci device of interest was
> > +	 * not cleanly shut down and resource is still held on host
> > +	 * side, the host could return invalid device status.
> > +	 * We need to explicitly request host to release the resource
> > +	 * and try to enter D0 again.
> > +	 */
> > +	if (comp_pkt.completion_status < 0 && retry) {
> > +		retry = false;
> > +
> > +		dev_err(&hdev->device, "Retrying D0 Entry\n");
> > +
> > +		/*
> > +		 * Hv_pci_bus_exit() calls hv_send_resource_released()
> > +		 * to free up resources of its child devices.
> > +		 * In the kdump kernel we need to set the
> > +		 * wslot_res_allocated to 255 so it scans all child
> > +		 * devices to release resources allocated in the
> > +		 * normal kernel before panic happened.
> > +		 */
> > +		hbus->wslot_res_allocated = 255;
> 
> I'd rather write a specific function eg hv_pci_bus_shutdown() to make it explicit.
> Actually, isn't it something that should *always* be _enforced_ at host bridge
> probe time - regardless of the kernel you are booting on ?
> 

It is only needed in kdump kernel when the normal kernel which just crashed 
failed to deallocate the PCI resources cleanly. All the states have been taken
care of in the normal kernel without needing this. 

Wei