On Mon, Sep 11, 2023 at 09:03:57AM -0700, Brett Creeley wrote:
> On 9/11/2023 7:24 AM, Dan Carpenter wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > Hello Brett Creeley,
> >
> > The patch bb500dbe2ac6: "vfio/pds: Add VFIO live migration support"
> > from Aug 7, 2023 (linux-next), leads to the following Smatch static
> > checker warning:
> >
> > drivers/vfio/pci/pds/lm.c:117 pds_vfio_put_save_file()
> > warn: sleeping in atomic context
> >
> > The call tree is:
> >
> > pds_vfio_state_mutex_unlock() <- disables preempt
> > -> pds_vfio_put_save_file() <- sleeps
> >
> > drivers/vfio/pci/pds/vfio_dev.c
> > 29 void pds_vfio_state_mutex_unlock(struct pds_vfio_pci_device *pds_vfio)
> > 30 {
> > 31 again:
> > 32 spin_lock(&pds_vfio->reset_lock);
> > ^^^^^^^^^
> > Preempt disabled
> >
> > 33 if (pds_vfio->deferred_reset) {
> > 34 pds_vfio->deferred_reset = false;
> > 35 if (pds_vfio->state == VFIO_DEVICE_STATE_ERROR) {
> > 36 pds_vfio_put_restore_file(pds_vfio);
> > 37 pds_vfio_put_save_file(pds_vfio);
> > ^^^^^^^^^^^^^^^^^^^^^^
> >
> > 38 pds_vfio_dirty_disable(pds_vfio, false);
> > 39 }
> > 40 pds_vfio->state = pds_vfio->deferred_reset_state;
> > 41 pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;
> > 42 spin_unlock(&pds_vfio->reset_lock);
> > 43 goto again;
> > 44 }
> > 45 mutex_unlock(&pds_vfio->state_mutex);
> > 46 spin_unlock(&pds_vfio->reset_lock);
> >
> > Unrelated but it really makes me itch that we drop the mutex before the
> > spinlock.
>
> This was done based on Mellanox's implementation and there's some
> history/notes on where this came from.
>
> AFAIK these are the relevant pointers for the original discussion and a code
> comment as well:
>
> Original thread:
> https://lore.kernel.org/netdev/20211019105838.227569-8-yishaih@xxxxxxxxxx/T/
>
> Also, there is a comment in drivers/vfio/pci/mlx5/main.c inside of
> mlx5vf_pci_aer_reset_done().
>
> Maybe it would be best to add a comment in pds_vfio_reset() as well?
Nah. It's fine. Or we could just add a comment like:
/* see mlx5vf_pci_aer_reset_done() to understand the lock ordering */
regards,
dan carpenter
