diff mbox series

[v2,7/9] vfio/ccw: Remove private->mdev

Message ID 7-v2-7d3a384024cf+2060-ccw_mdev_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Move vfio_ccw to the new mdev API | expand

Commit Message

Jason Gunthorpe Sept. 9, 2021, 7:38 p.m. UTC
Having a mdev pointer floating about in addition to a struct vfio_device
is confusing. It is only used for three things:

- Getting the mdev 'struct device *' - this is the same as
     private->vdev.dev

- Printing the uuid of the mdev in logging. The uuid is also the dev_name
  of the mdev so this is the same string as
     dev_name(private->vdev.dev)

- A weird attempt to fence the vfio_ccw_sch_io_todo() work. This work is
  only queued during states IDLE/PROCESSING/PENDING and flushed when
  entering CLOSED. Thus the work already cannot run when the mdev is NULL.
  Remove it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     |  6 ++--
 drivers/s390/cio/vfio_ccw_fsm.c     | 48 +++++++++++++----------------
 drivers/s390/cio/vfio_ccw_ops.c     | 16 ++++------
 drivers/s390/cio/vfio_ccw_private.h |  2 --
 include/linux/mdev.h                |  4 ---
 5 files changed, 30 insertions(+), 46 deletions(-)

Comments

Eric Farman Sept. 24, 2021, 8:45 p.m. UTC | #1
On Thu, 2021-09-09 at 16:38 -0300, Jason Gunthorpe wrote:
> Having a mdev pointer floating about in addition to a struct
> vfio_device
> is confusing. It is only used for three things:
> 
> - Getting the mdev 'struct device *' - this is the same as
>      private->vdev.dev
> 
> - Printing the uuid of the mdev in logging. The uuid is also the
> dev_name
>   of the mdev so this is the same string as
>      dev_name(private->vdev.dev)
> 
> - A weird attempt to fence the vfio_ccw_sch_io_todo() work. This work
> is
>   only queued during states IDLE/PROCESSING/PENDING and flushed when
>   entering CLOSED. Thus the work already cannot run when the mdev is
> NULL.
>   Remove it.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     |  6 ++--
>  drivers/s390/cio/vfio_ccw_fsm.c     | 48 +++++++++++++------------
> ----
>  drivers/s390/cio/vfio_ccw_ops.c     | 16 ++++------
>  drivers/s390/cio/vfio_ccw_private.h |  2 --
>  include/linux/mdev.h                |  4 ---
>  5 files changed, 30 insertions(+), 46 deletions(-)

I like this patch. Unfortunately it depends on the removal of a hunk in
patch 4, which sets the FSM state to different values based on whether
private->mdev is NULL or not, so can't go on its own. Need to spend
more time thinking about that patch.

Eric
Jason Gunthorpe Sept. 27, 2021, 12:32 p.m. UTC | #2
On Fri, Sep 24, 2021 at 04:45:02PM -0400, Eric Farman wrote:
> On Thu, 2021-09-09 at 16:38 -0300, Jason Gunthorpe wrote:
> > Having a mdev pointer floating about in addition to a struct
> > vfio_device
> > is confusing. It is only used for three things:
> > 
> > - Getting the mdev 'struct device *' - this is the same as
> >      private->vdev.dev
> > 
> > - Printing the uuid of the mdev in logging. The uuid is also the
> > dev_name
> >   of the mdev so this is the same string as
> >      dev_name(private->vdev.dev)
> > 
> > - A weird attempt to fence the vfio_ccw_sch_io_todo() work. This work
> > is
> >   only queued during states IDLE/PROCESSING/PENDING and flushed when
> >   entering CLOSED. Thus the work already cannot run when the mdev is
> > NULL.
> >   Remove it.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/s390/cio/vfio_ccw_drv.c     |  6 ++--
> >  drivers/s390/cio/vfio_ccw_fsm.c     | 48 +++++++++++++------------
> >  drivers/s390/cio/vfio_ccw_ops.c     | 16 ++++------
> >  drivers/s390/cio/vfio_ccw_private.h |  2 --
> >  include/linux/mdev.h                |  4 ---
> >  5 files changed, 30 insertions(+), 46 deletions(-)
> 
> I like this patch. Unfortunately it depends on the removal of a hunk in
> patch 4, which sets the FSM state to different values based on whether
> private->mdev is NULL or not, so can't go on its own. Need to spend
> more time thinking about that patch.

