On Fri, 12 Apr 2019 10:38:50 -0400 Farhan Ali <alifm@xxxxxxxxxxxxx> wrote: > On 04/12/2019 04:10 AM, Cornelia Huck wrote: > > On Thu, 11 Apr 2019 16:30:44 -0400 > > Farhan Ali <alifm@xxxxxxxxxxxxx> wrote: > > > >> On 04/11/2019 12:24 PM, Cornelia Huck wrote: > >>> On Mon, 8 Apr 2019 17:05:32 -0400 > >>> Farhan Ali <alifm@xxxxxxxxxxxxx> wrote: > >>> Looking at the possible return codes: > >>> * -ENODEV -> device is not operational anyway, in theory you should even > >>> not need to bother with disabling the subchannel > >>> * -EIO -> we've run out of retries, and the subchannel still is not > >>> idle; I'm not sure if we could do anything here, as disable is > >>> unlikely to work, either (...) > Thinking a little bit more about EIO, if the return code is EIO then it > means we have exhausted all our options with cancel_halt_clear and the > subchannel/device is still status pending, right? Yes. > > I think we should still continue to try and disable the subchannel, > because if not then the subchannel/device could in some point of time > come back and bite us. So we really should protect the system from this > behavior. I think trying to disable the subchannel does not really hurt, but I fear it won't succeed in that case... > > I think for EIO we should log an error message, but still try to > continue with disabling the subchannel. What do you or others think? Logging an error may be useful (it's really fouled up at that time), but... > > > > > >> > >>>> + flush_workqueue(vfio_ccw_work_q); > >>>> + spin_lock_irq(sch->lock); > >>>> ret = cio_disable_subchannel(sch); ...there's a good chance that we'd get -EBUSY here, which would keep us in the loop. We probably need to break out after we got -EIO from cancel_halt_clear, regardless of which return code we get from the disable. (It will be "interesting" to see what happens with such a stuck subchannel in the calling code; but I don't really see many options. Panic seems way too strong; maybe mark the subchannel as "broken; no idea how to fix"? But that would be a follow-on patch; I think if we avoid the endless loop here, this patch is a real improvement and should just go in.) > >>>> } while (ret == -EBUSY); > >>>> out_unlock: > >>> > >>> > >> > > > > >
