On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote: > Hi Liviu > > > -----Original Message----- > > From: liviu.dudau@xxxxxxx [mailto:liviu.dudau@xxxxxxx] > > Sent: 09 November 2016 16:51 > > To: Gabriele Paoloni > > Cc: Yuanzhichang; catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; > > robh+dt@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; mark.rutland@xxxxxxx; > > olof@xxxxxxxxx; arnd@xxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > lorenzo.pieralisi@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Linuxarm; > > devicetree@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux- > > serial@xxxxxxxxxxxxxxx; minyard@xxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; > > zourongrong@xxxxxxxxx; John Garry; zhichang.yuan02@xxxxxxxxx; > > kantyzc@xxxxxxx; xuwei (O) > > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for > > special ISA > > > > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote: > > > Hi Liviu > > > > > > Thanks for reviewing > > > > > > > [removed some irrelevant part of discussion, avoid crazy formatting] > > > > > > > +/** > > > > > + * addr_is_indirect_io - check whether the input taddr is for > > > > indirectIO. > > > > > + * @taddr: the io address to be checked. > > > > > + * > > > > > + * Returns 1 when taddr is in the range; otherwise return 0. > > > > > + */ > > > > > +int addr_is_indirect_io(u64 taddr) > > > > > +{ > > > > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end > > < > > > > taddr) > > > > > > > > start >= taddr ? > > > > > > Nope... if (taddr < arm64_extio_ops->start || taddr > > > arm64_extio_ops->end) > > > then taddr is outside the range [start; end] and will return 0; > > otherwise > > > it will return 1... > > > > Oops, sorry, did not pay attention to the returned value. The check is > > correct as it is, no need to change then. > > > > > > > > > > > > > > + return 0; > > > > > + > > > > > + return 1; > > > > > +} > > > > > > > > > > BUILD_EXTIO(b, u8) > > > > > > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > > > > index 02b2903..cc2a05d 100644 > > > > > --- a/drivers/of/address.c > > > > > +++ b/drivers/of/address.c > > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct > > > > device_node *np) > > > > > return false; > > > > > } > > > > > > > > > > + > > > > > +/* > > > > > + * of_isa_indirect_io - get the IO address from some isa reg > > > > property value. > > > > > + * For some isa/lpc devices, no ranges property in ancestor > > node. > > > > > + * The device addresses are described directly in their regs > > > > property. > > > > > + * This fixup function will be called to get the IO address of > > > > isa/lpc > > > > > + * devices when the normal of_translation failed. > > > > > + * > > > > > + * @parent: points to the parent dts node; > > > > > + * @bus: points to the of_bus which can be used to parse > > > > address; > > > > > + * @addr: the address from reg property; > > > > > + * @na: the address cell counter of @addr; > > > > > + * @presult: store the address paresed from @addr; > > > > > + * > > > > > + * return 1 when successfully get the I/O address; > > > > > + * 0 will return for some failures. > > > > > > > > Bah, you are returning a signed int, why 0 for failure? Return a > > > > negative value with > > > > error codes. Otherwise change the return value into a bool. > > > > > > Yes we'll move to bool > > > > > > > > > > > > + */ > > > > > +static int of_get_isa_indirect_io(struct device_node *parent, > > > > > + struct of_bus *bus, __be32 *addr, > > > > > + int na, u64 *presult) > > > > > +{ > > > > > + unsigned int flags; > > > > > + unsigned int rlen; > > > > > + > > > > > + /* whether support indirectIO */ > > > > > + if (!indirect_io_enabled()) > > > > > + return 0; > > > > > + > > > > > + if (!of_bus_isa_match(parent)) > > > > > + return 0; > > > > > + > > > > > + flags = bus->get_flags(addr); > > > > > + if (!(flags & IORESOURCE_IO)) > > > > > + return 0; > > > > > + > > > > > + /* there is ranges property, apply the normal translation > > > > directly. */ > > > > > > > > s/there is ranges/if we have a 'ranges'/ > > > > > > Thanks for spotting this > > > > > > > > > > > > + if (of_get_property(parent, "ranges", &rlen)) > > > > > + return 0; > > > > > + > > > > > + *presult = of_read_number(addr + 1, na - 1); > > > > > + /* this fixup is only valid for specific I/O range. */ > > > > > + return addr_is_indirect_io(*presult); > > > > > +} > > > > > + > > > > > static int of_translate_one(struct device_node *parent, struct > > > > of_bus *bus, > > > > > struct of_bus *pbus, __be32 *addr, > > > > > int na, int ns, int pna, const char *rprop) > > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct > > > > device_node *dev, > > > > > result = of_read_number(addr, na); > > > > > break; > > > > > } > > > > > + /* > > > > > + * For indirectIO device which has no ranges > > property, get > > > > > + * the address from reg directly. > > > > > + */ > > > > > + if (of_get_isa_indirect_io(dev, bus, addr, na, > > &result)) { > > > > > + pr_debug("isa indirectIO matched(%s)..addr = > > > > 0x%llx\n", > > > > > + of_node_full_name(dev), result); > > > > > + break; > > > > > + } > > > > > > > > > > /* Get new parent bus and counts */ > > > > > pbus = of_match_bus(parent); > > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct > > > > device_node *dev, > > > > > if (taddr == OF_BAD_ADDR) > > > > > return -EINVAL; > > > > > memset(r, 0, sizeof(struct resource)); > > > > > - if (flags & IORESOURCE_IO) { > > > > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) { > > > > > unsigned long port; > > > > > + > > > > > port = pci_address_to_pio(taddr); > > > > > if (port == (unsigned long)-1) > > > > > return -EINVAL; > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index ba34907..1a08511 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -3263,7 +3263,7 @@ int __weak > > pci_register_io_range(phys_addr_t > > > > addr, resource_size_t size) > > > > > > > > > > #ifdef PCI_IOBASE > > > > > struct io_range *range; > > > > > - resource_size_t allocated_size = 0; > > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO; > > > > > > > > > > /* check if the range hasn't been previously recorded */ > > > > > spin_lock(&io_range_lock); > > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned > > long > > > > pio) > > > > > > > > > > #ifdef PCI_IOBASE > > > > > struct io_range *range; > > > > > - resource_size_t allocated_size = 0; > > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO; > > > > > > > > Have you checked that pci_pio_to_address still returns valid values > > > > after this? I know that > > > > you are trying to take into account PCIBIOS_MIN_IO limit when > > > > allocating reserving the IO ranges, > > > > but the values added in the io_range_list are still starting from > > zero, > > > > no from PCIBIOS_MIN_IO, > > > > > > I think you're wrong here as in pci_address_to_pio we have: > > > + resource_size_t offset = PCIBIOS_MIN_IO; > > > > > > This should be enough to guarantee that the PIOs start at > > > PCIBIOS_MIN_IO...right? > > > > I don't think you can guarantee that the pio value that gets passed > > into > > pci_pio_to_address() always comes from a previously returned value by > > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address() > > Maybe I am missing something...could you make an exampleof a case > where an IO toke doesn’t come from pci_address_to_pio() ? Don't know, maybe it is coming from some DT or platform data? I was just asking for confirmation that no one has seen issues there, I'm not saying I've seen it happening. Best regards, Liviu > > Thanks > > Gab > > > > > > if (pio < PCIBIOS_MIN_IO) > > return address; > > > > to avoid adding more checks in the list_for_each_entry() loop. > > > > Best regards, > > Liviu > > > > > > > > > > > > so the calculation of the address in this function could return > > > > negative values casted to pci_addr_t. > > > > > > > > Maybe you want to adjust the range->start value in > > > > pci_register_io_range() as well to have it > > > > offset by PCIBIOS_MIN_IO as well. > > > > > > > > Best regards, > > > > Liviu > > > > > > > > > > > > > > if (pio > IO_SPACE_LIMIT) > > > > > return address; > > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak > > > > pci_address_to_pio(phys_addr_t address) > > > > > { > > > > > #ifdef PCI_IOBASE > > > > > struct io_range *res; > > > > > - resource_size_t offset = 0; > > > > > + resource_size_t offset = PCIBIOS_MIN_IO; > > > > > unsigned long addr = -1; > > > > > > > > > > spin_lock(&io_range_lock); > > > > > diff --git a/include/linux/of_address.h > > b/include/linux/of_address.h > > > > > index 3786473..deec469 100644 > > > > > --- a/include/linux/of_address.h > > > > > +++ b/include/linux/of_address.h > > > > > @@ -24,6 +24,23 @@ struct of_pci_range { > > > > > #define for_each_of_pci_range(parser, range) \ > > > > > for (; of_pci_range_parser_one(parser, range);) > > > > > > > > > > + > > > > > +#ifndef indirect_io_enabled > > > > > +#define indirect_io_enabled indirect_io_enabled > > > > > +static inline bool indirect_io_enabled(void) > > > > > +{ > > > > > + return false; > > > > > +} > > > > > +#endif > > > > > + > > > > > +#ifndef addr_is_indirect_io > > > > > +#define addr_is_indirect_io addr_is_indirect_io > > > > > +static inline int addr_is_indirect_io(u64 taddr) > > > > > +{ > > > > > + return 0; > > > > > +} > > > > > +#endif > > > > > + > > > > > /* Translate a DMA address from device space to CPU space */ > > > > > extern u64 of_translate_dma_address(struct device_node *dev, > > > > > const __be32 *in_addr); > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > > > > index 0e49f70..7f6bbb6 100644 > > > > > --- a/include/linux/pci.h > > > > > +++ b/include/linux/pci.h > > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct > > > > pci_bus *bus) > > > > > /* provide the legacy pci_dma_* API */ > > > > > #include <linux/pci-dma-compat.h> > > > > > > > > > > +/* > > > > > + * define this macro here to refrain from compilation error for > > some > > > > > + * platforms. Please keep this macro at the end of this header > > file. > > > > > + */ > > > > > +#ifndef PCIBIOS_MIN_IO > > > > > +#define PCIBIOS_MIN_IO 0 > > > > > +#endif > > > > > + > > > > > #endif /* LINUX_PCI_H */ > > > > > -- > > > > > 1.9.1 > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe linux- > > pci" > > > > in > > > > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > > > > More majordomo info at http://vger.kernel.org/majordomo- > > info.html > > > > -- > > ==================== > > | I would like to | > > | fix the world, | > > | but they're not | > > | giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯ -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
