Re: [PATCH v11 01/17] pci: msi: pass pdev to pci_enable_msi() function

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 Dec 2023, Volodymyr Babchuk wrote:
> Previously pci_enable_msi() function obtained pdev pointer by itself,
> but taking into account upcoming changes to PCI locking, it is better
> when caller passes already acquired pdev pointer to the function,
> because caller knows better how to obtain the pointer and which locks
> are needed to be used. Also, in most cases caller already has pointer
> to pdev, so we can avoid an extra list walk.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> 
> ---
> In v11:
>  - Made pdev parameter very first in pci_enable_msi() and friends.
>  - Extended the commit message
>  - Added check for pdev into ns16550 driver
>  - Replaced hard tabs with spaces
> 
> Changes in v10:
> 
>  - New in v10. This is the result of discussion in "vpci: add initial
>  support for virtual PCI bus topology"
> ---
>  xen/arch/x86/include/asm/msi.h |  5 +++--
>  xen/arch/x86/irq.c             |  2 +-
>  xen/arch/x86/msi.c             | 19 ++++++++++---------
>  xen/drivers/char/ns16550.c     | 28 ++++++++++++++++++----------
>  4 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index c1ece2786e..07b3ee55e9 100644
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -81,8 +81,9 @@ struct irq_desc;
>  struct hw_interrupt_type;
>  struct msi_desc;
>  /* Helper functions */
> -extern int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc);
> -extern void pci_disable_msi(struct msi_desc *msi_desc);
> +extern int pci_enable_msi(struct pci_dev *pdev, struct msi_info *msi,
> +                          struct msi_desc **desc);
> +extern void pci_disable_msi(struct msi_desc *desc);

As the parameters name should match between declaration and definition,
you should also rename msi_desc to desc in the implementation of
pci_disable_msi. Or keep the parameter called "msi_desc" for
pci_disable_msi here. That change could be done on commit and everything
else looks OK:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



>  extern int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off);
>  extern void pci_cleanup_msi(struct pci_dev *pdev);
>  extern int setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc);
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 6e668b1b4f..50e49e1a4b 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2176,7 +2176,7 @@ int map_domain_pirq(
>          if ( !pdev )
>              goto done;
>  
> -        ret = pci_enable_msi(msi, &msi_desc);
> +        ret = pci_enable_msi(pdev, msi, &msi_desc);
>          if ( ret )
>          {
>              if ( ret > 0 )
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 7f8e794254..335c0868a2 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -983,13 +983,13 @@ static int msix_capability_init(struct pci_dev *dev,
>   * irq or non-zero for otherwise.
>   **/
>  
> -static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
> +static int __pci_enable_msi(struct pci_dev *pdev, struct msi_info *msi,
> +                            struct msi_desc **desc)
>  {
> -    struct pci_dev *pdev;
>      struct msi_desc *old_desc;
>  
>      ASSERT(pcidevs_locked());
> -    pdev = pci_get_pdev(NULL, msi->sbdf);
> +
>      if ( !pdev )
>          return -ENODEV;
>  
> @@ -1038,13 +1038,13 @@ static void __pci_disable_msi(struct msi_desc *entry)
>   * of irqs available. Driver should use the returned value to re-send
>   * its request.
>   **/
> -static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
> +static int __pci_enable_msix(struct pci_dev *pdev, struct msi_info *msi,
> +                             struct msi_desc **desc)
>  {
> -    struct pci_dev *pdev;
>      struct msi_desc *old_desc;
>  
>      ASSERT(pcidevs_locked());
> -    pdev = pci_get_pdev(NULL, msi->sbdf);
> +
>      if ( !pdev || !pdev->msix )
>          return -ENODEV;
>  
> @@ -1151,15 +1151,16 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool 
> off)
>   * Notice: only construct the msi_desc
>   * no change to irq_desc here, and the interrupt is masked
>   */
> -int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
> +int pci_enable_msi(struct pci_dev *pdev, struct msi_info *msi,
> +                   struct msi_desc **desc)
>  {
>      ASSERT(pcidevs_locked());
>  
>      if ( !use_msi )
>          return -EPERM;
>  
> -    return msi->table_base ? __pci_enable_msix(msi, desc) :
> -                             __pci_enable_msi(msi, desc);
> +    return msi->table_base ? __pci_enable_msix(pdev, msi, desc) :
> +                             __pci_enable_msi(pdev, msi, desc);
>  }
>  
>  /*
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index ddf2a48be6..cfe9ff8d2a 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -452,21 +452,29 @@ static void __init cf_check ns16550_init_postirq(struct 
> serial_port *port)
>              if ( rc > 0 )
>              {
>                  struct msi_desc *msi_desc = NULL;
> +                struct pci_dev *pdev;
>  
>                  pcidevs_lock();
>  
> -                rc = pci_enable_msi(&msi, &msi_desc);
> -                if ( !rc )
> +                pdev = pci_get_pdev(NULL, msi.sbdf);
> +                if ( pdev )
>                  {
> -                    struct irq_desc *desc = irq_to_desc(msi.irq);
> -                    unsigned long flags;
> -
> -                    spin_lock_irqsave(&desc->lock, flags);
> -                    rc = setup_msi_irq(desc, msi_desc);
> -                    spin_unlock_irqrestore(&desc->lock, flags);
> -                    if ( rc )
> -                        pci_disable_msi(msi_desc);
> +                    rc = pci_enable_msi(pdev, &msi, &msi_desc);
> +
> +                    if ( !rc )
> +                    {
> +                        struct irq_desc *desc = irq_to_desc(msi.irq);
> +                        unsigned long flags;
> +
> +                        spin_lock_irqsave(&desc->lock, flags);
> +                        rc = setup_msi_irq(desc, msi_desc);
> +                        spin_unlock_irqrestore(&desc->lock, flags);
> +                        if ( rc )
> +                            pci_disable_msi(msi_desc);
> +                    }
>                  }
> +                else
> +                    rc = -ENODEV;
>  
>                  pcidevs_unlock();
>  
> -- 
> 2.42.0
>