diff mbox

[v2,5/5] vfio: ccw: add traceponits for interesting error paths

Message ID 20180423110113.59385-6-bjsdjshi@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dong Jia Shi April 23, 2018, 11:01 a.m. UTC
From: Halil Pasic <pasic@linux.vnet.ibm.com>

Add some tracepoints so we can inspect what is not working as is should.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 drivers/s390/cio/Makefile         |  1 +
 drivers/s390/cio/vfio_ccw_fsm.c   | 16 +++++++-
 drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_trace.h

Comments

Cornelia Huck April 27, 2018, 10:13 a.m. UTC | #1
On Mon, 23 Apr 2018 13:01:13 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

typo in subject: s/traceponits/tracepoints/

> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> Add some tracepoints so we can inspect what is not working as is should.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/Makefile         |  1 +
>  drivers/s390/cio/vfio_ccw_fsm.c   | 16 +++++++-
>  drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_trace.h


> @@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  			goto err_out;
>  
>  		io_region->ret_code = cp_prefetch(&private->cp);
> +		trace_vfio_ccw_cp_prefetch(get_schid(private),
> +					   io_region->ret_code);
>  		if (io_region->ret_code) {
>  			cp_free(&private->cp);
>  			goto err_out;
> @@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  
>  		/* Start channel program and wait for I/O interrupt. */
>  		io_region->ret_code = fsm_io_helper(private);
> +		trace_vfio_ccw_fsm_io_helper(get_schid(private),
> +					     io_region->ret_code);
>  		if (io_region->ret_code) {
>  			cp_free(&private->cp);
>  			goto err_out;
>  		}
> -		return;
> +		goto out;
>  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>  		/* XXX: Handle halt. */
>  		io_region->ret_code = -EOPNOTSUPP;
> @@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  
>  err_out:
>  	private->state = VFIO_CCW_STATE_IDLE;
> +out:
> +	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
> +			       io_region->ret_code);
>  }
>  
>  /*

I really don't want to bikeshed, especially as some tracepoints are
better than no tracepoints, but...

We now trace fctl/schid/ret_code unconditionally (good).

We trace the outcome of cp_prefetch() and fsm_io_helper()
unconditionally. We don't, however, trace all things that may go wrong.
We have the tracepoint at the end, but it cannot tell us where the
error came from. Should we have tracepoints in every place (in this
function) that may generate an error? Only if there is an actual error?
Are the two enough for common debug scenarios?

Opinions? We can just go ahead with this and improve things later on, I
guess.
Cornelia Huck April 30, 2018, 11:51 a.m. UTC | #2
On Sat, 28 Apr 2018 13:50:23 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2018-04-27 12:13:53 +0200]:
> 
> > On Mon, 23 Apr 2018 13:01:13 +0200
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> > 
> > typo in subject: s/traceponits/tracepoints/
> >   
> > > From: Halil Pasic <pasic@linux.vnet.ibm.com>
> > > 
> > > Add some tracepoints so we can inspect what is not working as is should.
> > > 
> > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > > ---
> > >  drivers/s390/cio/Makefile         |  1 +
> > >  drivers/s390/cio/vfio_ccw_fsm.c   | 16 +++++++-
> > >  drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 93 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/s390/cio/vfio_ccw_trace.h  
> > 
> >   
> > > @@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> > >  			goto err_out;
> > >  
> > >  		io_region->ret_code = cp_prefetch(&private->cp);
> > > +		trace_vfio_ccw_cp_prefetch(get_schid(private),
> > > +					   io_region->ret_code);
> > >  		if (io_region->ret_code) {
> > >  			cp_free(&private->cp);
> > >  			goto err_out;
> > > @@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> > >  
> > >  		/* Start channel program and wait for I/O interrupt. */
> > >  		io_region->ret_code = fsm_io_helper(private);
> > > +		trace_vfio_ccw_fsm_io_helper(get_schid(private),
> > > +					     io_region->ret_code);
> > >  		if (io_region->ret_code) {
> > >  			cp_free(&private->cp);
> > >  			goto err_out;
> > >  		}
> > > -		return;
> > > +		goto out;
> > >  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
> > >  		/* XXX: Handle halt. */
> > >  		io_region->ret_code = -EOPNOTSUPP;
> > > @@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> > >  
> > >  err_out:
> > >  	private->state = VFIO_CCW_STATE_IDLE;
> > > +out:
> > > +	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
> > > +			       io_region->ret_code);
> > >  }
> > >  
> > >  /*  
> > 
> > I really don't want to bikeshed, especially as some tracepoints are
> > better than no tracepoints, but...
> > 
> > We now trace fctl/schid/ret_code unconditionally (good).
> > 
> > We trace the outcome of cp_prefetch() and fsm_io_helper()
> > unconditionally. We don't, however, trace all things that may go wrong.
> > We have the tracepoint at the end, but it cannot tell us where the
> > error came from. Should we have tracepoints in every place (in this
> > function) that may generate an error? Only if there is an actual error?
> > Are the two enough for common debug scenarios?  
> Trace actual error sounds like a better idea than trace unconditionally
> of these two functions.
> These two are not enough for common debug scenarios. For example, we
> cann't tell if a -EOPNOTSUPP is a orb->tm.b problem, or error code
> returned by cp_init().
> 
> Idea to improve:
> 1. Trace actual error.
> 2. Define a trace event and add error trace for cp_init().

Hm. Going from what I have done in the past when doing printk debugging:

- stick in a message that is always hit, with some information about
  parameters, if it makes sense
- stick in a message "foo happened!" in the error branches
   - or, alternatively, trace the called functions

So tracing on failure only might be more useful? Have all failure paths
under a common knob to turn on/off?

> > Opinions? We can just go ahead with this and improve things later
> > on, I guess.
> >   
> I think it's also fine to do this - better something than nothing. We
> could at least have a code base to be improved to make everybody
> happier in future.

Maybe keep the patch as it is now, except trace the errors only
(keeping the fctl trace point)?

Halil, as you wrote the patch (and I presume you found it helpful):
What is your opinion?
Halil Pasic April 30, 2018, 2:14 p.m. UTC | #3
On 04/30/2018 01:51 PM, Cornelia Huck wrote:
> On Sat, 28 Apr 2018 13:50:23 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Cornelia Huck <cohuck@redhat.com> [2018-04-27 12:13:53 +0200]:
>>
>>> On Mon, 23 Apr 2018 13:01:13 +0200
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>
>>> typo in subject: s/traceponits/tracepoints/
>>>    
>>>> From: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>
>>>> Add some tracepoints so we can inspect what is not working as is should.
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>> ---
>>>>   drivers/s390/cio/Makefile         |  1 +
>>>>   drivers/s390/cio/vfio_ccw_fsm.c   | 16 +++++++-
>>>>   drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 93 insertions(+), 1 deletion(-)
>>>>   create mode 100644 drivers/s390/cio/vfio_ccw_trace.h
>>>
>>>    
>>>> @@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>>>   			goto err_out;
>>>>   
>>>>   		io_region->ret_code = cp_prefetch(&private->cp);
>>>> +		trace_vfio_ccw_cp_prefetch(get_schid(private),
>>>> +					   io_region->ret_code);
>>>>   		if (io_region->ret_code) {
>>>>   			cp_free(&private->cp);
>>>>   			goto err_out;
>>>> @@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>>>   
>>>>   		/* Start channel program and wait for I/O interrupt. */
>>>>   		io_region->ret_code = fsm_io_helper(private);
>>>> +		trace_vfio_ccw_fsm_io_helper(get_schid(private),
>>>> +					     io_region->ret_code);
>>>>   		if (io_region->ret_code) {
>>>>   			cp_free(&private->cp);
>>>>   			goto err_out;
>>>>   		}
>>>> -		return;
>>>> +		goto out;
>>>>   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>>>>   		/* XXX: Handle halt. */
>>>>   		io_region->ret_code = -EOPNOTSUPP;
>>>> @@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>>>   
>>>>   err_out:
>>>>   	private->state = VFIO_CCW_STATE_IDLE;
>>>> +out:
>>>> +	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
>>>> +			       io_region->ret_code);
>>>>   }
>>>>   
>>>>   /*
>>>
>>> I really don't want to bikeshed, especially as some tracepoints are
>>> better than no tracepoints, but...
>>>
>>> We now trace fctl/schid/ret_code unconditionally (good).
>>>
>>> We trace the outcome of cp_prefetch() and fsm_io_helper()
>>> unconditionally. We don't, however, trace all things that may go wrong.
>>> We have the tracepoint at the end, but it cannot tell us where the
>>> error came from. Should we have tracepoints in every place (in this
>>> function) that may generate an error? Only if there is an actual error?
>>> Are the two enough for common debug scenarios?
>> Trace actual error sounds like a better idea than trace unconditionally
>> of these two functions.
>> These two are not enough for common debug scenarios. For example, we
>> cann't tell if a -EOPNOTSUPP is a orb->tm.b problem, or error code
>> returned by cp_init().
>>
>> Idea to improve:
>> 1. Trace actual error.
>> 2. Define a trace event and add error trace for cp_init().
> 
> Hm. Going from what I have done in the past when doing printk debugging:
> 
> - stick in a message that is always hit, with some information about
>    parameters, if it makes sense
> - stick in a message "foo happened!" in the error branches
>     - or, alternatively, trace the called functions
> 
> So tracing on failure only might be more useful? Have all failure paths
> under a common knob to turn on/off?
> 
>>> Opinions? We can just go ahead with this and improve things later
>>> on, I guess.
>>>    
>> I think it's also fine to do this - better something than nothing. We
>> could at least have a code base to be improved to make everybody
>> happier in future.
> 
> Maybe keep the patch as it is now, except trace the errors only
> (keeping the fctl trace point)?

What do you mean by this sentence. Get rid of vfio_ccw_io_fctl or get
rid of vfio_ccw_cp_prefetch and vfio_ccw_fsm_io_helper, or get don't
get rid of any, but make some conditional (!errno)?

> 
> Halil, as you wrote the patch (and I presume you found it helpful):
> What is your opinion?
> 

I'm in favor of this patch (as previously stated here
https://patchwork.kernel.org/patch/10298305/). And regarding the
questions under discussion I'm mostly fine either way.

I think the naming of this fctl thing is a bit cryptic,
but if we don't see this as ABI I'm fine with it -- can be improved.
What would be a better name? I was thinking along the lines accept_request.
(Bad error code would mean that the request did not get accepted. Good
code does not mean the requested function was performed successfully.)

Also I think vfio_ccw_io_fctl with no zero error code would make sense
as dev_warn. If I were an admin looking into a problem I would very much
appreciate seeing something in the messages log (and not having to enable
tracing first). This point seems to be a good one for high level 'request gone
bad' kind of report. Opinions?

Regards,
Halil
Cornelia Huck April 30, 2018, 3:03 p.m. UTC | #4
On Mon, 30 Apr 2018 16:14:21 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 04/30/2018 01:51 PM, Cornelia Huck wrote:
> > On Sat, 28 Apr 2018 13:50:23 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> >> * Cornelia Huck <cohuck@redhat.com> [2018-04-27 12:13:53 +0200]:
> >>  
> >>> On Mon, 23 Apr 2018 13:01:13 +0200
> >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >>>
> >>> typo in subject: s/traceponits/tracepoints/
> >>>      
> >>>> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>>
> >>>> Add some tracepoints so we can inspect what is not working as is should.
> >>>>
> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> >>>> ---
> >>>>   drivers/s390/cio/Makefile         |  1 +
> >>>>   drivers/s390/cio/vfio_ccw_fsm.c   | 16 +++++++-
> >>>>   drivers/s390/cio/vfio_ccw_trace.h | 77 +++++++++++++++++++++++++++++++++++++++
> >>>>   3 files changed, 93 insertions(+), 1 deletion(-)
> >>>>   create mode 100644 drivers/s390/cio/vfio_ccw_trace.h  
> >>>
> >>>      
> >>>> @@ -135,6 +142,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> >>>>   			goto err_out;
> >>>>   
> >>>>   		io_region->ret_code = cp_prefetch(&private->cp);
> >>>> +		trace_vfio_ccw_cp_prefetch(get_schid(private),
> >>>> +					   io_region->ret_code);
> >>>>   		if (io_region->ret_code) {
> >>>>   			cp_free(&private->cp);
> >>>>   			goto err_out;
> >>>> @@ -142,11 +151,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> >>>>   
> >>>>   		/* Start channel program and wait for I/O interrupt. */
> >>>>   		io_region->ret_code = fsm_io_helper(private);
> >>>> +		trace_vfio_ccw_fsm_io_helper(get_schid(private),
> >>>> +					     io_region->ret_code);
> >>>>   		if (io_region->ret_code) {
> >>>>   			cp_free(&private->cp);
> >>>>   			goto err_out;
> >>>>   		}
> >>>> -		return;
> >>>> +		goto out;
> >>>>   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
> >>>>   		/* XXX: Handle halt. */
> >>>>   		io_region->ret_code = -EOPNOTSUPP;
> >>>> @@ -159,6 +170,9 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> >>>>   
> >>>>   err_out:
> >>>>   	private->state = VFIO_CCW_STATE_IDLE;
> >>>> +out:
> >>>> +	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
> >>>> +			       io_region->ret_code);
> >>>>   }
> >>>>   
> >>>>   /*  
> >>>
> >>> I really don't want to bikeshed, especially as some tracepoints are
> >>> better than no tracepoints, but...
> >>>
> >>> We now trace fctl/schid/ret_code unconditionally (good).
> >>>
> >>> We trace the outcome of cp_prefetch() and fsm_io_helper()
> >>> unconditionally. We don't, however, trace all things that may go wrong.
> >>> We have the tracepoint at the end, but it cannot tell us where the
> >>> error came from. Should we have tracepoints in every place (in this
> >>> function) that may generate an error? Only if there is an actual error?
> >>> Are the two enough for common debug scenarios?  
> >> Trace actual error sounds like a better idea than trace unconditionally
> >> of these two functions.
> >> These two are not enough for common debug scenarios. For example, we
> >> cann't tell if a -EOPNOTSUPP is a orb->tm.b problem, or error code
> >> returned by cp_init().
> >>
> >> Idea to improve:
> >> 1. Trace actual error.
> >> 2. Define a trace event and add error trace for cp_init().  
> > 
> > Hm. Going from what I have done in the past when doing printk debugging:
> > 
> > - stick in a message that is always hit, with some information about
> >    parameters, if it makes sense
> > - stick in a message "foo happened!" in the error branches
> >     - or, alternatively, trace the called functions
> > 
> > So tracing on failure only might be more useful? Have all failure paths
> > under a common knob to turn on/off?
> >   
> >>> Opinions? We can just go ahead with this and improve things later
> >>> on, I guess.
> >>>      
> >> I think it's also fine to do this - better something than nothing. We
> >> could at least have a code base to be improved to make everybody
> >> happier in future.  
> > 
> > Maybe keep the patch as it is now, except trace the errors only
> > (keeping the fctl trace point)?  
> 
> What do you mean by this sentence. Get rid of vfio_ccw_io_fctl or get
> rid of vfio_ccw_cp_prefetch and vfio_ccw_fsm_io_helper, or get don't
> get rid of any, but make some conditional (!errno)?

The third option.

> 
> > 
> > Halil, as you wrote the patch (and I presume you found it helpful):
> > What is your opinion?
> >   
> 
> I'm in favor of this patch (as previously stated here
> https://patchwork.kernel.org/patch/10298305/). And regarding the
> questions under discussion I'm mostly fine either way.

OK.

> 
> I think the naming of this fctl thing is a bit cryptic,
> but if we don't see this as ABI I'm fine with it -- can be improved.
> What would be a better name? I was thinking along the lines accept_request.
> (Bad error code would mean that the request did not get accepted. Good
> code does not mean the requested function was performed successfully.)

I think fctl is fine (if you don't understand what 'fctl' is, you're
unlikely to understand it even if it were named differently.)

> 
> Also I think vfio_ccw_io_fctl with no zero error code would make sense
> as dev_warn. If I were an admin looking into a problem I would very much
> appreciate seeing something in the messages log (and not having to enable
> tracing first). This point seems to be a good one for high level 'request gone
> bad' kind of report. Opinions?

I'd also exclude -EOPNOTSUPP (as this also might happen with e.g. a halt/clear enabled user space, which probes availability of halt/clear support by giving it a try once (yes, I really want to post my patches this week.))
Halil Pasic April 30, 2018, 4:51 p.m. UTC | #5
On 04/30/2018 05:03 PM, Cornelia Huck wrote:
>> I think the naming of this fctl thing is a bit cryptic,
>> but if we don't see this as ABI I'm fine with it -- can be improved.
>> What would be a better name? I was thinking along the lines accept_request.
>> (Bad error code would mean that the request did not get accepted. Good
>> code does not mean the requested function was performed successfully.)
> I think fctl is fine (if you don't understand what 'fctl' is, you're
> unlikely to understand it even if it were named differently.)
> 

AFAIU this fctl is a bit more complicated than the normal fctl. But
better let sleeping dogs lie.

>> Also I think vfio_ccw_io_fctl with no zero error code would make sense
>> as dev_warn. If I were an admin looking into a problem I would very much
>> appreciate seeing something in the messages log (and not having to enable
>> tracing first). This point seems to be a good one for high level 'request gone
>> bad' kind of report. Opinions?
> I'd also exclude -EOPNOTSUPP (as this also might happen with e.g. a halt/clear enabled user space, which probes availability of halt/clear support by giving it a try once (yes, I really want to post my patches this week.))
> 

I'm looking forward to the clear/halt. It hope it will help me understand
the big vfio-ccw picture better. There are still dark spots, but I don't
feel like doing something against this, as there is quite some activity
going on here -- and I don't want to hamper the efforts by binding resources.

Regards,
Halil
Cornelia Huck May 22, 2018, 12:55 p.m. UTC | #6
On Wed, 16 May 2018 14:53:55 +0800
Dong Jia Shi <bjsdjshi@linux.ibm.com> wrote:

> Politely ping. Want a new version?

Just walking through my backlog now, sorry about the delay. Yes, a new
version would be easiest for me.
diff mbox

Patch

diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index a070ef0efe65..f230516abb96 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -5,6 +5,7 @@ 
 
 # The following is required for define_trace.h to find ./trace.h
 CFLAGS_trace.o := -I$(src)
+CFLAGS_vfio_ccw_fsm.o := -I$(src)
 
 obj-y += airq.o blacklist.o chsc.o cio.o css.o chp.o idset.o isc.o \
 	fcx.o itcw.o crw.o ccwreq.o trace.o ioasm.o
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index ff6963ad6e39..90cab4e1436e 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -13,6 +13,9 @@ 
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
 
+#define CREATE_TRACE_POINTS
+#include "vfio_ccw_trace.h"
+
 static int fsm_io_helper(struct vfio_ccw_private *private)
 {
 	struct subchannel *sch;
@@ -105,6 +108,10 @@  static void fsm_disabled_irq(struct vfio_ccw_private *private,
 	 */
 	cio_disable_subchannel(sch);
 }
