On Fri, 15 Feb 2019 17:13:21 -0500 Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote: > On 2/14/19 8:51 AM, Pierre Morel wrote: > > We need to find the queue with a specific APQN during the > > handling of the interception of the PQAP/AQIC instruction. > > > > To handle the AP associated device reference count we keep > > track of it in the vfio_ap_queue until we put the device. > > > > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx> > > --- > > drivers/s390/crypto/vfio_ap_ops.c | 54 +++++++++++++++++++++++++++++++++++ > > drivers/s390/crypto/vfio_ap_private.h | 1 + > > 2 files changed, 55 insertions(+) > > +/** > > + * vfio_ap_get_queue: Retrieve a queue with a specific APQN > > + * @apqn: The queue APQN > > + * > > + * Retrieve a queue with a specific APQN from the list of the > > + * devices associated to the vfio_ap_driver. > > + * > > + * The vfio_ap_queue has been already associated with the device > > + * during the probe. > > + * Store the associated device for reference counting > > + * > > + * Returns the pointer to the associated vfio_ap_queue > > + */ > > +static __attribute__((unused)) > > + struct vfio_ap_queue *vfio_ap_get_queue(int apqn) > > I think you should change this signature for the reasons I stated > below: > > struct device *vfio_ap_get_queue_dev(int apqn) > > > +{ > > + struct device *dev; > > + struct vfio_ap_queue *q; > > + > > + dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn, > > + vfio_ap_check_apqn); > > + if (!dev) > > + return NULL; > > + q = dev_get_drvdata(dev); > > + q->dev = dev; > > Why store the device with the vfio_ap_queue object? Why not just return > the device. The caller can get the vfio_ap_queue from the device's > driver data. It seems the only reason for the 'dev' field is to > temporarily hold a ref to the device so it can be put later. Why not > just put the device. Having looked at the remainder of the patches, I tend to agree that we don't really need the backlink; we walk the driver's list of devices in any case IIUC. We *might* want a mechanism to grab the queue quickly (i.e. without walking the list) if there's anything performance sensitive in there; but from the patch descriptions, I don't think anything is done in a hot path, so it should be fine.