The FSM patch is important, really what is happening is the FSM logic
takes on the roles that was being split all over the place with other
logic, like this mdev stuff. To make that work we need a FSM that
makes sense..

Jason
Eric Farman Sept. 27, 2021, 8:45 p.m. UTC | #3
On Mon, 2021-09-27 at 09:32 -0300, Jason Gunthorpe wrote:
> On Fri, Sep 24, 2021 at 04:45:02PM -0400, Eric Farman wrote:
> > On Thu, 2021-09-09 at 16:38 -0300, Jason Gunthorpe wrote:
> > > Having a mdev pointer floating about in addition to a struct
> > > vfio_device
> > > is confusing. It is only used for three things:
> > > 
> > > - Getting the mdev 'struct device *' - this is the same as
> > >      private->vdev.dev
> > > 
> > > - Printing the uuid of the mdev in logging. The uuid is also the
> > > dev_name
> > >   of the mdev so this is the same string as
> > >      dev_name(private->vdev.dev)
> > > 
> > > - A weird attempt to fence the vfio_ccw_sch_io_todo() work. This
> > > work
> > > is
> > >   only queued during states IDLE/PROCESSING/PENDING and flushed
> > > when
> > >   entering CLOSED. Thus the work already cannot run when the mdev
> > > is
> > > NULL.
> > >   Remove it.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  drivers/s390/cio/vfio_ccw_drv.c     |  6 ++--
> > >  drivers/s390/cio/vfio_ccw_fsm.c     | 48 +++++++++++++--------
> > > ----
> > >  drivers/s390/cio/vfio_ccw_ops.c     | 16 ++++------
> > >  drivers/s390/cio/vfio_ccw_private.h |  2 --
> > >  include/linux/mdev.h                |  4 ---
> > >  5 files changed, 30 insertions(+), 46 deletions(-)
> > 
> > I like this patch. Unfortunately it depends on the removal of a
> > hunk in
> > patch 4, which sets the FSM state to different values based on
> > whether
> > private->mdev is NULL or not, so can't go on its own. Need to spend
> > more time thinking about that patch.
> 
> The FSM patch is important, really what is happening is the FSM logic
> takes on the roles that was being split all over the place with other
> logic, like this mdev stuff. To make that work we need a FSM that
> makes sense..

No argument from me about that. My point is that I could consume this
patch easier than the FSM patch, and need to get back to that one.

Eric

> 
> Jason
diff mbox series

Patch

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index de782e967a5474..0e2edd96567a09 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -64,7 +64,7 @@  static void vfio_ccw_sch_io_todo(struct work_struct *work)
 	 * has finished. Do not overwrite a possible processing
 	 * state if the final interrupt was for HSCH or CSCH.
 	 */
-	if (private->mdev && cp_is_finished)
+	if (cp_is_finished)
 		private->state = VFIO_CCW_STATE_IDLE;
 
 	if (private->io_trigger)
@@ -303,8 +303,8 @@  static int vfio_ccw_chp_event(struct subchannel *sch,
 		return 0;
 
 	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
-	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
-			   mdev_uuid(private->mdev), sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "%s (%x.%x.%04x): mask=0x%x event=%d\n",
+			   dev_name(private->vdev.dev), sch->schid.cssid,
 			   sch->schid.ssid, sch->schid.sch_no,
 			   mask, event);
 
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 302215090b9ac7..df1490943b20ec 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -245,7 +245,6 @@  static void fsm_io_request(struct vfio_ccw_private *private,
 	union orb *orb;
 	union scsw *scsw = &private->scsw;
 	struct ccw_io_region *io_region = private->io_region;
-	struct mdev_device *mdev = private->mdev;
 	char *errstr = "request";
 	struct subchannel_id schid = get_schid(private);
 
@@ -258,32 +257,30 @@  static void fsm_io_request(struct vfio_ccw_private *private,
 		/* Don't try to build a cp if transport mode is specified. */
 		if (orb->tm.b) {
 			io_region->ret_code = -EOPNOTSUPP;
-			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): transport mode\n",
-					   mdev_uuid(mdev), schid.cssid,
-					   schid.ssid, schid.sch_no);
+			VFIO_CCW_MSG_EVENT(
+				2, "%s (%x.%x.%04x): transport mode\n",
+				dev_name(private->vdev.dev), schid.cssid,
+				schid.ssid, schid.sch_no);
 			errstr = "transport mode";
 			goto err_out;
 		}
