Re: [PATCH 02/10] vfio: ccw: Transform FSM functions to return state

From: Pierre Morel <pmorel@linux.vnet.ibm.com>
To: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Cc: pasic@linux.vnet.ibm.com, linux-s390@vger.kernel.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	cohuck@redhat.com
Subject: Re: [PATCH 02/10] vfio: ccw: Transform FSM functions to return state
Date: Tue, 24 Apr 2018 10:22:15 +0200	[thread overview]
Message-ID: <047dd088-6cba-c1fa-2295-05d31d7f4f2d@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180424072550.GW12194@bjsdjshi@linux.vnet.ibm.com>

On 24/04/2018 09:25, Dong Jia Shi wrote:
> * Pierre Morel <pmorel@linux.vnet.ibm.com> [2018-04-19 16:48:05 +0200]:
>
>> We change the FSM functions to return the next state,
>> and adapt the fsm_func_t function type.
> I think I'd need to read the rest patches to understand why we need this
> one, but no hurt to write some ideas that I noticed at my first glance.
> See below please.
>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 24 ++++++++++++++++--------
>>   drivers/s390/cio/vfio_ccw_private.h |  5 +++--
>>   2 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index af88551..2f3108d 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -60,7 +60,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>>   	}
>>   }
>>
>> -static void fsm_notoper(struct vfio_ccw_private *private,
>> +static int fsm_notoper(struct vfio_ccw_private *private,
>>   			enum vfio_ccw_event event)
>>   {
>>   	struct subchannel *sch = private->sch;
>> @@ -71,30 +71,34 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>>   	 */
>>   	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>>   	private->state = VFIO_CCW_STATE_NOT_OPER;
> Here..
>
>> +	return private->state;
>>   }
>>
>>   /*
>>    * No operation action.
>>    */
>> -static void fsm_nop(struct vfio_ccw_private *private,
>> +static int fsm_nop(struct vfio_ccw_private *private,
>>   		    enum vfio_ccw_event event)
>>   {
>> +	return private->state;
>>   }
>>
>> -static void fsm_io_error(struct vfio_ccw_private *private,
>> +static int fsm_io_error(struct vfio_ccw_private *private,
>>   			 enum vfio_ccw_event event)
>>   {
>>   	pr_err("vfio-ccw: FSM: I/O request from state:%d\n", private->state);
>>   	private->io_region.ret_code = -EIO;
>> +	return private->state;
>>   }
>>
>> -static void fsm_io_busy(struct vfio_ccw_private *private,
>> +static int fsm_io_busy(struct vfio_ccw_private *private,
>>   			enum vfio_ccw_event event)
>>   {
>>   	private->io_region.ret_code = -EBUSY;
>> +	return private->state;
>>   }
>>
>> -static void fsm_disabled_irq(struct vfio_ccw_private *private,
>> +static int fsm_disabled_irq(struct vfio_ccw_private *private,
>>   			     enum vfio_ccw_event event)
>>   {
>>   	struct subchannel *sch = private->sch;
>> @@ -104,12 +108,13 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
>>   	 * successful - should not happen, but we try to disable again.
>>   	 */
>>   	cio_disable_subchannel(sch);
>> +	return private->state;
>>   }
>>
>>   /*
>>    * Deal with the ccw command request from the userspace.
>>    */
>> -static void fsm_io_request(struct vfio_ccw_private *private,
>> +static int fsm_io_request(struct vfio_ccw_private *private,
>>   			   enum vfio_ccw_event event)
>>   {
>>   	union orb *orb;
>> @@ -141,7 +146,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>   			cp_free(&private->cp);
>>   			goto err_out;
>>   		}
>> -		return;
>> +		return private->state;
>>   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>>   		/* XXX: Handle halt. */
>>   		io_region->ret_code = -EOPNOTSUPP;
>> @@ -154,12 +159,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>
>>   err_out:
>>   	private->state = VFIO_CCW_STATE_IDLE;
> Here..
>
>> +	return private->state;
>>   }
>>
>>   /*
>>    * Got an interrupt for a normal io (state busy).
>>    */
>> -static void fsm_irq(struct vfio_ccw_private *private,
>> +static int fsm_irq(struct vfio_ccw_private *private,
>>   		    enum vfio_ccw_event event)
>>   {
>>   	struct irb *irb = &private->irb;
>> @@ -172,6 +178,8 @@ static void fsm_irq(struct vfio_ccw_private *private,
>>
>>   	if (private->io_trigger)
>>   		eventfd_signal(private->io_trigger, 1);
>> +
>> +	return private->state;
>>   }
>>
>>   /*
>> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
>> index 78a66d9..f526b18 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -83,13 +83,14 @@ enum vfio_ccw_event {
>>   /*
>>    * Action called through jumptable.
>>    */
>> -typedef void (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
>> +typedef int (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
>>   extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
>>
>>   static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
>>   				     int event)
>>   {
>> -	vfio_ccw_jumptable[private->state][event](private, event);
>> +	private->state = vfio_ccw_jumptable[private->state][event](private,
>> +								   event);
> Since here it assigns new value to private->state, there is no need to
> do that inside each fsm_func?
Absolutely.
I just kept the previous code, just adding the return private->state in 
the functions
in this patch.
merging the state and the return value is done in a later patch.
If you prefer I can do it in this patch.

>
>>   }
>>
>>   extern struct workqueue_struct *vfio_ccw_work_q;
>> -- 
>> 2.7.4
>>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany