| Message ID | 20140922093228.GA20256@rric.localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 22, 2014 at 10:32:28AM +0100, Robert Richter wrote: > On 18.09.14 02:30:23, Liviu Dudau wrote: > > +int of_pci_get_host_bridge_resources(struct device_node *dev, > > + unsigned char busno, unsigned char bus_max, > > + struct list_head *resources, resource_size_t *io_base) > > +{ > > + struct resource *res; > > + struct resource *bus_range; > > + struct of_pci_range range; > > + struct of_pci_range_parser parser; > > + char range_type[4]; > > + int err; > > + > > + if (!io_base) > > + return -EINVAL; > > + *io_base = OF_BAD_ADDR; > Hi Robert, > This breaks for mem-mapped pci host controllers. The patch below fixes > this. I think you mean PCI host controller that have only memory mapped ranges, am I right? Initially I've read your reply as to mean that the host controller is accessed through some memory mapped area, which I believe is the case for all host controllers. > > This series was tested with the fix on top for Cavium Thunder. > > Tested-by: Robert Richter <rrichter@cavium.com> Thanks for that! > > -Robert > > > > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > From: Robert Richter <rrichter@cavium.com> > Date: Mon, 22 Sep 2014 10:46:01 +0200 > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges > > The pci host bridge was not created if io_base was not set when > calling of_pci_get_host_bridge_resources(). This is esp. the case for > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function > parameter io_base is optional now. I think the message is misleading. What you want to do is make io_base optional for the case where the PCI host bridge only expects to have only IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. As I'm going to touch this area again to address a comment from Bjorn, do you mind if I roll this patch into mine with your Signed-off-by and the mention that you have made io_base optional? Best regards, Liviu > > Signed-off-by: Robert Richter <rrichter@cavium.com> > --- > drivers/of/of_pci.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index ffdb45ed8682..1f0e7c2505ee 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -182,9 +182,8 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > char range_type[4]; > int err; > > - if (!io_base) > - return -EINVAL; > - *io_base = OF_BAD_ADDR; > + if (io_base) > + *io_base = OF_BAD_ADDR; > > bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > if (!bus_range) > @@ -242,7 +241,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > goto parse_failed; > } > > - if (resource_type(res) == IORESOURCE_IO) { > + if (io_base && resource_type(res) == IORESOURCE_IO) { > if (*io_base != OF_BAD_ADDR) > pr_warn("More than one I/O resource converted. CPU offset for old range lost!\n"); > *io_base = range.cpu_addr; > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 22.09.14 12:43:17, Liviu Dudau wrote: > On Mon, Sep 22, 2014 at 10:32:28AM +0100, Robert Richter wrote: > > On 18.09.14 02:30:23, Liviu Dudau wrote: > > > +int of_pci_get_host_bridge_resources(struct device_node *dev, > > > + unsigned char busno, unsigned char bus_max, > > > + struct list_head *resources, resource_size_t *io_base) > > > +{ > > > + struct resource *res; > > > + struct resource *bus_range; > > > + struct of_pci_range range; > > > + struct of_pci_range_parser parser; > > > + char range_type[4]; > > > + int err; > > > + > > > + if (!io_base) > > > + return -EINVAL; > > > + *io_base = OF_BAD_ADDR; > > > > Hi Robert, > > > This breaks for mem-mapped pci host controllers. The patch below fixes > > this. > > I think you mean PCI host controller that have only memory mapped ranges, > am I right? Initially I've read your reply as to mean that the host > controller is accessed through some memory mapped area, which I believe is > the case for all host controllers. Right, that's meant here. Sorry for the misleading comment. > > > > > This series was tested with the fix on top for Cavium Thunder. > > > > Tested-by: Robert Richter <rrichter@cavium.com> > > Thanks for that! > > > > > -Robert > > > > > > > > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > > From: Robert Richter <rrichter@cavium.com> > > Date: Mon, 22 Sep 2014 10:46:01 +0200 > > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges > > > > The pci host bridge was not created if io_base was not set when > > calling of_pci_get_host_bridge_resources(). This is esp. the case for > > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function > > parameter io_base is optional now. > > I think the message is misleading. What you want to do is make io_base > optional for the case where the PCI host bridge only expects to have only > IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. > > As I'm going to touch this area again to address a comment from Bjorn, > do you mind if I roll this patch into mine with your Signed-off-by and > the mention that you have made io_base optional? Sure, fine with me. Thanks, -Robert > > Best regards, > Liviu > > > > > Signed-off-by: Robert Richter <rrichter@cavium.com> > > --- > > drivers/of/of_pci.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > > index ffdb45ed8682..1f0e7c2505ee 100644 > > --- a/drivers/of/of_pci.c > > +++ b/drivers/of/of_pci.c > > @@ -182,9 +182,8 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > char range_type[4]; > > int err; > > > > - if (!io_base) > > - return -EINVAL; > > - *io_base = OF_BAD_ADDR; > > + if (io_base) > > + *io_base = OF_BAD_ADDR; > > > > bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > > if (!bus_range) > > @@ -242,7 +241,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > goto parse_failed; > > } > > > > - if (resource_type(res) == IORESOURCE_IO) { > > + if (io_base && resource_type(res) == IORESOURCE_IO) { > > if (*io_base != OF_BAD_ADDR) > > pr_warn("More than one I/O resource converted. CPU offset for old range lost!\n"); > > *io_base = range.cpu_addr; > > -- > > 2.1.0 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@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! / > --------------- > ¯\_(?)_/¯ >
On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: > > > > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > > From: Robert Richter <rrichter@cavium.com> > > Date: Mon, 22 Sep 2014 10:46:01 +0200 > > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges > > > > The pci host bridge was not created if io_base was not set when > > calling of_pci_get_host_bridge_resources(). This is esp. the case for > > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function > > parameter io_base is optional now. > > I think the message is misleading. What you want to do is make io_base > optional for the case where the PCI host bridge only expects to have only > IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. > > As I'm going to touch this area again to address a comment from Bjorn, > do you mind if I roll this patch into mine with your Signed-off-by and > the mention that you have made io_base optional? I think the best way to deal with this is to move the check for io_base down into the place where it is used: As long as the DT only specifies IORESOURCE_MEM windows, we don't need to look at io_base, but if the host controller driver does not support IORESOURCE_IO while the DT specifies it, I guess it would be nice to return an error. Arnd
On Tue, Sep 23, 2014 at 08:56:37AM +0100, Arnd Bergmann wrote: > On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: > > > > > > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > > > From: Robert Richter <rrichter@cavium.com> > > > Date: Mon, 22 Sep 2014 10:46:01 +0200 > > > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges > > > > > > The pci host bridge was not created if io_base was not set when > > > calling of_pci_get_host_bridge_resources(). This is esp. the case for > > > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function > > > parameter io_base is optional now. > > > > I think the message is misleading. What you want to do is make io_base > > optional for the case where the PCI host bridge only expects to have only > > IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. > > > > As I'm going to touch this area again to address a comment from Bjorn, > > do you mind if I roll this patch into mine with your Signed-off-by and > > the mention that you have made io_base optional? > > I think the best way to deal with this is to move the check for > io_base down into the place where it is used: As long as the DT only > specifies IORESOURCE_MEM windows, we don't need to look at io_base, > but if the host controller driver does not support IORESOURCE_IO > while the DT specifies it, I guess it would be nice to return an > error. Because the detection of IORESOURCE_{IO|MEM} happens in a loop I still need to initialise the io_base to some invalid value before going into the for_each_of_pci_range() {...} body. I have added a check in v12 for the lack of valid io_base pointer if IORESOURCE_IO range is found. Best regards, Liviu > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
On Tue, Sep 23, 2014 at 2:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: >> > >> > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 >> > From: Robert Richter <rrichter@cavium.com> >> > Date: Mon, 22 Sep 2014 10:46:01 +0200 >> > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges >> > >> > The pci host bridge was not created if io_base was not set when >> > calling of_pci_get_host_bridge_resources(). This is esp. the case for >> > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function >> > parameter io_base is optional now. >> >> I think the message is misleading. What you want to do is make io_base >> optional for the case where the PCI host bridge only expects to have only >> IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. >> >> As I'm going to touch this area again to address a comment from Bjorn, >> do you mind if I roll this patch into mine with your Signed-off-by and >> the mention that you have made io_base optional? > > I think the best way to deal with this is to move the check for > io_base down into the place where it is used: As long as the DT only > specifies IORESOURCE_MEM windows, we don't need to look at io_base, > but if the host controller driver does not support IORESOURCE_IO > while the DT specifies it, I guess it would be nice to return an > error. The DT may specify it, but the h/w could be broken in some way so the host driver chooses to ignore it. I don't think we should force the host driver to provide a pointer in that case. Also, would we want it added to the resource list if it is not going to be used? Rob
On Tue, Sep 23, 2014 at 02:30:08PM +0100, Rob Herring wrote: > On Tue, Sep 23, 2014 at 2:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: > >> > > >> > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > >> > From: Robert Richter <rrichter@cavium.com> > >> > Date: Mon, 22 Sep 2014 10:46:01 +0200 > >> > Subject: [PATCH] OF: PCI: Fix creation of mem-mapped pci host bridges > >> > > >> > The pci host bridge was not created if io_base was not set when > >> > calling of_pci_get_host_bridge_resources(). This is esp. the case for > >> > mem-mapped io (IORESOURCE_MEM). This patch fixes this. Function > >> > parameter io_base is optional now. > >> > >> I think the message is misleading. What you want to do is make io_base > >> optional for the case where the PCI host bridge only expects to have only > >> IORESOURCE_MEM ranges and doesn't care about IORESOURCE_IO ones. > >> > >> As I'm going to touch this area again to address a comment from Bjorn, > >> do you mind if I roll this patch into mine with your Signed-off-by and > >> the mention that you have made io_base optional? > > > > I think the best way to deal with this is to move the check for > > io_base down into the place where it is used: As long as the DT only > > specifies IORESOURCE_MEM windows, we don't need to look at io_base, > > but if the host controller driver does not support IORESOURCE_IO > > while the DT specifies it, I guess it would be nice to return an > > error. > > The DT may specify it, but the h/w could be broken in some way so the > host driver chooses to ignore it. I don't think we should force the > host driver to provide a pointer in that case. Also, would we want it > added to the resource list if it is not going to be used? This is only for scanning the DT ranges. If the hardware is broken and the driver knows about that then it can filter out the I/O ranges before it passes them on to pci_scan_root_bus() or pci_create_root_bus(). Lets not complicate this function any further. If you get an I/O range it should be expected to also pass an io_base pointer to store the CPU address of the range, otherwise it will get lost in the conversion to an IORESOURCE_IO resource. You want later to discard the resource and forget the CPU address, then don't use it! Best regards, Liviu > > Rob >
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index ffdb45ed8682..1f0e7c2505ee 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -182,9 +182,8 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, char range_type[4]; int err; - if (!io_base) - return -EINVAL; - *io_base = OF_BAD_ADDR; + if (io_base) + *io_base = OF_BAD_ADDR; bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); if (!bus_range) @@ -242,7 +241,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, goto parse_failed; } - if (resource_type(res) == IORESOURCE_IO) { + if (io_base && resource_type(res) == IORESOURCE_IO) { if (*io_base != OF_BAD_ADDR) pr_warn("More than one I/O resource converted. CPU offset for old range lost!\n"); *io_base = range.cpu_addr;