Messages in this thread | | | Subject | Re: [PATCH v13 4/9] PCI: Apply the new generic I/O management on PCI IO hosts | From | John Garry <> | Date | Wed, 14 Feb 2018 15:42:15 +0000 |
| |
On 13/02/2018 22:57, dann frazier wrote: > On Wed, Feb 14, 2018 at 01:45:28AM +0800, John Garry wrote: >> From: Zhichang Yuan <yuanzhichang@hisilicon.com> >> >> After introducing the new generic I/O space management in logic pio, the >> original PCI MMIO relevant helpers need to be updated based on the new >> interfaces. >> This patch adapts the corresponding code to match the changes introduced >> by logic pio. >> >> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> #earlier draft >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > I saw that Bjorn Acked this back in v6, but it seems like the code in > pci.c was reworked a bit for v7 onwards and I didn't see a follow-up > review (apologies if I just missed it). In which case, maybe his Ack > should have the "#earlier draft" tag as well? >
Hi Dann,
I see Bjorn acked again later on in the series, here: https://lkml.org/lkml/2017/5/26/612
>> --- >> drivers/pci/pci.c | 95 +++++++++--------------------------------------- >> include/asm-generic/io.h | 2 +- >> 2 files changed, 18 insertions(+), 79 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 07290a3..8aa5c54 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -22,6 +22,7 @@ >> #include <linux/spinlock.h> >> #include <linux/string.h> >> #include <linux/log2.h> >> +#include <linux/logic_pio.h> >> #include <linux/pci-aspm.h> >> #include <linux/pm_wakeup.h> >> #include <linux/interrupt.h> >> @@ -3440,17 +3441,6 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) >> } >> EXPORT_SYMBOL(pci_request_regions_exclusive); >> >> -#ifdef PCI_IOBASE >> -struct io_range { >> - struct list_head list; >> - phys_addr_t start; >> - resource_size_t size; >> -}; >> - >> -static LIST_HEAD(io_range_list); >> -static DEFINE_SPINLOCK(io_range_lock); >> -#endif >> - >> /* >> * Record the PCI IO range (expressed as CPU physical address + size). >> * Return a negative value if an error has occured, zero otherwise >> @@ -3458,51 +3448,28 @@ struct io_range { >> int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr, >> resource_size_t size) >> { >> - int err = 0; >> - >> + int ret = 0; >> #ifdef PCI_IOBASE >> - struct io_range *range; >> - resource_size_t allocated_size = 0; >> - >> - /* check if the range hasn't been previously recorded */ >> - spin_lock(&io_range_lock); >> - list_for_each_entry(range, &io_range_list, list) { >> - if (addr >= range->start && addr + size <= range->start + size) { >> - /* range already registered, bail out */ >> - goto end_register; >> - } >> - allocated_size += range->size; >> - } >> + struct logic_pio_hwaddr *range; >> >> - /* range not registed yet, check for available space */ >> - if (allocated_size + size - 1 > IO_SPACE_LIMIT) { >> - /* if it's too big check if 64K space can be reserved */ >> - if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) { >> - err = -E2BIG; >> - goto end_register; >> - } >> - >> - size = SZ_64K; >> - pr_warn("Requested IO range too big, new size set to 64K\n"); >> - } >> + if (!size || addr + size < addr) >> + return -EINVAL; >> >> - /* add the range to the list */ >> range = kzalloc(sizeof(*range), GFP_ATOMIC); >> - if (!range) { >> - err = -ENOMEM; >> - goto end_register; >> - } >> + if (!range) >> + return -ENOMEM; >> >> - range->start = addr; >> + range->fwnode = fwnode; >> range->size = size; >> + range->hw_start = addr; >> + range->flags = PIO_CPU_MMIO; >> >> - list_add_tail(&range->list, &io_range_list); >> - >> -end_register: >> - spin_unlock(&io_range_lock); >> + ret = logic_pio_register_range(range); >> + if (ret) >> + kfree(range); >> #endif >> >> - return err; >> + return ret; >> } >> >> phys_addr_t pci_pio_to_address(unsigned long pio) >> @@ -3510,21 +3477,10 @@ phys_addr_t pci_pio_to_address(unsigned long pio) >> phys_addr_t address = (phys_addr_t)OF_BAD_ADDR; >> >> #ifdef PCI_IOBASE >> - struct io_range *range; >> - resource_size_t allocated_size = 0; >> - >> - if (pio > IO_SPACE_LIMIT) >> + if (pio >= MMIO_UPPER_LIMIT) >> return address; >> >> - spin_lock(&io_range_lock); >> - list_for_each_entry(range, &io_range_list, list) { >> - if (pio >= allocated_size && pio < allocated_size + range->size) { >> - address = range->start + pio - allocated_size; >> - break; >> - } >> - allocated_size += range->size; >> - } >> - spin_unlock(&io_range_lock); >> + address = logic_pio_to_hwaddr(pio); >> #endif >> >> return address; >> @@ -3533,25 +3489,8 @@ phys_addr_t pci_pio_to_address(unsigned long pio) >> unsigned long __weak pci_address_to_pio(phys_addr_t address) >> { >> #ifdef PCI_IOBASE >> - struct io_range *res; >> - resource_size_t offset = 0; >> - unsigned long addr = -1; >> - >> - spin_lock(&io_range_lock); >> - list_for_each_entry(res, &io_range_list, list) { >> - if (address >= res->start && address < res->start + res->size) { >> - addr = address - res->start + offset; >> - break; >> - } >> - offset += res->size; >> - } >> - spin_unlock(&io_range_lock); >> - >> - return addr; >> + return logic_pio_trans_cpuaddr(address); >> #else >> - if (address > IO_SPACE_LIMIT) >> - return (unsigned long)-1; >> - > > Why is this check now safe to drop in the !PCI_IOBASE case?
So I have been studying the (long) patchset history and I don't see the reason for removing this check, and I think it can be reinstated.
The address and pio are analogous here, and pio should not exceed the IO space limit, so just a safety check.
> > -dann > >> return (unsigned long) address; >> #endif >> } >> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h >> index b7996a79..5a59931 100644 >> --- a/include/asm-generic/io.h >> +++ b/include/asm-generic/io.h >> @@ -901,7 +901,7 @@ static inline void iounmap(void __iomem *addr) >> #define ioport_map ioport_map >> static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) >> {
Thanks, John
>> - return PCI_IOBASE + (port & IO_SPACE_LIMIT); >> + return PCI_IOBASE + (port & MMIO_UPPER_LIMIT); >> } >> #endif >> > > . >
| |