+inline struct subchannel_id get_schid(struct vfio_ccw_private *p)
+{
+	return p->sch->schid;
+}
 
 /*
  * Deal with the ccw command request from the userspace.
@@ -135,6 +142,8 @@  static void fsm_io_request(struct vfio_ccw_private *private,
 			goto err_out;
 
 		io_region->ret_code = cp_prefetch(&private->cp);
+		trace_vfio_ccw_cp_prefetch(get_schid(private),
+					   io_region->ret_code);
 		if (io_region->ret_code) {
 			cp_free(&private->cp);
 			goto err_out;
@@ -142,11 +151,13 @@  static void fsm_io_request(struct vfio_ccw_private *private,
 
 		/* Start channel program and wait for I/O interrupt. */
 		io_region->ret_code = fsm_io_helper(private);
+		trace_vfio_ccw_fsm_io_helper(get_schid(private),
+					     io_region->ret_code);
 		if (io_region->ret_code) {
 			cp_free(&private->cp);
 			goto err_out;
 		}
-		return;
+		goto out;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
 		/* XXX: Handle halt. */
 		io_region->ret_code = -EOPNOTSUPP;
@@ -159,6 +170,9 @@  static void fsm_io_request(struct vfio_ccw_private *private,
 
 err_out:
 	private->state = VFIO_CCW_STATE_IDLE;
+out:
+	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
+			       io_region->ret_code);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_trace.h b/drivers/s390/cio/vfio_ccw_trace.h
new file mode 100644
index 000000000000..25128331c285
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_trace.h
@@ -0,0 +1,77 @@ 
+/* SPDX-License-Identifier: GPL-2.0
+ * Tracepoints for vfio_ccw driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
+ *            Halil Pasic <pasic@linux.vnet.ibm.com>
+ */
+
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfio_ccw
+
+#if !defined(_VFIO_CCW_TRACE_) || defined(TRACE_HEADER_MULTI_READ)
+#define _VFIO_CCW_TRACE_
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(vfio_ccw_schid_errno,
+	TP_PROTO(struct subchannel_id schid, int errno),
+	TP_ARGS(schid, errno),
+
+	TP_STRUCT__entry(
+		__field_struct(struct subchannel_id, schid)
+		__field(int, errno)
+	),
+
+	TP_fast_assign(
+		__entry->schid = schid;
+		__entry->errno = errno;
+	),
+
+	TP_printk("schid=%x.%x.%04x errno=%d", __entry->schid.cssid,
+		  __entry->schid.ssid, __entry->schid.sch_no, __entry->errno)
+);
+
+DEFINE_EVENT(vfio_ccw_schid_errno, vfio_ccw_cp_prefetch,
+	TP_PROTO(struct subchannel_id schid, int errno),
+	TP_ARGS(schid, errno)
+);
+
+DEFINE_EVENT(vfio_ccw_schid_errno, vfio_ccw_fsm_io_helper,
+	TP_PROTO(struct subchannel_id schid, int errno),
+	TP_ARGS(schid, errno)
+);
+
+TRACE_EVENT(vfio_ccw_io_fctl,
+	TP_PROTO(int fctl, struct subchannel_id schid, int errno),
+	TP_ARGS(fctl, schid, errno),
+
+	TP_STRUCT__entry(
+		__field(int, fctl)
+		__field_struct(struct subchannel_id, schid)
+		__field(int, errno)
+	),
+
+	TP_fast_assign(
+		__entry->fctl = fctl;
+		__entry->schid = schid;
+		__entry->errno = errno;
+	),
+
+	TP_printk("schid=%x.%x.%04x fctl=%x errno=%d", __entry->schid.cssid,
+		  __entry->schid.ssid, __entry->schid.sch_no,
+		  __entry->fctl, __entry->errno)
+);
+
+#endif /* _VFIO_CCW_TRACE_ */
+
+/* This part must be outside protection */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE vfio_ccw_trace
+
+#include <trace/define_trace.h>