PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

[PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Rob Herring (Arm) 1 year, 3 months ago

The mvebu "ranges" is a bit unusual with its own encoding of addresses,
but it's still just normal "ranges" as far as parsing is concerned.
Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
instead of open coding the parsing.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Compile tested only.
---
 drivers/pci/controller/pci-mvebu.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 29fe09c99e7d..d4e3f1e76f84 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -1179,37 +1179,29 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 			      unsigned int *tgt,
 			      unsigned int *attr)
 {
-	const int na = 3, ns = 2;
-	const __be32 *range;
-	int rlen, nranges, rangesz, pna, i;
+	struct of_range range;
+	struct of_range_parser parser;
 
 	*tgt = -1;
 	*attr = -1;
 
-	range = of_get_property(np, "ranges", &rlen);
-	if (!range)
+	if (of_pci_range_parser_init(&parser, np))
 		return -EINVAL;
 
-	pna = of_n_addr_cells(np);
-	rangesz = pna + na + ns;
-	nranges = rlen / sizeof(__be32) / rangesz;
-
-	for (i = 0; i < nranges; i++, range += rangesz) {
-		u32 flags = of_read_number(range, 1);
-		u32 slot = of_read_number(range + 1, 1);
-		u64 cpuaddr = of_read_number(range + na, pna);
+	for_each_of_range(&parser, &range) {
 		unsigned long rtype;
+		u32 slot = upper_32_bits(range.bus_addr);
 
-		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
+		if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_IO)
 			rtype = IORESOURCE_IO;
-		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
+		else if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_MEM32)
 			rtype = IORESOURCE_MEM;
 		else
 			continue;
 
 		if (slot == PCI_SLOT(devfn) && type == rtype) {
-			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
-			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
+			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
+			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);
 			return 0;
 		}
 	}
-- 
2.45.2

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Manivannan Sadhasivam 9 months, 3 weeks ago

On Thu, 07 Nov 2024 09:32:55 -0600, Rob Herring (Arm) wrote:
> The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> but it's still just normal "ranges" as far as parsing is concerned.
> Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> instead of open coding the parsing.
> 
> 

Applied to controller/mvebu, thanks!

[1/1] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"
      commit: 506d34571e2f204c991aefe3f1300175907594e3

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Manivannan Sadhasivam 1 year, 2 months ago

On Thu, Nov 07, 2024 at 09:32:55AM -0600, Rob Herring (Arm) wrote:
> The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> but it's still just normal "ranges" as far as parsing is concerned.
> Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> instead of open coding the parsing.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

LGTM!

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Could someone please verify it on mvebu machine?

- Mani

> ---
> Compile tested only.
> ---
>  drivers/pci/controller/pci-mvebu.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 29fe09c99e7d..d4e3f1e76f84 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1179,37 +1179,29 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  			      unsigned int *tgt,
>  			      unsigned int *attr)
>  {
> -	const int na = 3, ns = 2;
> -	const __be32 *range;
> -	int rlen, nranges, rangesz, pna, i;
> +	struct of_range range;
> +	struct of_range_parser parser;
>  
>  	*tgt = -1;
>  	*attr = -1;
>  
> -	range = of_get_property(np, "ranges", &rlen);
> -	if (!range)
> +	if (of_pci_range_parser_init(&parser, np))
>  		return -EINVAL;
>  
> -	pna = of_n_addr_cells(np);
> -	rangesz = pna + na + ns;
> -	nranges = rlen / sizeof(__be32) / rangesz;
> -
> -	for (i = 0; i < nranges; i++, range += rangesz) {
> -		u32 flags = of_read_number(range, 1);
> -		u32 slot = of_read_number(range + 1, 1);
> -		u64 cpuaddr = of_read_number(range + na, pna);
> +	for_each_of_range(&parser, &range) {
>  		unsigned long rtype;
> +		u32 slot = upper_32_bits(range.bus_addr);
>  
> -		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
> +		if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_IO)
>  			rtype = IORESOURCE_IO;
> -		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
> +		else if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_MEM32)
>  			rtype = IORESOURCE_MEM;
>  		else
>  			continue;
>  
>  		if (slot == PCI_SLOT(devfn) && type == rtype) {
> -			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
> -			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
> +			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
> +			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);
>  			return 0;
>  		}
>  	}
> -- 
> 2.45.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Pali Rohár 11 months, 3 weeks ago

On Friday 15 November 2024 13:01:04 Manivannan Sadhasivam wrote:
> On Thu, Nov 07, 2024 at 09:32:55AM -0600, Rob Herring (Arm) wrote:
> > The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> > but it's still just normal "ranges" as far as parsing is concerned.
> > Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> > instead of open coding the parsing.
> > 
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> 
> LGTM!
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Could someone please verify it on mvebu machine?
> 
> - Mani

That is mostly impossible as pci-mvebu is broken. Bjorn and Krzysztof in
past already refused to take patches which would fix the driver or
extend it for other platforms.

So I do not understand why you are rewriting something which worked,
instead of fixing something which is broken. The only point can be to
make driver even more broken...

