On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:

Hi Liviu

quoted

-----Original Message-----
From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
Sent: 09 November 2016 16:51
To: Gabriele Paoloni
Cc: Yuanzhichang; catalin.marinas at arm.com; will.deacon at arm.com;
robh+dt at kernel.org; bhelgaas at google.com; mark.rutland at arm.com;
olof at lixom.net; arnd at arndb.de; linux-arm-kernel at lists.infradead.org;
lorenzo.pieralisi at arm.com; linux-kernel at vger.kernel.org; Linuxarm;
devicetree at vger.kernel.org; linux-pci at vger.kernel.org; linux-
serial at vger.kernel.org; minyard at acm.org; benh at kernel.crashing.org;
zourongrong at gmail.com; John Garry; zhichang.yuan02 at gmail.com;
kantyzc at 163.com; 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:

quoted

Hi Liviu

Thanks for reviewing

[removed some irrelevant part of discussion, avoid crazy formatting]

quoted

quoted

quoted

+/**
+ * addr_is_indirect_io - check whether the input taddr is for

indirectIO.

quoted

+ * @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

<

quoted

quoted

taddr)

start >= taddr ?

Nope... if  (taddr < arm64_extio_ops->start || taddr >

arm64_extio_ops->end)

quoted

then taddr is outside the range [start; end] and will return 0;

otherwise

quoted

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.

quoted

quoted

quoted

+		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)

quoted

 	return false;
 }

+
+/*
+ * of_isa_indirect_io - get the IO address from some isa reg

property value.

quoted

+ *	For some isa/lpc devices, no ranges property in ancestor

node.

quoted

quoted

quoted

+ *	The device addresses are described directly in their regs

property.

quoted

+ *	This fixup function will be called to get the IO address of

isa/lpc

quoted

+ *	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;

quoted

+ * @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

quoted

quoted

+ */
+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

quoted

quoted

+	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,

quoted

 			    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,

quoted

 			result = of_read_number(addr, na);
 			break;
 		}
+		/*
+		 * For indirectIO device which has no ranges

property, get

quoted

quoted

quoted

+		 * the address from reg directly.
+		 */
+		if (of_get_isa_indirect_io(dev, bus, addr, na,

&result)) {

quoted

quoted

quoted

+			pr_debug("isa indirectIO matched(%s)..addr =

0x%llx\n",

quoted

+				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,

quoted

 	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

quoted

quoted

addr, resource_size_t size)

quoted

 #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

quoted

quoted

pio)

quoted

 #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,

quoted

quoted

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

quoted

	if (pio < PCIBIOS_MIN_IO)
		return address;

to avoid adding more checks in the list_for_each_entry() loop.

Best regards,
Liviu

quoted

quoted

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

quoted

 	if (pio > IO_SPACE_LIMIT)
 		return address;

@@ -3335,7 +3335,7 @@ unsigned long __weak

pci_address_to_pio(phys_addr_t address)

quoted

 {
 #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

quoted

quoted

quoted

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)

quoted

 /* provide the legacy pci_dma_* API */
 #include <linux/pci-dma-compat.h>

+/*
+ * define this macro here to refrain from compilation error for

some

quoted

quoted

quoted

+ * platforms. Please keep this macro at the end of this header

file.

quoted

quoted

quoted

+ */
+#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"

quoted

quoted

in

quoted

the body of a message to majordomo at vger.kernel.org
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!  /
  ---------------
    ?\_(?)_/?