The vfio/pds series for locking updates/fixes in the following link
made some changes that can also be done for other vendor's vfio
drivers. Specifically, changing the reset lock from a spinlock to mutex
and also calling mutex_destroy() in the vfio device release callback.
https://lore.kernel.org/kvm/[email protected]/
So, this series makes these changes in order to remain separate from
the vfio/pds series linked above.
Note, that I don't have the required hardware to test on this vendor's
hardware, so help would be appreciated.
Brett Creeley (2):
hisi_acc_vfio_pci: Change reset_lock to mutex_lock
hisi_acc_vfio_pci: Destroy the [state|reset]_mutex on release
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 25 +++++++++++++------
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +--
2 files changed, 19 insertions(+), 9 deletions(-)
--
2.17.1
Based on comments from other vfio vendors and the
maintainer the vfio/pds driver changed the reset_lock
to a mutex_lock. As part of that change it was requested
that the other vendor drivers be changed as well. So,
make the change.
The comment that requested the change for reference:
https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA0A@BN9PR11MB5276.namprd11.prod.outlook.com/
Also, make checkpatch happy by moving the lock comment.
Signed-off-by: Brett Creeley <[email protected]>
---
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index b2f9778c8366..2c049b8de4b4 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -638,17 +638,17 @@ static void
hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
{
again:
- spin_lock(&hisi_acc_vdev->reset_lock);
+ mutex_lock(&hisi_acc_vdev->reset_mutex);
if (hisi_acc_vdev->deferred_reset) {
hisi_acc_vdev->deferred_reset = false;
- spin_unlock(&hisi_acc_vdev->reset_lock);
+ mutex_unlock(&hisi_acc_vdev->reset_mutex);
hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
hisi_acc_vf_disable_fds(hisi_acc_vdev);
goto again;
}
mutex_unlock(&hisi_acc_vdev->state_mutex);
- spin_unlock(&hisi_acc_vdev->reset_lock);
+ mutex_unlock(&hisi_acc_vdev->reset_mutex);
}
static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
@@ -1108,13 +1108,13 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
* In case the state_mutex was taken already we defer the cleanup work
* to the unlock flow of the other running context.
*/
- spin_lock(&hisi_acc_vdev->reset_lock);
+ mutex_lock(&hisi_acc_vdev->reset_mutex);
hisi_acc_vdev->deferred_reset = true;
if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
- spin_unlock(&hisi_acc_vdev->reset_lock);
+ mutex_unlock(&hisi_acc_vdev->reset_mutex);
return;
}
- spin_unlock(&hisi_acc_vdev->reset_lock);
+ mutex_unlock(&hisi_acc_vdev->reset_mutex);
hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
}
@@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
hisi_acc_vdev->pf_qm = pf_qm;
hisi_acc_vdev->vf_dev = pdev;
mutex_init(&hisi_acc_vdev->state_mutex);
+ mutex_init(&hisi_acc_vdev->reset_mutex);
core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY;
core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
index dcabfeec6ca1..ed5ab332d0f3 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
@@ -109,8 +109,7 @@ struct hisi_acc_vf_core_device {
struct hisi_qm vf_qm;
u32 vf_qm_state;
int vf_id;
- /* For reset handler */
- spinlock_t reset_lock;
+ struct mutex reset_mutex; /* For reset handler */
struct hisi_acc_vf_migration_file *resuming_migf;
struct hisi_acc_vf_migration_file *saving_migf;
};
--
2.17.1
> -----Original Message-----
> From: Brett Creeley [mailto:[email protected]]
> Sent: 22 November 2023 19:37
> To: [email protected]; [email protected]; liulongfang
> <[email protected]>; Shameerali Kolothum Thodi
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]
> Subject: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock
>
> Based on comments from other vfio vendors and the
> maintainer the vfio/pds driver changed the reset_lock
> to a mutex_lock. As part of that change it was requested
> that the other vendor drivers be changed as well. So,
> make the change.
>
> The comment that requested the change for reference:
> https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA
> [email protected]/
>
> Also, make checkpatch happy by moving the lock comment.
>
> Signed-off-by: Brett Creeley <[email protected]>
> ---
> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------
> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +--
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index b2f9778c8366..2c049b8de4b4 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -638,17 +638,17 @@ static void
> hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> *hisi_acc_vdev)
> {
> again:
> - spin_lock(&hisi_acc_vdev->reset_lock);
> + mutex_lock(&hisi_acc_vdev->reset_mutex);
> if (hisi_acc_vdev->deferred_reset) {
> hisi_acc_vdev->deferred_reset = false;
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + mutex_unlock(&hisi_acc_vdev->reset_mutex);
Don't think we have that sleeping while atomic case for this here.
Same for mlx5 as well. But if the idea is to have a common locking
across vendor drivers, it is fine.
Reviewed-by: Shameer Kolothum <[email protected]>
Thanks,
Shameer
> hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> hisi_acc_vf_disable_fds(hisi_acc_vdev);
> goto again;
> }
> mutex_unlock(&hisi_acc_vdev->state_mutex);
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + mutex_unlock(&hisi_acc_vdev->reset_mutex);
> }
>
> static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device
> *hisi_acc_vdev)
> @@ -1108,13 +1108,13 @@ static void
> hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
> * In case the state_mutex was taken already we defer the cleanup work
> * to the unlock flow of the other running context.
> */
> - spin_lock(&hisi_acc_vdev->reset_lock);
> + mutex_lock(&hisi_acc_vdev->reset_mutex);
> hisi_acc_vdev->deferred_reset = true;
> if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + mutex_unlock(&hisi_acc_vdev->reset_mutex);
> return;
> }
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + mutex_unlock(&hisi_acc_vdev->reset_mutex);
> hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> }
>
> @@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct
> vfio_device *core_vdev)
> hisi_acc_vdev->pf_qm = pf_qm;
> hisi_acc_vdev->vf_dev = pdev;
> mutex_init(&hisi_acc_vdev->state_mutex);
> + mutex_init(&hisi_acc_vdev->reset_mutex);
>
> core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY |
> VFIO_MIGRATION_PRE_COPY;
> core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..ed5ab332d0f3 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -109,8 +109,7 @@ struct hisi_acc_vf_core_device {
> struct hisi_qm vf_qm;
> u32 vf_qm_state;
> int vf_id;
> - /* For reset handler */
> - spinlock_t reset_lock;
> + struct mutex reset_mutex; /* For reset handler */
> struct hisi_acc_vf_migration_file *resuming_migf;
> struct hisi_acc_vf_migration_file *saving_migf;
> };
> --
> 2.17.1
On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote:
> > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > index b2f9778c8366..2c049b8de4b4 100644
> > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > @@ -638,17 +638,17 @@ static void
> > hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> > *hisi_acc_vdev)
> > {
> > again:
> > - spin_lock(&hisi_acc_vdev->reset_lock);
> > + mutex_lock(&hisi_acc_vdev->reset_mutex);
> > if (hisi_acc_vdev->deferred_reset) {
> > hisi_acc_vdev->deferred_reset = false;
> > - spin_unlock(&hisi_acc_vdev->reset_lock);
> > + mutex_unlock(&hisi_acc_vdev->reset_mutex);
>
> Don't think we have that sleeping while atomic case for this here.
> Same for mlx5 as well. But if the idea is to have a common locking
> across vendor drivers, it is fine.
Yeah, I'm not sure about changing spinlocks to mutex's for no reason..
If we don't sleep and don't hold it for very long then the spinlock is
appropriate
Jason
> From: Jason Gunthorpe <[email protected]>
> Sent: Tuesday, November 28, 2023 8:46 AM
>
> On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote:
> > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > index b2f9778c8366..2c049b8de4b4 100644
> > > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > @@ -638,17 +638,17 @@ static void
> > > hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> > > *hisi_acc_vdev)
> > > {
> > > again:
> > > - spin_lock(&hisi_acc_vdev->reset_lock);
> > > + mutex_lock(&hisi_acc_vdev->reset_mutex);
> > > if (hisi_acc_vdev->deferred_reset) {
> > > hisi_acc_vdev->deferred_reset = false;
> > > - spin_unlock(&hisi_acc_vdev->reset_lock);
> > > + mutex_unlock(&hisi_acc_vdev->reset_mutex);
> >
> > Don't think we have that sleeping while atomic case for this here.
> > Same for mlx5 as well. But if the idea is to have a common locking
> > across vendor drivers, it is fine.
>
> Yeah, I'm not sure about changing spinlocks to mutex's for no reason..
> If we don't sleep and don't hold it for very long then the spinlock is
> appropriate
>
It's me suggesting Brett to fix other two drivers, expecting a common
locking pattern would cause less confusion to future new variant drivers.
If both of you don't think it necessary then let's drop it. ????