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

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

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:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Register the chp_event callback to receive channel path related
>> events for the subchannels managed by vfio-ccw.
> 
> I'm wondering how useful this patch would be on its own.

Probably not much.  I can't speak to his original thoughts on the
matter, but it doesn't seem to buy us much by itself other than a
consumable sized patch.

> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Move s390dbf before cio_update_schib() call [CH]
>>     
>>     v0->v1: [EF]
>>      - Add s390dbf trace
>>
>>  drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
> (...)
>> @@ -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?

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