| Message ID | 20180921204013.95804-2-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | Refactor ccw_io_region | expand |
On Fri, 21 Sep 2018 22:40:12 +0200 Eric Farman <farman@linux.ibm.com> wrote: > In the event that we want to change the layout of the ccw_io_region in the > future[1], it might be easier to work with it as a pointer within the > vfio_ccw_private struct rather than an embedded struct. > > [1] https://patchwork.kernel.org/comment/22228541/ > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 12 +++++++++++- > drivers/s390/cio/vfio_ccw_fsm.c | 6 +++--- > drivers/s390/cio/vfio_ccw_ops.c | 4 ++-- > drivers/s390/cio/vfio_ccw_private.h | 2 +- > 4 files changed, 17 insertions(+), 7 deletions(-) > > @@ -114,6 +114,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) > private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA); > if (!private) > return -ENOMEM; > + > + private->io_region = kzalloc(sizeof(*private->io_region), > + GFP_KERNEL | GFP_DMA); Any particular reason for this to be under 2G? Looking through the code, I did not spot a place where we feed things in there directly to an interface that is 2G sensitive. I'm inclined to just keep it for now (as the structure it was taken from was also allocated below 2G), but this might be a remainder of the original implementation that was not using idals (and we might be able to get rid of GFP_DMA here). But maybe I'm just missing coffee :) > + if (!private->io_region) { > + kfree(private); > + return -ENOMEM; > + } > + > private->sch = sch; > dev_set_drvdata(&sch->dev, private); >
On 09/27/2018 07:13 AM, Cornelia Huck wrote: > On Fri, 21 Sep 2018 22:40:12 +0200 > Eric Farman <farman@linux.ibm.com> wrote: > >> In the event that we want to change the layout of the ccw_io_region in the >> future[1], it might be easier to work with it as a pointer within the >> vfio_ccw_private struct rather than an embedded struct. >> >> [1] https://patchwork.kernel.org/comment/22228541/ >> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_drv.c | 12 +++++++++++- >> drivers/s390/cio/vfio_ccw_fsm.c | 6 +++--- >> drivers/s390/cio/vfio_ccw_ops.c | 4 ++-- >> drivers/s390/cio/vfio_ccw_private.h | 2 +- >> 4 files changed, 17 insertions(+), 7 deletions(-) >> > >> @@ -114,6 +114,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) >> private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA); >> if (!private) >> return -ENOMEM; >> + >> + private->io_region = kzalloc(sizeof(*private->io_region), >> + GFP_KERNEL | GFP_DMA); > > Any particular reason for this to be under 2G? Looking through the > code, I did not spot a place where we feed things in there directly to > an interface that is 2G sensitive. > > I'm inclined to just keep it for now (as the structure it was taken > from was also allocated below 2G), but this might be a remainder of the > original implementation that was not using idals (and we might be able > to get rid of GFP_DMA here). I suspect you are right and it can be removed, since I don't see any reason it needs to be. But I don't know the history here, so I just kept the same allocation. I guess it makes sense to look into that cleanup here. But I did say this was a quick attempt. :) > > But maybe I'm just missing coffee :) Now that is the real tragedy right here! > >> + if (!private->io_region) { >> + kfree(private); >> + return -ENOMEM; >> + } >> + >> private->sch = sch; >> dev_set_drvdata(&sch->dev, private); >> >
On Thu, 27 Sep 2018 09:05:20 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 09/27/2018 07:13 AM, Cornelia Huck wrote: > > On Fri, 21 Sep 2018 22:40:12 +0200 > > Eric Farman <farman@linux.ibm.com> wrote: > > > >> In the event that we want to change the layout of the ccw_io_region in the > >> future[1], it might be easier to work with it as a pointer within the > >> vfio_ccw_private struct rather than an embedded struct. > >> > >> [1] https://patchwork.kernel.org/comment/22228541/ > >> > >> Signed-off-by: Eric Farman <farman@linux.ibm.com> > >> --- > >> drivers/s390/cio/vfio_ccw_drv.c | 12 +++++++++++- > >> drivers/s390/cio/vfio_ccw_fsm.c | 6 +++--- > >> drivers/s390/cio/vfio_ccw_ops.c | 4 ++-- > >> drivers/s390/cio/vfio_ccw_private.h | 2 +- > >> 4 files changed, 17 insertions(+), 7 deletions(-) > >> > > > >> @@ -114,6 +114,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) > >> private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA); > >> if (!private) > >> return -ENOMEM; > >> + > >> + private->io_region = kzalloc(sizeof(*private->io_region), > >> + GFP_KERNEL | GFP_DMA); > > > > Any particular reason for this to be under 2G? Looking through the > > code, I did not spot a place where we feed things in there directly to > > an interface that is 2G sensitive. > > > > I'm inclined to just keep it for now (as the structure it was taken > > from was also allocated below 2G), but this might be a remainder of the > > original implementation that was not using idals (and we might be able > > to get rid of GFP_DMA here). > > I suspect you are right and it can be removed, since I don't see any > reason it needs to be. But I don't know the history here, so I just > kept the same allocation. I guess it makes sense to look into that > cleanup here. But I did say this was a quick attempt. :) Yes, it makes sense to just keep the flags for now. We can still change it easily on top.
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 770fa9cfc310..f48e6f84eefe 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -79,7 +79,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) cp_update_scsw(&private->cp, &irb->scsw); cp_free(&private->cp); } - memcpy(private->io_region.irb_area, irb, sizeof(*irb)); + memcpy(private->io_region->irb_area, irb, sizeof(*irb)); if (private->io_trigger) eventfd_signal(private->io_trigger, 1); @@ -114,6 +114,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA); if (!private) return -ENOMEM; + + private->io_region = kzalloc(sizeof(*private->io_region), + GFP_KERNEL | GFP_DMA); + if (!private->io_region) { + kfree(private); + return -ENOMEM; + } + private->sch = sch; dev_set_drvdata(&sch->dev, private); @@ -139,6 +147,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) cio_disable_subchannel(sch); out_free: dev_set_drvdata(&sch->dev, NULL); + kfree(private->io_region); kfree(private); return ret; } @@ -153,6 +162,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch) dev_set_drvdata(&sch->dev, NULL); + kfree(private->io_region); kfree(private); return 0; diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 797a82731159..f94aa01f9c36 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -93,13 +93,13 @@ static void 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; + private->io_region->ret_code = -EIO; } static void fsm_io_busy(struct vfio_ccw_private *private, enum vfio_ccw_event event) { - private->io_region.ret_code = -EBUSY; + private->io_region->ret_code = -EBUSY; } static void fsm_disabled_irq(struct vfio_ccw_private *private, @@ -126,7 +126,7 @@ 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 ccw_io_region *io_region = private->io_region; struct mdev_device *mdev = private->mdev; char *errstr = "request"; diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 41eeb57d68a3..f673e106c041 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -174,7 +174,7 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev, return -EINVAL; private = dev_get_drvdata(mdev_parent_dev(mdev)); - region = &private->io_region; + region = private->io_region; if (copy_to_user(buf, (void *)region + *ppos, count)) return -EFAULT; @@ -196,7 +196,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, if (private->state != VFIO_CCW_STATE_IDLE) return -EACCES; - region = &private->io_region; + region = private->io_region; if (copy_from_user((void *)region + *ppos, buf, count)) return -EFAULT; diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index 78a66d96756b..078e46f9623d 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -41,7 +41,7 @@ struct vfio_ccw_private { atomic_t avail; struct mdev_device *mdev; struct notifier_block nb; - struct ccw_io_region io_region; + struct ccw_io_region *io_region; struct channel_program cp; struct irb irb;
In the event that we want to change the layout of the ccw_io_region in the future[1], it might be easier to work with it as a pointer within the vfio_ccw_private struct rather than an embedded struct. [1] https://patchwork.kernel.org/comment/22228541/ Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 12 +++++++++++- drivers/s390/cio/vfio_ccw_fsm.c | 6 +++--- drivers/s390/cio/vfio_ccw_ops.c | 4 ++-- drivers/s390/cio/vfio_ccw_private.h | 2 +- 4 files changed, 17 insertions(+), 7 deletions(-)