| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 Oct 2022 13:21:19 -0400
From: Eric Farman <farman@...ux.ibm.com>
To: Matthew Rosato <mjrosato@...ux.ibm.com>,
Alex Williamson <alex.williamson@...hat.com>,
Cornelia Huck <cohuck@...hat.com>,
Jason Gunthorpe <jgg@...dia.com>,
Kevin Tian <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>
Cc: Zhenyu Wang <zhenyuw@...ux.intel.com>,
Zhi Wang <zhi.a.wang@...el.com>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Halil Pasic <pasic@...ux.ibm.com>,
Vineeth Vijayan <vneethv@...ux.ibm.com>,
Peter Oberparleiter <oberpar@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Sven Schnelle <svens@...ux.ibm.com>,
Tony Krowiak <akrowiak@...ux.ibm.com>,
Jason Herne <jjherne@...ux.ibm.com>,
Harald Freudenberger <freude@...ux.ibm.com>,
Diana Craciun <diana.craciun@....nxp.com>,
Eric Auger <eric.auger@...hat.com>,
Kirti Wankhede <kwankhede@...dia.com>,
Abhishek Sahu <abhsahu@...dia.com>,
Yishai Hadas <yishaih@...dia.com>,
intel-gvt-dev@...ts.freedesktop.org,
intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
kvm@...r.kernel.org
Subject: Re: [PATCH v1 1/7] vfio/ccw: create a parent struct
On Fri, 2022-10-28 at 12:51 -0400, Matthew Rosato wrote:
> On 10/19/22 12:21 PM, Eric Farman wrote:
> > Move the stuff associated with the mdev parent (and thus the
> > subchannel struct) into its own struct, and leave the rest in
> > the existing private structure.
> >
> > The subchannel will point to the parent, and the parent will point
> > to the private, for the areas where one or both are needed. Further
> > separation of these structs will follow.
> >
> > Signed-off-by: Eric Farman <farman@...ux.ibm.com>
> > ---
> > drivers/s390/cio/vfio_ccw_drv.c | 104 ++++++++++++++++++++----
> > ----
> > drivers/s390/cio/vfio_ccw_ops.c | 9 ++-
> > drivers/s390/cio/vfio_ccw_parent.h | 28 ++++++++
> > drivers/s390/cio/vfio_ccw_private.h | 5 --
> > 4 files changed, 112 insertions(+), 34 deletions(-)
> > create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 7f5402fe857a..634760ca0dea 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -20,6 +20,7 @@
> > #include "chp.h"
> > #include "ioasm.h"
> > #include "css.h"
> > +#include "vfio_ccw_parent.h"
> > #include "vfio_ccw_private.h"
> >
> > struct workqueue_struct *vfio_ccw_work_q;
> > @@ -36,7 +37,8 @@ debug_info_t *vfio_ccw_debug_trace_id;
> > */
> > int vfio_ccw_sch_quiesce(struct subchannel *sch)
> > {
> > - struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> > >dev);
> > + struct vfio_ccw_parent *parent = dev_get_drvdata(&sch-
> > >dev);
> > + struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> > >dev);
> > DECLARE_COMPLETION_ONSTACK(completion);
> > int iretry, ret = 0;
> >
> > @@ -51,19 +53,21 @@ int vfio_ccw_sch_quiesce(struct subchannel
> > *sch)
> > break;
> > }
> >
> > - /*
> > - * Flush all I/O and wait for
> > - * cancel/halt/clear completion.
> > - */
> > - private->completion = &completion;
> > - spin_unlock_irq(sch->lock);
> > + if (private) {
>
> Is it valid to ever reach this code with private == NULL? If no,
> then this should probably be a WARN_ON upfront?
Hrm, the caller jumps from private -> subchannel, so it would be weird
if we couldn't then go back the other way. Probably impossible, I'll
unwind these whitespace changes and put the WARN_ON on top. Thanks for
the tip.
>
> > + /*
> > + * Flush all I/O and wait for
> > + * cancel/halt/clear completion.
> > + */
> > + private->completion = &completion;
> > + spin_unlock_irq(sch->lock);
> >
> > - if (ret == -EBUSY)
> > - wait_for_completion_timeout(&completion,
> > 3*HZ);
> > + if (ret == -EBUSY)
> > + wait_for_completion_timeout(&comple
> > tion, 3*HZ);
> >
> > - private->completion = NULL;
> > - flush_workqueue(vfio_ccw_work_q);
> > - spin_lock_irq(sch->lock);
> > + private->completion = NULL;
> > + flush_workqueue(vfio_ccw_work_q);
> > + spin_lock_irq(sch->lock);
> > + }
> > ret = cio_disable_subchannel(sch);
> > } while (ret == -EBUSY);
> >
>
> .. snip ..
>
> > diff --git a/drivers/s390/cio/vfio_ccw_parent.h
> > b/drivers/s390/cio/vfio_ccw_parent.h
> > new file mode 100644
> > index 000000000000..834c00077802
> > --- /dev/null
> > +++ b/drivers/s390/cio/vfio_ccw_parent.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * MDEV Parent contents for vfio_ccw driver
> > + *
> > + * Copyright IBM Corp. 2022
> > + */
> > +
> > +#ifndef _VFIO_CCW_PARENT_H_
> > +#define _VFIO_CCW_PARENT_H_
> > +
> > +#include <linux/mdev.h>
> > +
> > +/**
> > + * struct vfio_ccw_parent
> > + *
> > + * @dev: embedded device struct
> > + * @parent: parent data structures for mdevs created
> > + * @mdev_type(s): identifying information for mdevs created
> > + */
> > +struct vfio_ccw_parent {
> > + struct device dev;
> > +
> > + struct mdev_parent parent;
> > + struct mdev_type mdev_type;
> > + struct mdev_type *mdev_types[1];
> > +};
>
> Structure itself seems fine, but any reason we need a new file for
> it?
>
Not really. I could leave it in _private.h, but that file is just a
dumping ground for everything so I thought this would be a good
opportunity to start to cleaning that up. But it wouldn't bother me to
leave that whole process to another day too.
Powered by blists - more mailing lists