On Thu, Apr 22, 2021 at 12:21 PM Michal Suchánek <msucha...@suse.de> wrote: > > Hello, > > On Thu, Apr 22, 2021 at 12:06:45AM -0500, Lijun Pan wrote: > > On Wed, Apr 21, 2021 at 2:25 AM Sukadev Bhattiprolu > > <suka...@linux.ibm.com> wrote: > > > > > > Lijun Pan [l...@linux.vnet.ibm.com] wrote: > > > > > > > > > > > > > On Apr 20, 2021, at 4:35 PM, Dany Madden <d...@linux.ibm.com> wrote: > > > > > > > > > > When ibmvnic gets a FATAL error message from the vnicserver, it marks > > > > > the Command Respond Queue (CRQ) inactive and resets the adapter. If > > > > > this > > > > > FATAL reset fails and a transmission timeout reset follows, the CRQ is > > > > > still inactive, ibmvnic's attempt to set link down will also fail. If > > > > > ibmvnic abandons the reset because of this failed set link down and > > > > > this > > > > > is the last reset in the workqueue, then this adapter will be left in > > > > > an > > > > > inoperable state. > > > > > > > > > > Instead, make the driver ignore this link down failure and continue to > > > > > free and re-register CRQ so that the adapter has an opportunity to > > > > > recover. > > > > > > > > This v2 does not adddress the concerns mentioned in v1. > > > > And I think it is better to exit with error from do_reset, and schedule > > > > a thorough > > > > do_hard_reset if the the adapter is already in unstable state. > > > > > > We had a FATAL error and when handling it, we failed to send a > > > link-down message to the VIOS. So what we need to try next is to > > > reset the connection with the VIOS. For this we must talk to the > > > firmware using the H_FREE_CRQ and H_REG_CRQ hcalls. do_reset() > > > does just that in ibmvnic_reset_crq(). > > > > > > Now, sure we can attempt a "thorough hard reset" which also does > > > the same hcalls to reestablish the connection. Is there any > > > other magic in do_hard_reset()? But in addition, it also frees lot > > > more Linux kernel buffers and reallocates them for instance. > > > > Working around everything in do_reset will make the code very difficult > > to manage. Ultimately do_reset can do anything I am afraid, and > > do_hard_reset > > can be removed completely or merged into do_reset.
Michal, I should have given more details about the above statement. Thanks for your detailed info in the below. > > This debate is not very constructive. > > In the context of driver that has separate do_reset and do_hard_reset > this fix picks the correct one unless you can refute the arguments > provided. > > Merging do_reset and do_hard_reset might be a good code cleanup which is > out of the scope of this fix. Right. > > > > Given that vast majority of fixes to the vnic driver are related to the > reset handling it would improve stability and testability if every > reset took the same code path. I agree. > > In the context of merging do_hard_reset and do_reset the question is > what is the intended distinction and performance gain by having > 'lightweight' reset. Right. > > I don't have a vnic protocol manual at hand and I suspect I would not > get one even if I searched for one. > > From reading through the fixes in the past my understanding is that the > full reset is required when the backend changes which then potentially > requires different size/number of buffers. Yes, full reset is better when the backend changes. > > What is the expected situation when reset is required without changing > the backend? > > Is this so common that it warrants a separate 'lightweight' optimized > function? >