Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman <farman@linux.ibm.com>]

On 3/24/20 11:58 AM, Cornelia Huck wrote:
> On Fri, 14 Feb 2020 11:35:21 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 2/14/20 7:11 AM, Cornelia Huck wrote:
>>> On Thu,  6 Feb 2020 22:38:18 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
> 
>>> (...)  
>>>> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>>>  	return rc;
>>>>  }
>>>>  
>>>> +static int vfio_ccw_chp_event(struct subchannel *sch,
>>>> +			      struct chp_link *link, int event)
>>>> +{
>>>> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>>>> +	int retry = 255;
>>>> +
>>>> +	if (!private || !mask)
>>>> +		return 0;
>>>> +
>>>> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
>>>> +			   mdev_uuid(private->mdev), sch->schid.cssid,
>>>> +			   sch->schid.ssid, sch->schid.sch_no,
>>>> +			   mask, event);
>>>> +
>>>> +	if (cio_update_schib(sch))
>>>> +		return -ENODEV;
>>>> +
>>>> +	switch (event) {
>>>> +	case CHP_VARY_OFF:
>>>> +		/* Path logically turned off */
>>>> +		sch->opm &= ~mask;
>>>> +		sch->lpm &= ~mask;
>>>> +		break;
>>>> +	case CHP_OFFLINE:
>>>> +		/* Path is gone */
>>>> +		cio_cancel_halt_clear(sch, &retry);  
>>>
>>> Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF?  
>>
>> Hrm...  No reason that I can think of.  I can fix this.
>>
>>>   
>>>> +		break;
>>>> +	case CHP_VARY_ON:
>>>> +		/* Path logically turned on */
>>>> +		sch->opm |= mask;
>>>> +		sch->lpm |= mask;
>>>> +		break;
>>>> +	case CHP_ONLINE:
>>>> +		/* Path became available */
>>>> +		sch->lpm |= mask & sch->opm;  
>>>
>>> If I'm not mistaken, this patch introduces the first usage of sch->opm
>>> in the vfio-ccw code.   
>>
>> Correct.
>>
>>> Are we missing something?  
>>
>> Maybe?  :)
>>
>>> Or am I missing
>>> something? :)
>>>   
>>
>> Since it's only used in this code, for acting as a step between
>> vary/config off/on, maybe this only needs to be dealing with the lpm
>> field itself?
> 
> Ok, I went over this again and also looked at what the standard I/O
> subchannel driver does, and I think this is fine, as the lpm basically
> factors in the opm already. (Will need to keep this in mind for the
> following patches.)

Just to make sure I don't misunderstand, when you say "I think this is
fine" ... Do you mean keeping the opm field within vfio-ccw, as this
patch does?  Or removing it, and only adjusting the lpm within vfio-ccw,
as I suggested in my response just above?

(It's long in the day, and should not look at vfio-ccw at this hour.)

> 
>>
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static struct css_device_id vfio_ccw_sch_ids[] = {
>>>>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>>>>  	{ /* end of list */ },  
>>> (...)
>>>   
>>
>