Re: [PATCH v3 1/1] vfio-ccw: Prevent quiesce function going into an infinite loop

Farhan Ali
2019-04-22T14:01:46Z

On 04/19/2019 04:12 PM, Halil Pasic wrote:
> On Wed, 17 Apr 2019 11:18:19 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
>
>>
>>
>> On 04/17/2019 11:13 AM, Halil Pasic wrote:
>>>>> Otherwise, looks good to me. Will queue when I get some ack/r-b.
>>>>>
>>>> I like it, but I feel weird giving an r-b to something I suggested:
>>>>
>>>> Acked-by: Eric Farman<farman@linux.ibm.com>
>>>>
>>> I think r-b is fine. You did verify both the design and the
>>> implementation I guess. So I don't see why not.
>>>
>>> How urgent is this. I could give this some love till the end of the
>>> week. Should I @Connie,@Farhan?
>>
>> Having more people review it is always a good thing :)
>>
>
> Hi Farhan,
>
> I was starring at this code for about an hour if not more and could not
> figure out the intentions/ideas behind it. That is not a fault of your
> patch, but I can't say that I understand neither the before nor the
> after.
>
> What understand this patch basically does is make us call
> cio_disable_subchannel() more often. That is what you point out in your
> commit message as well. But I fail to see how does this achieve what the
> summary line promises: 'Prevent quiesce function going into an infinite
> loop'.
>

The main problem with the previous way, was we were calling
cio_cancel_halt_clear and then waiting and then calling it again.

So if cio_cancel_halt_clear returned EBUSY we would always be stuck in
the first loop. Now a problem can occur when cancel subchannel returns
EINVAL (cc 2) and so we try to do halt subchannel. cio_cancel_halt_clear
will return EBUSY for a successful halt subchannel as well. And so back
in the quiesce function we will wait and if the halt succeeds, the
channel subsystem will clear the halt pending bit in the activity
control field of SCSW. This means the next time we try
cio_cancel_halt_clear we will again start by calling cancel subchannel,
which could again return EINVAL....

We would be stuck in an infinite loop. One way to prevent this is to
call cio_disable_subchannel right after calling cio_cancel_halt_clear,
if we can successfully disable the subchannel then we are sure the
device is quiesced.

> Sorry, I can't r-b this. Maybe you can help me gain an understanding of
> this code offline.

I hope the above explanation helps.

>
> I guess, the approval of the people who actually understand what it is
> going on (i.e. Connie and Eric) will have to suffice.
>
> Regards,
> Halil
>
>