| Message ID | 20220602171948.2790690-4-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO ccw/mdev rework | expand |
On 6/2/22 1:19 PM, Eric Farman wrote: > The mdev is linked with the vfio_ccw_private pointer when the mdev > is probed, but it's not cleared once the mdev is removed. > > This isn't much of a concern based on the current device lifecycle, > but fix it so that things make sense in later shuffling. > > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_ops.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > index a403d059a4e6..a0a3200b0b04 100644 > --- a/drivers/s390/cio/vfio_ccw_ops.c > +++ b/drivers/s390/cio/vfio_ccw_ops.c > @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) > private->sch->schid.ssid, > private->sch->schid.sch_no); > > + dev_set_drvdata(&mdev->dev, NULL); > vfio_unregister_group_dev(&private->vdev); > > if ((private->state != VFIO_CCW_STATE_NOT_OPER) && Seems harmless enough. Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> But is this just precautionary or is it fixing a real problem (if the former I don't think a fixes tag makes sense) I also ask because I note vfio-ap clears its driver_data in mdev_remove but also leaves the pointer set, meaning they might need a similar cleanup and should probably have a look (CC Tony & Jason H)
On Fri, Jun 03, 2022 at 09:25:19AM -0400, Matthew Rosato wrote: > On 6/2/22 1:19 PM, Eric Farman wrote: > > The mdev is linked with the vfio_ccw_private pointer when the mdev > > is probed, but it's not cleared once the mdev is removed. > > > > This isn't much of a concern based on the current device lifecycle, > > but fix it so that things make sense in later shuffling. > > > > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > drivers/s390/cio/vfio_ccw_ops.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > > index a403d059a4e6..a0a3200b0b04 100644 > > +++ b/drivers/s390/cio/vfio_ccw_ops.c > > @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) > > private->sch->schid.ssid, > > private->sch->schid.sch_no); > > + dev_set_drvdata(&mdev->dev, NULL); > > vfio_unregister_group_dev(&private->vdev); > > if ((private->state != VFIO_CCW_STATE_NOT_OPER) && > > Seems harmless enough. > > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> > > But is this just precautionary or is it fixing a real problem (if the former > I don't think a fixes tag makes sense) > > I also ask because I note vfio-ap clears its driver_data in mdev_remove but > also leaves the pointer set, meaning they might need a similar cleanup and > should probably have a look (CC Tony & Jason H) There should be no reason to clear the drvdata on remove - the driver must be designed to guarentee all references to the dev stop before the remove function returns. Jason
On 6/3/22 9:37 AM, Jason Gunthorpe wrote: > On Fri, Jun 03, 2022 at 09:25:19AM -0400, Matthew Rosato wrote: >> On 6/2/22 1:19 PM, Eric Farman wrote: >>> The mdev is linked with the vfio_ccw_private pointer when the mdev >>> is probed, but it's not cleared once the mdev is removed. >>> >>> This isn't much of a concern based on the current device lifecycle, >>> but fix it so that things make sense in later shuffling. >>> >>> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") >>> Signed-off-by: Eric Farman <farman@linux.ibm.com> >>> drivers/s390/cio/vfio_ccw_ops.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c >>> index a403d059a4e6..a0a3200b0b04 100644 >>> +++ b/drivers/s390/cio/vfio_ccw_ops.c >>> @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) >>> private->sch->schid.ssid, >>> private->sch->schid.sch_no); >>> + dev_set_drvdata(&mdev->dev, NULL); >>> vfio_unregister_group_dev(&private->vdev); >>> if ((private->state != VFIO_CCW_STATE_NOT_OPER) && >> Seems harmless enough. >> >> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> >> >> But is this just precautionary or is it fixing a real problem (if the former >> I don't think a fixes tag makes sense) >> >> I also ask because I note vfio-ap clears its driver_data in mdev_remove but >> also leaves the pointer set, meaning they might need a similar cleanup and >> should probably have a look (CC Tony & Jason H) > There should be no reason to clear the drvdata on remove - the driver > must be designed to guarentee all references to the dev stop before > the remove function returns. Each function that gets the driver data backs a sysfs attribute of the mdev. If the mdev is removed, these attributes will not exist. The mdev remove callback must get the same locks acquired by the sysfs attribute functions before clearing the driver data, so I believe this guarantees that all references to the dev stop before the remove function returns. > > Jason
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index a403d059a4e6..a0a3200b0b04 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) private->sch->schid.ssid, private->sch->schid.sch_no); + dev_set_drvdata(&mdev->dev, NULL); vfio_unregister_group_dev(&private->vdev); if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
The mdev is linked with the vfio_ccw_private pointer when the mdev is probed, but it's not cleared once the mdev is removed. This isn't much of a concern based on the current device lifecycle, but fix it so that things make sense in later shuffling. Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_ops.c | 1 + 1 file changed, 1 insertion(+)