Re: [PATCH net v3] ibmvnic: Continue with reset if set link down failed

[Dany Madden <drt@linux.ibm.com>]

On 2021-05-04 12:31, Lijun Pan wrote:
> On Tue, May 4, 2021 at 2:27 PM Lijun Pan <lijunp213@gmail.com> wrote:
>> 
>> On Tue, May 4, 2021 at 2:14 PM Dany Madden <drt@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.
>> >
>> > Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
>> > Signed-off-by: Dany Madden <drt@linux.ibm.com>
>> > Reviewed-by: Rick Lindsley <ricklind@linux.ibm.com>
>> > Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
>> > ---
>> > Changes in V2:
>> > - Update description to clarify background for the patch
>> > - Include Reviewed-by tags
>> > Changes in V3:
>> > - Add comment above the code change
>> > ---
>> >  drivers/net/ethernet/ibm/ibmvnic.c | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> > index 5788bb956d73..9e005a08d43b 100644
>> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> > @@ -2017,8 +2017,15 @@ static int do_reset(struct ibmvnic_adapter *adapter,
>> >                         rtnl_unlock();
>> >                         rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
>> >                         rtnl_lock();
>> > -                       if (rc)
>> > -                               goto out;
>> > +
>> > +                       /* Attempted to set the link down. It could fail if the
>> > +                        * vnicserver has already torn down the CRQ. We will
>> > +                        * note it and continue with reset to reinit the CRQ.
>> > +                        */
>> > +                       if (rc) {
>> > +                               netdev_dbg(netdev,
>> > +                                          "Setting link down failed rc=%d. Continue anyway\n", rc);
>> > +                       }
>> 
>> There are other places which check and rely on the return value of
>> this function. Your change makes that inconsistent. Can you stop
> 
> To be more specific, __ibmvnic_close, __ibmvnic_open both call this
> set_link_state.
Inconsistent would have been not checking for the rc at all. Here we 
checked and noted it that there are times that it's ok to continue.

> 
>> posting new versions and soliciting the maintainer to accept it before
>> there is material change? There are many ways to make reset
>> successful. I think this is the worst approach of all.

Can you show me a patch that is better than this one, that has gone thru 
a 30+ hours of testing?

>> 
>> 
>> >
>> >                         if (adapter->state == VNIC_OPEN) {
>> >                                 /* When we dropped rtnl, ibmvnic_open() got
>> > --
>> > 2.18.2
>> >