-		io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev),
+		io_region->ret_code = cp_init(&private->cp, private->vdev.dev,
 					      orb);
 		if (io_region->ret_code) {
-			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_init=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
-					   schid.ssid, schid.sch_no,
-					   io_region->ret_code);
+			VFIO_CCW_MSG_EVENT(2, "%s (%x.%x.%04x): cp_init=%d\n",
+					   dev_name(private->vdev.dev),
+					   schid.cssid, schid.ssid,
+					   schid.sch_no, io_region->ret_code);
 			errstr = "cp init";
 			goto err_out;
 		}
 
 		io_region->ret_code = cp_prefetch(&private->cp);
 		if (io_region->ret_code) {
-			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
-					   schid.ssid, schid.sch_no,
-					   io_region->ret_code);
+			VFIO_CCW_MSG_EVENT(
+				2, "%s (%x.%x.%04x): cp_prefetch=%d\n",
+				dev_name(private->vdev.dev), schid.cssid,
+				schid.ssid, schid.sch_no, io_region->ret_code);
 			errstr = "cp prefetch";
 			cp_free(&private->cp);
 			goto err_out;
@@ -292,28 +289,25 @@  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);
 		if (io_region->ret_code) {
-			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): fsm_io_helper=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
-					   schid.ssid, schid.sch_no,
-					   io_region->ret_code);
+			VFIO_CCW_MSG_EVENT(
+				2, "%s (%x.%x.%04x): fsm_io_helper=%d\n",
+				dev_name(private->vdev.dev), schid.cssid,
+				schid.ssid, schid.sch_no, io_region->ret_code);
 			errstr = "cp fsm_io_helper";
 			cp_free(&private->cp);
 			goto err_out;
 		}
 		return;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): halt on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+		VFIO_CCW_MSG_EVENT(2, "%s (%x.%x.%04x): halt on io_region\n",
+				   dev_name(private->vdev.dev), schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* halt is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
 		goto err_out;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): clear on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+		VFIO_CCW_MSG_EVENT(2, "%s (%x.%x.%04x): clear on io_region\n",
+				   dev_name(private->vdev.dev), schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* clear is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 38ab5c1f25ec09..23004e67c492f6 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -95,11 +95,9 @@  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
 
-	private->mdev = mdev;
-
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
-			   private->sch->schid.ssid,
+	VFIO_CCW_MSG_EVENT(2, "mdev %s, sch %x.%x.%04x: create\n",
+			   dev_name(private->vdev.dev),
+			   private->sch->schid.cssid, private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
 	ret = vfio_register_group_dev(&private->vdev);
@@ -110,7 +108,6 @@  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 
 err_init:
 	vfio_uninit_group_dev(&private->vdev);
-	private->mdev = NULL;
 	return ret;
 }
 
@@ -118,14 +115,13 @@  static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
 
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
-			   private->sch->schid.ssid,
+	VFIO_CCW_MSG_EVENT(2, "mdev %s, sch %x.%x.%04x: remove\n",
+			   dev_name(private->vdev.dev),
+			   private->sch->schid.cssid, private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
 	vfio_unregister_group_dev(&private->vdev);
 	vfio_uninit_group_dev(&private->vdev);
-	private->mdev = NULL;
 }
 
 static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index bbc97eb9d9c6fc..67ee9c624393b0 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -72,7 +72,6 @@  struct vfio_ccw_crw {
  * @sch: pointer to the subchannel
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
- * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
  * @io_mutex: protect against concurrent update of I/O regions
@@ -95,7 +94,6 @@  struct vfio_ccw_private {
 	struct subchannel	*sch;
 	int			state;
 	struct completion	*completion;
-	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
 	struct mutex		io_mutex;
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 819eaf98ffac73..ea59610d45fd89 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -158,10 +158,6 @@  static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
 {
 	mdev->driver_data = data;
 }
-static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
 
 extern struct bus_type mdev_bus_type;