Re: [PATCH] PCI: Remove pci_try_set_mwi
On Sun, Mar 28, 2021 at 12:04:35AM +0100, Heiner Kallweit wrote: > On 26.03.2021 22:26, Bjorn Helgaas wrote: > > [+cc Randy, Andrew (though I'm sure you have zero interest in this > > ancient question :))] > > > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit 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 > >> 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. > > > > There's not much I like better than removing things. But some > > significant thought went into adding pci_try_set_mwi() in the first > > place, so I need a little more convincing about why it's safe to > > remove it. > > > > Thanks for the link to the 13 yrs old discussion. Unfortunately it > doesn't mention any real argument for the __must_check, just: > > "And one of the reasons for adding the __must_check annotation is to > weed out design errors." > And the very next response in the discussion calls this a "non-argument". > Plus not mentioning what the other reasons could be. I think you're referring to Alan's response [1]: akpm> And we *need* to be excessively anal in the PCI setup code. akpm> We have metric shitloads of bugs due to problems in that area, akpm> and the more formality and error handling and error reporting akpm> we can get in there the better off we will be. ac> No argument there So Alan is actually *agreeing* that "we need to be excessively anal in the PCI setup code," not saying that "weeding out design errors is not an argument for __must_check." > Currently we have three ancient drivers that bail out if the call fails. > Most callers of pci_set_mwi() use the return code only to emit an > error message, but they proceed normally. Majority of users calls > pci_try_set_mwi(). And as stated in the commit message I don't expect > any new usage of pci_set_mwi(). I would love to merge this patch. We just need to clarify the commit log. Right now the only justification is "I don't think there's much sense in the __must_check annotation," which may well be true but could use some support. If MWI is purely an optimization and there's never a functional problem if pci_set_mwi() fails, we should say that (and maybe update any drivers that bail out on failure). Andrew and Alan both seem to agree that MSI *is* purely advisory: akpm> pci_set_mwi() is an advisory thing, and on certain platforms akpm> it might fail to set the cacheline size to the desired number. akpm> This is not a fatal error and the driver can successfully run akpm> at a lesser performance level. ac> Correct. But even after that, Andrew proposed adding pci_try_set_mwi(). So it makes sense to really understand what was going on there so we don't break something in the name of cleaning it up. [1] https://lore.kernel.org/linux-ide/20070405211609.5263d...@the-village.bc.nu/ > > The argument should cite the discussion about adding it. I think one > > of the earliest conversations is here: > > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/
Re: [PATCH] PCI: Remove pci_try_set_mwi
On 26.03.2021 22:26, Bjorn Helgaas wrote: > [+cc Randy, Andrew (though I'm sure you have zero interest in this > ancient question :))] > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit 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 >> 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. > > There's not much I like better than removing things. But some > significant thought went into adding pci_try_set_mwi() in the first > place, so I need a little more convincing about why it's safe to > remove it. > Thanks for the link to the 13 yrs old discussion. Unfortunately it doesn't mention any real argument for the __must_check, just: "And one of the reasons for adding the __must_check annotation is to weed out design errors." And the very next response in the discussion calls this a "non-argument". Plus not mentioning what the other reasons could be. Currently we have three ancient drivers that bail out if the call fails. Most callers of pci_set_mwi() use the return code only to emit an error message, but they proceed normally. Majority of users calls pci_try_set_mwi(). And as stated in the commit message I don't expect any new usage of pci_set_mwi(). > The argument should cite the discussion about adding it. I think one > of the earliest conversations is here: > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ > >> Signed-off-by: Heiner Kallweit >> --- >> patch applies on top of pci/misc for v5.11 >> --- >> Documentation/PCI/pci.rst | 5 + >> drivers/ata/pata_cs5530.c | 2 +- >> drivers/ata/sata_mv.c | 2 +- >> drivers/dma/dw/pci.c | 2 +- >> drivers/dma/hsu/pci.c | 2 +- >> drivers/ide/cs5530.c | 2 +- >> drivers/mfd/intel-lpss-pci.c | 2 +- >> drivers/net/ethernet/adaptec/starfire.c | 2 +- >> drivers/net/ethernet/alacritech/slicoss.c | 2 +- >> drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + >> drivers/net/ethernet/sun/cassini.c| 4 ++-- >> drivers/net/wireless/intersil/p54/p54pci.c| 2 +- >> .../intersil/prism54/islpci_hotplug.c | 3 +-- >> .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- >> drivers/pci/pci.c | 19 --- >> drivers/scsi/3w-9xxx.c| 4 ++-- >> drivers/scsi/3w-sas.c | 4 ++-- >> drivers/scsi/csiostor/csio_init.c | 2 +- >> drivers/scsi/lpfc/lpfc_init.c | 2 +- >> drivers/scsi/qla2xxx/qla_init.c | 8 >> drivers/scsi/qla2xxx/qla_mr.c | 2 +- >> drivers/tty/serial/8250/8250_lpss.c | 2 +- >> drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- >> drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- >> drivers/usb/gadget/udc/net2280.c | 2 +- >> drivers/usb/gadget/udc/pch_udc.c | 2 +- >> include/linux/pci.h | 5 ++--- >> 27 files changed, 33 insertions(+), 60 deletions(-) >> >> 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. >> >> >> Request MMIO/IOP resources >> diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c >> index ad75d02b6..8654b3ae1 100644 >> --- a/drivers/ata/pata_cs5530.c >> +++ b/drivers/ata/pata_cs5530.c >> @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) >> } >> >> pci_set_master(cs5530_0); >> -pci_try_set_mwi(cs5530_0); >> +pci_set_mwi(cs5530_0); >> >> /* >> * Set PCI CacheLineSize to 16-bytes: >> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c >> index 664ef658a..ee37755ea 100644 >> --- a/drivers/ata/sata_mv.c >> +++ b/drivers/ata/sata_mv.c >> @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, >> mv_print_info(host); >>
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Fri, Mar 26, 2021 at 11:42:46PM +0200, Andy Shevchenko wrote: > On Fri, Mar 26, 2021 at 04:26:55PM -0500, Bjorn Helgaas wrote: > > [+cc Randy, Andrew (though I'm sure you have zero interest in this > > ancient question :))] > > > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit 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 > > > 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. > > > > There's not much I like better than removing things. But some > > significant thought went into adding pci_try_set_mwi() in the first > > place, so I need a little more convincing about why it's safe to > > remove it. > > > > The argument should cite the discussion about adding it. I think one > > of the earliest conversations is here: > > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ > > It's solely PCI feature which is absent on PCIe. > > So, if there is a guarantee that the driver never services a device connected > to old PCI bus, it's okay to remove the call (it's no-op on PCIe anyway). Yes, I'm aware that MWI is a no-op on PCIe. If we want to argue that we don't need to support Conventional PCI devices, that should be explicit, and we could remove pci_set_mwi() completely. But I don't think we're ready to drop Conventional PCI support. > OTOH, PCI core may try MWI itself for every device (but this is an opposite, > what should we do on broken devices that do change their state based on that > bit while violating specification). > > In any case > > Acked-by: Andy Shevchenko Thanks! Bjorn
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Fri, Mar 26, 2021 at 04:26:55PM -0500, Bjorn Helgaas wrote: > [+cc Randy, Andrew (though I'm sure you have zero interest in this > ancient question :))] > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit 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 > > 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. > > There's not much I like better than removing things. But some > significant thought went into adding pci_try_set_mwi() in the first > place, so I need a little more convincing about why it's safe to > remove it. > > The argument should cite the discussion about adding it. I think one > of the earliest conversations is here: > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ It's solely PCI feature which is absent on PCIe. So, if there is a guarantee that the driver never services a device connected to old PCI bus, it's okay to remove the call (it's no-op on PCIe anyway). OTOH, PCI core may try MWI itself for every device (but this is an opposite, what should we do on broken devices that do change their state based on that bit while violating specification). In any case Acked-by: Andy Shevchenko for DesignWare DMA case. I have added that and I never saw that IP connected to the old PCI. -- With Best Regards, Andy Shevchenko
Re: [PATCH] PCI: Remove pci_try_set_mwi
[+cc Randy, Andrew (though I'm sure you have zero interest in this ancient question :))] On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit 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 > 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. There's not much I like better than removing things. But some significant thought went into adding pci_try_set_mwi() in the first place, so I need a little more convincing about why it's safe to remove it. The argument should cite the discussion about adding it. I think one of the earliest conversations is here: https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ > Signed-off-by: Heiner Kallweit > --- > patch applies on top of pci/misc for v5.11 > --- > Documentation/PCI/pci.rst | 5 + > drivers/ata/pata_cs5530.c | 2 +- > drivers/ata/sata_mv.c | 2 +- > drivers/dma/dw/pci.c | 2 +- > drivers/dma/hsu/pci.c | 2 +- > drivers/ide/cs5530.c | 2 +- > drivers/mfd/intel-lpss-pci.c | 2 +- > drivers/net/ethernet/adaptec/starfire.c | 2 +- > drivers/net/ethernet/alacritech/slicoss.c | 2 +- > drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + > drivers/net/ethernet/sun/cassini.c| 4 ++-- > drivers/net/wireless/intersil/p54/p54pci.c| 2 +- > .../intersil/prism54/islpci_hotplug.c | 3 +-- > .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- > drivers/pci/pci.c | 19 --- > drivers/scsi/3w-9xxx.c| 4 ++-- > drivers/scsi/3w-sas.c | 4 ++-- > drivers/scsi/csiostor/csio_init.c | 2 +- > drivers/scsi/lpfc/lpfc_init.c | 2 +- > drivers/scsi/qla2xxx/qla_init.c | 8 > drivers/scsi/qla2xxx/qla_mr.c | 2 +- > drivers/tty/serial/8250/8250_lpss.c | 2 +- > drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- > drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- > drivers/usb/gadget/udc/net2280.c | 2 +- > drivers/usb/gadget/udc/pch_udc.c | 2 +- > include/linux/pci.h | 5 ++--- > 27 files changed, 33 insertions(+), 60 deletions(-) > > 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. > > > Request MMIO/IOP resources > diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c > index ad75d02b6..8654b3ae1 100644 > --- a/drivers/ata/pata_cs5530.c > +++ b/drivers/ata/pata_cs5530.c > @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) > } > > pci_set_master(cs5530_0); > - pci_try_set_mwi(cs5530_0); > + pci_set_mwi(cs5530_0); > > /* >* Set PCI CacheLineSize to 16-bytes: > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > index 664ef658a..ee37755ea 100644 > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, > mv_print_info(host); > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED, >IS_GEN_I(hpriv) ? _sht : _sht); > } > 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 >
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Wed, Dec 9, 2020 at 12:59 PM Andy Shevchenko
wrote:
> On Wed, Dec 9, 2020 at 10:35 AM Heiner Kallweit wrote:
...
> > -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.
I see that pci_set_mwi() also has the ifdeffery (I thought it's only
here), so it's fine.
> > -}
> > -EXPORT_SYMBOL(pci_try_set_mwi);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Wed, Dec 9, 2020 at 10:35 AM Heiner Kallweit 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
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(>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 = 0xf000;
> 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
> @@
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Wed, 09 Dec 2020, Heiner Kallweit 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 > 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. > > Signed-off-by: Heiner Kallweit > --- > patch applies on top of pci/misc for v5.11 > --- > Documentation/PCI/pci.rst | 5 + > drivers/ata/pata_cs5530.c | 2 +- > drivers/ata/sata_mv.c | 2 +- > drivers/dma/dw/pci.c | 2 +- > drivers/dma/hsu/pci.c | 2 +- > drivers/ide/cs5530.c | 2 +- > drivers/mfd/intel-lpss-pci.c | 2 +- Acked-by: Lee Jones > drivers/net/ethernet/adaptec/starfire.c | 2 +- > drivers/net/ethernet/alacritech/slicoss.c | 2 +- > drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + > drivers/net/ethernet/sun/cassini.c| 4 ++-- > drivers/net/wireless/intersil/p54/p54pci.c| 2 +- > .../intersil/prism54/islpci_hotplug.c | 3 +-- > .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- > drivers/pci/pci.c | 19 --- > drivers/scsi/3w-9xxx.c| 4 ++-- > drivers/scsi/3w-sas.c | 4 ++-- > drivers/scsi/csiostor/csio_init.c | 2 +- > drivers/scsi/lpfc/lpfc_init.c | 2 +- > drivers/scsi/qla2xxx/qla_init.c | 8 > drivers/scsi/qla2xxx/qla_mr.c | 2 +- > drivers/tty/serial/8250/8250_lpss.c | 2 +- > drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- > drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- > drivers/usb/gadget/udc/net2280.c | 2 +- > drivers/usb/gadget/udc/pch_udc.c | 2 +- > include/linux/pci.h | 5 ++--- > 27 files changed, 33 insertions(+), 60 deletions(-) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH] PCI: Remove pci_try_set_mwi
On 20-12-09 09:31:21, Heiner Kallweit wrote: > drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- For chipidea changes: Acked-by: Peter Chen Peter > drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- > drivers/usb/gadget/udc/net2280.c | 2 +- > drivers/usb/gadget/udc/pch_udc.c | 2 +- > include/linux/pci.h | 5 ++--- > 27 files changed, 33 insertions(+), 60 deletions(-) > > 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. > > > Request MMIO/IOP resources > diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c > index ad75d02b6..8654b3ae1 100644 > --- a/drivers/ata/pata_cs5530.c > +++ b/drivers/ata/pata_cs5530.c > @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) > } > > pci_set_master(cs5530_0); > - pci_try_set_mwi(cs5530_0); > + pci_set_mwi(cs5530_0); > > /* >* Set PCI CacheLineSize to 16-bytes: > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > index 664ef658a..ee37755ea 100644 > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, > mv_print_info(host); > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED, >IS_GEN_I(hpriv) ? _sht : _sht); > } > 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/ide/cs5530.c b/drivers/ide/cs5530.c > index 5bb46e713..5d2c421ab 100644 > --- a/drivers/ide/cs5530.c > +++ b/drivers/ide/cs5530.c > @@ -168,7 +168,7 @@ static int init_chipset_cs5530(struct pci_dev *dev) >*/ > > pci_set_master(cs5530_0); > - pci_try_set_mwi(cs5530_0); > + pci_set_mwi(cs5530_0); > > /* >* Set PCI CacheLineSize to 16-bytes: > 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(>dev, info); > if (ret) > diff --git a/drivers/net/ethernet/adaptec/starfire.c > b/drivers/net/ethernet/adaptec/starfire.c > index 555299737..1dbff34c4 100644 > --- a/drivers/net/ethernet/adaptec/starfire.c > +++ b/drivers/net/ethernet/adaptec/starfire.c > @@ -679,7 +679,7 @@ static int starfire_init_one(struct pci_dev *pdev, > pci_set_master(pdev); > > /* enable MWI -- it vastly improves Rx performance on sparc64 */ > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > #ifdef ZEROCOPY > /* Starfire can do TCP/UDP checksumming */ > diff --git a/drivers/net/ethernet/alacritech/slicoss.c > b/drivers/net/ethernet/alacritech/slicoss.c > index 696517eae..544510f57 100644 > --- a/drivers/net/ethernet/alacritech/slicoss.c > +++ b/drivers/net/ethernet/alacritech/slicoss.c > @@ -1749,7 +1749,7 @@ static int slic_probe(struct pci_dev *pdev, const > struct pci_device_id *ent) > } > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > slic_configure_pci(pdev); > > diff --git
Re: [PATCH] PCI: Remove pci_try_set_mwi
On 09-12-20, 09:31, Heiner Kallweit 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 > 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. > > Signed-off-by: Heiner Kallweit > --- > patch applies on top of pci/misc for v5.11 > --- > drivers/dma/dw/pci.c | 2 +- > drivers/dma/hsu/pci.c | 2 +- Acked-By: Vinod Koul -- ~Vinod
Re: [PATCH] PCI: Remove pci_try_set_mwi
Heiner Kallweit writes: > 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 > 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. > > Signed-off-by: Heiner Kallweit > --- > patch applies on top of pci/misc for v5.11 > drivers/net/wireless/intersil/p54/p54pci.c| 2 +- > .../intersil/prism54/islpci_hotplug.c | 3 +-- > .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- For drivers/wireless: Acked-by: Kalle Valo -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
[PATCH] PCI: Remove pci_try_set_mwi
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 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. Signed-off-by: Heiner Kallweit --- patch applies on top of pci/misc for v5.11 --- Documentation/PCI/pci.rst | 5 + drivers/ata/pata_cs5530.c | 2 +- drivers/ata/sata_mv.c | 2 +- drivers/dma/dw/pci.c | 2 +- drivers/dma/hsu/pci.c | 2 +- drivers/ide/cs5530.c | 2 +- drivers/mfd/intel-lpss-pci.c | 2 +- drivers/net/ethernet/adaptec/starfire.c | 2 +- drivers/net/ethernet/alacritech/slicoss.c | 2 +- drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + drivers/net/ethernet/sun/cassini.c| 4 ++-- drivers/net/wireless/intersil/p54/p54pci.c| 2 +- .../intersil/prism54/islpci_hotplug.c | 3 +-- .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- drivers/pci/pci.c | 19 --- drivers/scsi/3w-9xxx.c| 4 ++-- drivers/scsi/3w-sas.c | 4 ++-- drivers/scsi/csiostor/csio_init.c | 2 +- drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/qla2xxx/qla_init.c | 8 drivers/scsi/qla2xxx/qla_mr.c | 2 +- drivers/tty/serial/8250/8250_lpss.c | 2 +- drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- drivers/usb/gadget/udc/net2280.c | 2 +- drivers/usb/gadget/udc/pch_udc.c | 2 +- include/linux/pci.h | 5 ++--- 27 files changed, 33 insertions(+), 60 deletions(-) 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. Request MMIO/IOP resources diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c index ad75d02b6..8654b3ae1 100644 --- a/drivers/ata/pata_cs5530.c +++ b/drivers/ata/pata_cs5530.c @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) } pci_set_master(cs5530_0); - pci_try_set_mwi(cs5530_0); + pci_set_mwi(cs5530_0); /* * Set PCI CacheLineSize to 16-bytes: diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index 664ef658a..ee37755ea 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, mv_print_info(host); pci_set_master(pdev); - pci_try_set_mwi(pdev); + pci_set_mwi(pdev); return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED, IS_GEN_I(hpriv) ? _sht : _sht); } 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/ide/cs5530.c b/drivers/ide/cs5530.c index 5bb46e713..5d2c421ab 100644 --- a/drivers/ide/cs5530.c +++ b/drivers/ide/cs5530.c @@ -168,7 +168,7 @@ static int init_chipset_cs5530(struct pci_dev *dev) */ pci_set_master(cs5530_0); - pci_try_set_mwi(cs5530_0); + pci_set_mwi(cs5530_0); /* * Set PCI CacheLineSize to 16-bytes: diff --git
