Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init

Eric Farman
2019-07-02T16:15:32Z

On 7/2/19 9:58 AM, Farhan Ali wrote:
>
>
> On 07/02/2019 04:42 AM, Cornelia Huck wrote:
>> On Mon,  1 Jul 2019 12:23:44 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> We don't set cp->initialized to true so calling cp_free
>>> will just return and not do anything.
>>>
>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>> index 5ac4c1e..cab1be9 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct
>>> device *mdev, union orb *orb)
>>>         /* Build a ccwchain for the first CCW segment */
>>>       ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>>> -    if (ret)
>>> -        cp_free(cp);
>>
>> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on
>> error :) (I think it does)
>>
>
> I have checked that it does as well, but wouldn't hurt if someone else
> also glances over once again :)

Oh noes. What happens once we start encountering TICs? If we do:

ccwchain_handle_ccw() (OK)
ccwchain_loop_tic() (OK)
ccwchain_handle_ccw() (FAIL)

The first _handle_ccw() will have added a ccwchain to the cp list, which
doesn't appear to get cleaned up now. That used to be done in cp_init()
until I squashed cp_free and cp_unpin_free. :(

>
>> Maybe add a comment
>>
>> /* ccwchain_handle_ccw() already cleans up on error */
>>
>> so we don't stumble over this in the future?
>
> Sure.
>
>>
>> (Also, does this want a Fixes: tag?)
>
> This might warrant a fixes tag as well.
>>
>>>         if (!ret)
>>>           cp->initialized = true;
>>
>>