Re: [PATCH v3] PCI: hv: Fix a timing issue which causes kdump to fail occasionally

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Wei Hu <weh@microsoft.com>
Cc: 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>,
	Michael Kelley <mikelley@microsoft.com>
Subject: Re: [PATCH v3] PCI: hv: Fix a timing issue which causes kdump to fail occasionally
Date: Thu, 23 Jul 2020 17:36:43 +0100	[thread overview]
Message-ID: <20200723163643.GB11749@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <SG2P153MB03774814C5D6450238CC4471BB760@SG2P153MB0377.APCP153.PROD.OUTLOOK.COM>

On Thu, Jul 23, 2020 at 03:52:39PM +0000, Wei Hu wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Kdump could fail sometime on Hyper-V guest over Accelerated Network
> > > interface. This is because the retry in hv_pci_enter_d0() relies on an
> > > asynchronous host event arriving before the guest calls
> > > hv_send_resources_allocated(). Fix the problem by moving retry to
> > > hv_pci_probe(), removing this dependency and making the calling
> > > sequence synchronous.
> > 
> > You have to explain why this code move fixes the problem
> 
> How about following commit message:
> 
> Kdump could fail sometime on Hyper-V guest over Accelerated Network
> nterface.

"Interface" and this is irrelevant if you don't explain why the
failure is caused by it and not any other piece of software,
ie why the failure happens explicitly on the Accelerated Network
Interface only.

> This is because the retry in hv_pci_enter_d0() relies on an
> asynchronous host event arriving before the guest calls
> hv_send_resources_allocated(). Fix the problem by moving retry to
> hv_pci_probe() and start the retry from hv_pci_query_relations() call. 
> This causes the host to generate an synchronous event, hence removing 
> this unreliable dependency.

It is a bit better but I am pretty sure it can be improved, for instance
by describing the failure path in relation to the asynchronous
notification, in other words what happens when things go wrong.

> > and you also have to
> > add a comment to the code so that anyone who has to fix it in the future can
> > understand why the code is where you are moving it to and why that's a
> > solution.
> 
> It already has the comment in the code as:
> +	 * The retry should start from hv_pci_query_relations() call.

Explain *why* it should restart from there, it is not that complicated.

> It would be a bug if the retry does not start from this according to the host
> Behaviour. So I think no further explanation would be needed.

It is needed, add it please.

> > > Fixes: c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invalid
> > > device state")
> > > Signed-off-by: Wei Hu <weh@microsoft.com>
> > 
> > Please carry tags and send patches -in-reply-to the previous version to allow
> > threading.
> > 
> 
> Do you mean to add review-by tags? If not would you please elaborate
> what 'carry tags' means? Sorry I am not familiar to this term.

It should not be me who applies Reviewed-by tags added on previous patch
versions, it should be you who add it to newer versions.

AKA Michael already reviewed it and I need to see his reviewed-by tag
to apply it.

Thanks,
Lorenzo