Re: [PATCH] PCI: Remove pci_try_set_mwi

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Dec 9, 2020 at 10:35 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> pci_set_mwi() and pci_try_set_mwi() do exactly the same, just that the
> former one is declared as __must_check. However also some callers of

However also -> However

> pci_set_mwi() have a comment that it's an optional feature. I don't
> think there's much sense in this separation and the use of
> __must_check. Therefore remove pci_try_set_mwi() and remove the
> __must_check attribute from pci_set_mwi().


> I don't expect either function to be used in new code anyway.

You probably want to elaborate here that the feature is specific to
PCI and isn't present on PCIe.

Besides that one comment below.
After addressing, have my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for the files left in this message.

...

>  drivers/dma/dw/pci.c                          |  2 +-
>  drivers/dma/hsu/pci.c                         |  2 +-

>  drivers/mfd/intel-lpss-pci.c                  |  2 +-

>  drivers/pci/pci.c                             | 19 -------------------

>  drivers/tty/serial/8250/8250_lpss.c           |  2 +-
>  drivers/usb/chipidea/ci_hdrc_pci.c            |  2 +-

>  drivers/usb/gadget/udc/pch_udc.c              |  2 +-
>  include/linux/pci.h                           |  5 ++---

> diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst
> index 814b40f83..120362cc9 100644
> --- a/Documentation/PCI/pci.rst
> +++ b/Documentation/PCI/pci.rst
> @@ -226,10 +226,7 @@ If the PCI device can use the PCI Memory-Write-Invalidate transaction,
>  call pci_set_mwi().  This enables the PCI_COMMAND bit for Mem-Wr-Inval
>  and also ensures that the cache line size register is set correctly.
>  Check the return value of pci_set_mwi() as not all architectures
> -or chip-sets may support Memory-Write-Invalidate.  Alternatively,
> -if Mem-Wr-Inval would be nice to have but is not required, call
> -pci_try_set_mwi() to have the system do its best effort at enabling
> -Mem-Wr-Inval.
> +or chip-sets may support Memory-Write-Invalidate.

...

> diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
> index 1142aa6f8..1c20b7485 100644
> --- a/drivers/dma/dw/pci.c
> +++ b/drivers/dma/dw/pci.c
> @@ -30,7 +30,7 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid)
>         }
>
>         pci_set_master(pdev);
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
>         if (ret)
> diff --git a/drivers/dma/hsu/pci.c b/drivers/dma/hsu/pci.c
> index 07cc7320a..420dd3706 100644
> --- a/drivers/dma/hsu/pci.c
> +++ b/drivers/dma/hsu/pci.c
> @@ -73,7 +73,7 @@ static int hsu_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>         }
>
>         pci_set_master(pdev);
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
>         if (ret)

...

> diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
> index 2d7c588ef..a0c3be750 100644
> --- a/drivers/mfd/intel-lpss-pci.c
> +++ b/drivers/mfd/intel-lpss-pci.c
> @@ -39,7 +39,7 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
>
>         /* Probably it is enough to set this for iDMA capable devices only */
>         pci_set_master(pdev);
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         ret = intel_lpss_probe(&pdev->dev, info);
>         if (ret)

...

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9a5500287..f0ab432d2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4389,25 +4389,6 @@ int pcim_set_mwi(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL(pcim_set_mwi);
>
> -/**
> - * pci_try_set_mwi - enables memory-write-invalidate PCI transaction
> - * @dev: the PCI device for which MWI is enabled
> - *
> - * Enables the Memory-Write-Invalidate transaction in %PCI_COMMAND.
> - * Callers are not required to check the return value.
> - *
> - * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> - */
> -int pci_try_set_mwi(struct pci_dev *dev)
> -{

> -#ifdef PCI_DISABLE_MWI
> -       return 0;
> -#else
> -       return pci_set_mwi(dev);
> -#endif

This seems still valid case for PowerPC and SH.

> -}
> -EXPORT_SYMBOL(pci_try_set_mwi);

...

> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 4dee8a9e0..8acc1e5c9 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -193,7 +193,7 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>         if (ret)
>                 return;
>
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         /* Special DMA address for UART */
>         dma->rx_dma_addr = 0xfffff000;
> diff --git a/drivers/usb/chipidea/ci_hdrc_pci.c b/drivers/usb/chipidea/ci_hdrc_pci.c
> index d63479e1a..d412fa910 100644
> --- a/drivers/usb/chipidea/ci_hdrc_pci.c
> +++ b/drivers/usb/chipidea/ci_hdrc_pci.c
> @@ -78,7 +78,7 @@ static int ci_hdrc_pci_probe(struct pci_dev *pdev,
>         }
>
>         pci_set_master(pdev);
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         /* register a nop PHY */
>         ci->phy = usb_phy_generic_register();

...

> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index a3c1fc924..4a0484a0c 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -3100,7 +3100,7 @@ static int pch_udc_probe(struct pci_dev *pdev,
>         }
>
>         pci_set_master(pdev);
> -       pci_try_set_mwi(pdev);
> +       pci_set_mwi(pdev);
>
>         /* device struct setup */
>         spin_lock_init(&dev->lock);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index de75f6a4d..c590f616d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1191,9 +1191,8 @@ void pci_clear_master(struct pci_dev *dev);
>
>  int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
>  int pci_set_cacheline_size(struct pci_dev *dev);
> -int __must_check pci_set_mwi(struct pci_dev *dev);
> -int __must_check pcim_set_mwi(struct pci_dev *dev);
> -int pci_try_set_mwi(struct pci_dev *dev);
> +int pci_set_mwi(struct pci_dev *dev);
> +int pcim_set_mwi(struct pci_dev *dev);
>  void pci_clear_mwi(struct pci_dev *dev);
>  void pci_intx(struct pci_dev *dev, int enable);
>  bool pci_check_and_mask_intx(struct pci_dev *dev);


-- 
With Best Regards,
Andy Shevchenko