Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Tue, Sep 23, 2014 at 02:30:08PM +0100, Rob Herring wrote: > On Tue, Sep 23, 2014 at 2:56 AM, 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 > >> > 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 > -- | 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Tue, Sep 23, 2014 at 2:56 AM, 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 >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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
> > > 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 majord...@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! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: > > > > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 > > From: Robert Richter > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Monday 22 September 2014 12:43:17 Liviu Dudau wrote: From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001 From: Robert Richter rrich...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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 rrich...@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 majord...@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! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Tue, Sep 23, 2014 at 2:56 AM, Arnd Bergmann a...@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 rrich...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Tue, Sep 23, 2014 at 02:30:08PM +0100, Rob Herring wrote: On Tue, Sep 23, 2014 at 2:56 AM, Arnd Bergmann a...@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 rrich...@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 -- | 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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Mon, Sep 22, 2014 at 12:55 PM, Liviu Dudau wrote:
> On Sat, Sep 20, 2014 at 01:28:38AM +0100, Rob Herring wrote:
>> On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
>> > On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
>> >> Provide a function to parse the PCI DT ranges that can be used to
>> >> create a pci_host_bridge structure together with its associated
>> >> bus.
>> >>
>> >> Cc: Bjorn Helgaas
>> >> Cc: Arnd Bergmann
>> >> Cc: Grant Likely
>> >> Cc: Rob Herring
>> >> Cc: Catalin Marinas
>> >> Signed-off-by: Liviu Dudau
>> >> ---
>>
>> [...]
>>
>> >> +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;
>> >> +
>> >> + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>>
>> This function does a lot of kalloc's but there is not an easy way to
>> undo those allocations. Hot unplug of a host bridge or probe error
>> handling would leak memory.
>>
>> You could pass in struct device and use the devm_ variant (also
>> addressing Bjorn's comment), but not having an uninit/remove function
>> make what clean-up drivers have to do error prone. For example, on
>> uninit a driver needs to call pci_free_resource_list.
>
> If the function fails to parse the ranges for whatever reason it will
> call pci_free_resource_list on the resources that have been added already
> and it will clean up. If it is successful, then it is the job of the caller
> to free the list, as mentioned in the comment associated with the function.
>
> The reason why I am reluctant to use devm_ here is that the ranges are only
> parsed here, no filtering is applied. Architectures and/or host bridge
> drivers can/could impose additional restrictions to the list before passing
> it to pci_scan_root_bus() for example.
Okay, as long as the caller has a single clean-up responsibility, I
guess it is fine.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Sat, Sep 20, 2014 at 01:28:38AM +0100, Rob Herring wrote:
> On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
> >> Provide a function to parse the PCI DT ranges that can be used to
> >> create a pci_host_bridge structure together with its associated
> >> bus.
> >>
> >> Cc: Bjorn Helgaas
> >> Cc: Arnd Bergmann
> >> Cc: Grant Likely
> >> Cc: Rob Herring
> >> Cc: Catalin Marinas
> >> Signed-off-by: Liviu Dudau
> >> ---
>
> [...]
>
> >> +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;
> >> +
> >> + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>
> This function does a lot of kalloc's but there is not an easy way to
> undo those allocations. Hot unplug of a host bridge or probe error
> handling would leak memory.
>
> You could pass in struct device and use the devm_ variant (also
> addressing Bjorn's comment), but not having an uninit/remove function
> make what clean-up drivers have to do error prone. For example, on
> uninit a driver needs to call pci_free_resource_list.
If the function fails to parse the ranges for whatever reason it will
call pci_free_resource_list on the resources that have been added already
and it will clean up. If it is successful, then it is the job of the caller
to free the list, as mentioned in the comment associated with the function.
The reason why I am reluctant to use devm_ here is that the ranges are only
parsed here, no filtering is applied. Architectures and/or host bridge
drivers can/could impose additional restrictions to the list before passing
it to pci_scan_root_bus() for example.
Best regards,
Liviu
>
> Rob
>
--
| 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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
>
> Thanks for that!
>
> >
> > -Robert
> >
> >
> >
> > From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
> > From: Robert Richter
> > 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
> > ---
> > 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 majord...@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! /
> ---
> ¯\_(ツ)_/¯
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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
Thanks for that!
>
> -Robert
>
>
>
> From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
> From: Robert Richter
> 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
> ---
> 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 majord...@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! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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;
This breaks for mem-mapped pci host controllers. The patch below fixes
this.
This series was tested with the fix on top for Cavium Thunder.
Tested-by: Robert Richter
-Robert
>From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
From: Robert Richter
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.
Signed-off-by: Robert Richter
---
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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;
This breaks for mem-mapped pci host controllers. The patch below fixes
this.
This series was tested with the fix on top for Cavium Thunder.
Tested-by: Robert Richter rrich...@cavium.com
-Robert
From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
From: Robert Richter rrich...@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.
Signed-off-by: Robert Richter rrich...@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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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 rrich...@cavium.com
Thanks for that!
-Robert
From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
From: Robert Richter rrich...@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 rrich...@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 majord...@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! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
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 rrich...@cavium.com
Thanks for that!
-Robert
From e798af4fc2f664d1aff7e863489b8298f90e716e Mon Sep 17 00:00:00 2001
From: Robert Richter rrich...@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 rrich...@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 majord...@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! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Sat, Sep 20, 2014 at 01:28:38AM +0100, Rob Herring wrote:
On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
---
[...]
+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;
+
+ bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
This function does a lot of kalloc's but there is not an easy way to
undo those allocations. Hot unplug of a host bridge or probe error
handling would leak memory.
You could pass in struct device and use the devm_ variant (also
addressing Bjorn's comment), but not having an uninit/remove function
make what clean-up drivers have to do error prone. For example, on
uninit a driver needs to call pci_free_resource_list.
If the function fails to parse the ranges for whatever reason it will
call pci_free_resource_list on the resources that have been added already
and it will clean up. If it is successful, then it is the job of the caller
to free the list, as mentioned in the comment associated with the function.
The reason why I am reluctant to use devm_ here is that the ranges are only
parsed here, no filtering is applied. Architectures and/or host bridge
drivers can/could impose additional restrictions to the list before passing
it to pci_scan_root_bus() for example.
Best regards,
Liviu
Rob
--
| 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Mon, Sep 22, 2014 at 12:55 PM, Liviu Dudau liviu.du...@arm.com wrote:
On Sat, Sep 20, 2014 at 01:28:38AM +0100, Rob Herring wrote:
On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
---
[...]
+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;
+
+ bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
This function does a lot of kalloc's but there is not an easy way to
undo those allocations. Hot unplug of a host bridge or probe error
handling would leak memory.
You could pass in struct device and use the devm_ variant (also
addressing Bjorn's comment), but not having an uninit/remove function
make what clean-up drivers have to do error prone. For example, on
uninit a driver needs to call pci_free_resource_list.
If the function fails to parse the ranges for whatever reason it will
call pci_free_resource_list on the resources that have been added already
and it will clean up. If it is successful, then it is the job of the caller
to free the list, as mentioned in the comment associated with the function.
The reason why I am reluctant to use devm_ here is that the ranges are only
parsed here, no filtering is applied. Architectures and/or host bridge
drivers can/could impose additional restrictions to the list before passing
it to pci_scan_root_bus() for example.
Okay, as long as the caller has a single clean-up responsibility, I
guess it is fine.
Rob
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
> On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
>> Provide a function to parse the PCI DT ranges that can be used to
>> create a pci_host_bridge structure together with its associated
>> bus.
>>
>> Cc: Bjorn Helgaas
>> Cc: Arnd Bergmann
>> Cc: Grant Likely
>> Cc: Rob Herring
>> Cc: Catalin Marinas
>> Signed-off-by: Liviu Dudau
>> ---
[...]
>> +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;
>> +
>> +bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
This function does a lot of kalloc's but there is not an easy way to
undo those allocations. Hot unplug of a host bridge or probe error
handling would leak memory.
You could pass in struct device and use the devm_ variant (also
addressing Bjorn's comment), but not having an uninit/remove function
make what clean-up drivers have to do error prone. For example, on
uninit a driver needs to call pci_free_resource_list.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
> Provide a function to parse the PCI DT ranges that can be used to
> create a pci_host_bridge structure together with its associated
> bus.
>
> Cc: Bjorn Helgaas
> Cc: Arnd Bergmann
> Cc: Grant Likely
> Cc: Rob Herring
> Cc: Catalin Marinas
> Signed-off-by: Liviu Dudau
> ---
> drivers/of/of_pci.c| 108
> +
> include/linux/of_pci.h | 11 +
> 2 files changed, 119 insertions(+)
>
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 7eaeac2..c18b8da 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -1,7 +1,9 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
>
> #include "of_private.h"
>
> @@ -151,6 +153,112 @@ int of_pci_get_domain_nr(struct device_node *node, bool
> allocate_if_missing)
> }
> EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
>
> +/**
> + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
> + * @dev: device node of the host bridge having the range property
> + * @busno: bus number associated with the bridge root bus
> + * @bus_max: maximum number of busses for this bridge
> + * @resources: list where the range of resources will be added after DT
> parsing
> + * @io_base: pointer to a variable that will contain on return the physical
> + * address for the start of the I/O range.
> + *
> + * It is the callers job to free the @resources list.
> + *
> + * This function will parse the "ranges" property of a PCI host bridge device
> + * node and setup the resource mapping based on its content. It is expected
> + * that the property conforms with the Power ePAPR document.
> + *
> + * It returns zero if the range parsing has been successful or a standard
> error
> + * value if it failed.
> + */
> +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;
> +
> + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> + if (!bus_range)
> + return -ENOMEM;
> +
> + pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> +
> + err = of_pci_parse_bus_range(dev, bus_range);
> + if (err) {
> + bus_range->start = busno;
> + bus_range->end = bus_max;
> + bus_range->flags = IORESOURCE_BUS;
> + pr_info(" No bus range found for %s, using %pR\n",
> + dev->full_name, _range);
> + } else {
> + if (bus_range->end > bus_range->start + bus_max)
> + bus_range->end = bus_range->start + bus_max;
> + }
> + pci_add_resource(resources, bus_range);
> +
> + /* Check for ranges property */
> + err = of_pci_range_parser_init(, dev);
> + if (err)
> + goto parse_failed;
> +
> + pr_debug("Parsing ranges property...\n");
> + for_each_of_pci_range(, ) {
> + /* Read next ranges element */
> + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
> + snprintf(range_type, 4, " IO");
> + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
> + snprintf(range_type, 4, "MEM");
> + else
> + snprintf(range_type, 4, "err");
> + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type,
> + range.cpu_addr, range.cpu_addr + range.size - 1,
> + range.pci_addr);
> +
> + /*
> + * If we failed translation or got a zero-sized region
> + * then skip this range
> + */
> + if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> + continue;
> +
> + res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> + if (!res) {
> + err = -ENOMEM;
> + goto parse_failed;
> + }
> +
> + err = of_pci_range_to_resource(, dev, res);
> + if (err) {
> + kfree(res);
> + goto parse_failed;
> + }
> +
> + if (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");
It'd be nice if the output included a hint about which device this warning
applies to.
> + *io_base = range.cpu_addr;
> + }
> +
> + pci_add_resource_offset(resources, res, res->start -
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
---
drivers/of/of_pci.c| 108
+
include/linux/of_pci.h | 11 +
2 files changed, 119 insertions(+)
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 7eaeac2..c18b8da 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -1,7 +1,9 @@
#include linux/kernel.h
#include linux/export.h
#include linux/of.h
+#include linux/of_address.h
#include linux/of_pci.h
+#include linux/slab.h
#include of_private.h
@@ -151,6 +153,112 @@ int of_pci_get_domain_nr(struct device_node *node, bool
allocate_if_missing)
}
EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
+/**
+ * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @busno: bus number associated with the bridge root bus
+ * @bus_max: maximum number of busses for this bridge
+ * @resources: list where the range of resources will be added after DT
parsing
+ * @io_base: pointer to a variable that will contain on return the physical
+ * address for the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list.
+ *
+ * This function will parse the ranges property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.
+ *
+ * It returns zero if the range parsing has been successful or a standard
error
+ * value if it failed.
+ */
+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;
+
+ bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+ if (!bus_range)
+ return -ENOMEM;
+
+ pr_info(PCI host bridge %s ranges:\n, dev-full_name);
+
+ err = of_pci_parse_bus_range(dev, bus_range);
+ if (err) {
+ bus_range-start = busno;
+ bus_range-end = bus_max;
+ bus_range-flags = IORESOURCE_BUS;
+ pr_info( No bus range found for %s, using %pR\n,
+ dev-full_name, bus_range);
+ } else {
+ if (bus_range-end bus_range-start + bus_max)
+ bus_range-end = bus_range-start + bus_max;
+ }
+ pci_add_resource(resources, bus_range);
+
+ /* Check for ranges property */
+ err = of_pci_range_parser_init(parser, dev);
+ if (err)
+ goto parse_failed;
+
+ pr_debug(Parsing ranges property...\n);
+ for_each_of_pci_range(parser, range) {
+ /* Read next ranges element */
+ if ((range.flags IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
+ snprintf(range_type, 4, IO);
+ else if ((range.flags IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
+ snprintf(range_type, 4, MEM);
+ else
+ snprintf(range_type, 4, err);
+ pr_info( %s %#010llx..%#010llx - %#010llx\n, range_type,
+ range.cpu_addr, range.cpu_addr + range.size - 1,
+ range.pci_addr);
+
+ /*
+ * If we failed translation or got a zero-sized region
+ * then skip this range
+ */
+ if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+ continue;
+
+ res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+ if (!res) {
+ err = -ENOMEM;
+ goto parse_failed;
+ }
+
+ err = of_pci_range_to_resource(range, dev, res);
+ if (err) {
+ kfree(res);
+ goto parse_failed;
+ }
+
+ if (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);
It'd be nice if the output included a hint about which device this warning
applies to.
+ *io_base = range.cpu_addr;
+ }
+
+
Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
---
[...]
+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;
+
+bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
This function does a lot of kalloc's but there is not an easy way to
undo those allocations. Hot unplug of a host bridge or probe error
handling would leak memory.
You could pass in struct device and use the devm_ variant (also
addressing Bjorn's comment), but not having an uninit/remove function
make what clean-up drivers have to do error prone. For example, on
uninit a driver needs to call pci_free_resource_list.
Rob
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas
Cc: Arnd Bergmann
Cc: Grant Likely
Cc: Rob Herring
Cc: Catalin Marinas
Signed-off-by: Liviu Dudau
---
drivers/of/of_pci.c| 108 +
include/linux/of_pci.h | 11 +
2 files changed, 119 insertions(+)
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 7eaeac2..c18b8da 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -1,7 +1,9 @@
#include
#include
#include
+#include
#include
+#include
#include "of_private.h"
@@ -151,6 +153,112 @@ int of_pci_get_domain_nr(struct device_node *node, bool
allocate_if_missing)
}
EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
+/**
+ * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @busno: bus number associated with the bridge root bus
+ * @bus_max: maximum number of busses for this bridge
+ * @resources: list where the range of resources will be added after DT parsing
+ * @io_base: pointer to a variable that will contain on return the physical
+ * address for the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list.
+ *
+ * This function will parse the "ranges" property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.
+ *
+ * It returns zero if the range parsing has been successful or a standard error
+ * value if it failed.
+ */
+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;
+
+ bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+ if (!bus_range)
+ return -ENOMEM;
+
+ pr_info("PCI host bridge %s ranges:\n", dev->full_name);
+
+ err = of_pci_parse_bus_range(dev, bus_range);
+ if (err) {
+ bus_range->start = busno;
+ bus_range->end = bus_max;
+ bus_range->flags = IORESOURCE_BUS;
+ pr_info(" No bus range found for %s, using %pR\n",
+ dev->full_name, _range);
+ } else {
+ if (bus_range->end > bus_range->start + bus_max)
+ bus_range->end = bus_range->start + bus_max;
+ }
+ pci_add_resource(resources, bus_range);
+
+ /* Check for ranges property */
+ err = of_pci_range_parser_init(, dev);
+ if (err)
+ goto parse_failed;
+
+ pr_debug("Parsing ranges property...\n");
+ for_each_of_pci_range(, ) {
+ /* Read next ranges element */
+ if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
+ snprintf(range_type, 4, " IO");
+ else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
+ snprintf(range_type, 4, "MEM");
+ else
+ snprintf(range_type, 4, "err");
+ pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type,
+ range.cpu_addr, range.cpu_addr + range.size - 1,
+ range.pci_addr);
+
+ /*
+* If we failed translation or got a zero-sized region
+* then skip this range
+*/
+ if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+ continue;
+
+ res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+ if (!res) {
+ err = -ENOMEM;
+ goto parse_failed;
+ }
+
+ err = of_pci_range_to_resource(, dev, res);
+ if (err) {
+ kfree(res);
+ goto parse_failed;
+ }
+
+ if (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;
+ }
+
+ pci_add_resource_offset(resources, res, res->start -
range.pci_addr);
+ }
+
+ return 0;
+
+parse_failed:
+ pci_free_resource_list(resources);
+ return err;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
+
#ifdef CONFIG_PCI_MSI
static LIST_HEAD(of_pci_msi_chip_list);
diff --git a/include/linux/of_pci.h
[PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT
Provide a function to parse the PCI DT ranges that can be used to
create a pci_host_bridge structure together with its associated
bus.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Catalin Marinas catalin.mari...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
---
drivers/of/of_pci.c| 108 +
include/linux/of_pci.h | 11 +
2 files changed, 119 insertions(+)
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 7eaeac2..c18b8da 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -1,7 +1,9 @@
#include linux/kernel.h
#include linux/export.h
#include linux/of.h
+#include linux/of_address.h
#include linux/of_pci.h
+#include linux/slab.h
#include of_private.h
@@ -151,6 +153,112 @@ int of_pci_get_domain_nr(struct device_node *node, bool
allocate_if_missing)
}
EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
+/**
+ * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @busno: bus number associated with the bridge root bus
+ * @bus_max: maximum number of busses for this bridge
+ * @resources: list where the range of resources will be added after DT parsing
+ * @io_base: pointer to a variable that will contain on return the physical
+ * address for the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list.
+ *
+ * This function will parse the ranges property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.
+ *
+ * It returns zero if the range parsing has been successful or a standard error
+ * value if it failed.
+ */
+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;
+
+ bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+ if (!bus_range)
+ return -ENOMEM;
+
+ pr_info(PCI host bridge %s ranges:\n, dev-full_name);
+
+ err = of_pci_parse_bus_range(dev, bus_range);
+ if (err) {
+ bus_range-start = busno;
+ bus_range-end = bus_max;
+ bus_range-flags = IORESOURCE_BUS;
+ pr_info( No bus range found for %s, using %pR\n,
+ dev-full_name, bus_range);
+ } else {
+ if (bus_range-end bus_range-start + bus_max)
+ bus_range-end = bus_range-start + bus_max;
+ }
+ pci_add_resource(resources, bus_range);
+
+ /* Check for ranges property */
+ err = of_pci_range_parser_init(parser, dev);
+ if (err)
+ goto parse_failed;
+
+ pr_debug(Parsing ranges property...\n);
+ for_each_of_pci_range(parser, range) {
+ /* Read next ranges element */
+ if ((range.flags IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
+ snprintf(range_type, 4, IO);
+ else if ((range.flags IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
+ snprintf(range_type, 4, MEM);
+ else
+ snprintf(range_type, 4, err);
+ pr_info( %s %#010llx..%#010llx - %#010llx\n, range_type,
+ range.cpu_addr, range.cpu_addr + range.size - 1,
+ range.pci_addr);
+
+ /*
+* If we failed translation or got a zero-sized region
+* then skip this range
+*/
+ if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+ continue;
+
+ res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+ if (!res) {
+ err = -ENOMEM;
+ goto parse_failed;
+ }
+
+ err = of_pci_range_to_resource(range, dev, res);
+ if (err) {
+ kfree(res);
+ goto parse_failed;
+ }
+
+ if (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;
+ }
+
+ pci_add_resource_offset(resources, res, res-start -
range.pci_addr);
+ }
+
+ return 0;
+
+parse_failed:
+
