On Thu, Jul 23, 2020 at 03:52:39PM +0000, Wei Hu wrote:

quoted

-----Original Message-----
From: Lorenzo Pieralisi <redacted>

quoted

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.

quoted

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.

quoted

quoted

Fixes: c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invalid
device state")
Signed-off-by: Wei Hu <redacted>

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