> > ---
> > Compile tested only.
> > ---
> >  drivers/pci/controller/pci-mvebu.c | 26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index 29fe09c99e7d..d4e3f1e76f84 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -1179,37 +1179,29 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
> >  			      unsigned int *tgt,
> >  			      unsigned int *attr)
> >  {
> > -	const int na = 3, ns = 2;
> > -	const __be32 *range;
> > -	int rlen, nranges, rangesz, pna, i;
> > +	struct of_range range;
> > +	struct of_range_parser parser;
> >  
> >  	*tgt = -1;
> >  	*attr = -1;
> >  
> > -	range = of_get_property(np, "ranges", &rlen);
> > -	if (!range)
> > +	if (of_pci_range_parser_init(&parser, np))
> >  		return -EINVAL;
> >  
> > -	pna = of_n_addr_cells(np);
> > -	rangesz = pna + na + ns;
> > -	nranges = rlen / sizeof(__be32) / rangesz;
> > -
> > -	for (i = 0; i < nranges; i++, range += rangesz) {
> > -		u32 flags = of_read_number(range, 1);
> > -		u32 slot = of_read_number(range + 1, 1);
> > -		u64 cpuaddr = of_read_number(range + na, pna);
> > +	for_each_of_range(&parser, &range) {
> >  		unsigned long rtype;
> > +		u32 slot = upper_32_bits(range.bus_addr);
> >  
> > -		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
> > +		if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_IO)
> >  			rtype = IORESOURCE_IO;
> > -		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
> > +		else if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_MEM32)
> >  			rtype = IORESOURCE_MEM;
> >  		else
> >  			continue;
> >  
> >  		if (slot == PCI_SLOT(devfn) && type == rtype) {
> > -			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
> > -			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
> > +			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
> > +			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);
> >  			return 0;
> >  		}
> >  	}
> > -- 
> > 2.45.2
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Rob Herring 11 months, 3 weeks ago

On Sun, Feb 16, 2025 at 10:54 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 15 November 2024 13:01:04 Manivannan Sadhasivam wrote:
> > On Thu, Nov 07, 2024 at 09:32:55AM -0600, Rob Herring (Arm) wrote:
> > > The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> > > but it's still just normal "ranges" as far as parsing is concerned.
> > > Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> > > instead of open coding the parsing.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> >
> > LGTM!
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
> > Could someone please verify it on mvebu machine?
> >
> > - Mani
>
> That is mostly impossible as pci-mvebu is broken. Bjorn and Krzysztof in
> past already refused to take patches which would fix the driver or
> extend it for other platforms.

That makes no sense. The driver is broken and we won't take patches to fix it.

> So I do not understand why you are rewriting something which worked,
> instead of fixing something which is broken. The only point can be to
> make driver even more broken...

We make coding improvements to the kernel code all the time. There is
no way folks can test every platform. Either drivers need to get
tested at some frequency or they should just be removed until someone
cares enough to maintain them.

Rob

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Pali Rohár 1 year, 3 months ago

On Thursday 07 November 2024 09:32:55 Rob Herring (Arm) wrote:
> The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> but it's still just normal "ranges" as far as parsing is concerned.
> Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> instead of open coding the parsing.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Compile tested only.

I see no reason for such change, which was even non tested at all and
does not fix any issue. There are more important issues in the driver,
it was decided that bug fixes are not going to be included (yet).

> ---
>  drivers/pci/controller/pci-mvebu.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 29fe09c99e7d..d4e3f1e76f84 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1179,37 +1179,29 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  			      unsigned int *tgt,
>  			      unsigned int *attr)
>  {
> -	const int na = 3, ns = 2;
> -	const __be32 *range;
> -	int rlen, nranges, rangesz, pna, i;
> +	struct of_range range;
> +	struct of_range_parser parser;
>  
>  	*tgt = -1;
>  	*attr = -1;
>  
> -	range = of_get_property(np, "ranges", &rlen);
> -	if (!range)
> +	if (of_pci_range_parser_init(&parser, np))
>  		return -EINVAL;
>  
> -	pna = of_n_addr_cells(np);
> -	rangesz = pna + na + ns;
> -	nranges = rlen / sizeof(__be32) / rangesz;
> -
> -	for (i = 0; i < nranges; i++, range += rangesz) {
> -		u32 flags = of_read_number(range, 1);
> -		u32 slot = of_read_number(range + 1, 1);
> -		u64 cpuaddr = of_read_number(range + na, pna);
> +	for_each_of_range(&parser, &range) {
>  		unsigned long rtype;
> +		u32 slot = upper_32_bits(range.bus_addr);
>  
> -		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
> +		if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_IO)
>  			rtype = IORESOURCE_IO;
> -		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
> +		else if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_MEM32)
>  			rtype = IORESOURCE_MEM;
>  		else
>  			continue;
>  
>  		if (slot == PCI_SLOT(devfn) && type == rtype) {
> -			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
> -			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
> +			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
> +			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);
>  			return 0;
>  		}
>  	}
> -- 
> 2.45.2
>

Re: [PATCH] PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"

Posted by Rob Herring 1 year, 3 months ago

On Thu, Nov 7, 2024 at 11:00 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 07 November 2024 09:32:55 Rob Herring (Arm) wrote:
> > The mvebu "ranges" is a bit unusual with its own encoding of addresses,
> > but it's still just normal "ranges" as far as parsing is concerned.
> > Convert mvebu_get_tgt_attr() to use the for_each_of_range() iterator
> > instead of open coding the parsing.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Compile tested only.
>
> I see no reason for such change, which was even non tested at all and
> does not fix any issue.

Maintenance of the kernel is an issue. Maybe not for you, but for some of us.

>  There are more important issues in the driver,
> it was decided that bug fixes are not going to be included (yet).

The larger reason is to get rid of custom parsing of address
properties throughout the kernel. As well as remove and consolidate
uses of_n_addr_cells() and remove its support of long since deprecated
behavior. Also, I intend to make of_get_property/of_find_property()
warn about leaking data since it just returns a pointer into the raw
DT data and we have no control over the lifetime. Then we can't free
those properties. Why do we care? Overlays and Rust.

Rob