[PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Bjorn Helgaas]

Any objections to the patch below?  I posted it last Wednesday,
but haven't heard anything.  Once we have this fix, 8250_pnp
should have sufficient functionality that we can get rid of
8250_acpi.



Use types that match the ACPI resource structures.  Previously
the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
which corrupts the value.

This is one of the things that prevents 8250_pnp from working
on HP ia64 boxes.  After 8250_pnp works, we will be able to
remove 8250_acpi.c.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- work.orig/drivers/pnp/pnpacpi/rsparser.c	2005-07-25 15:04:26.000000000 -0600
+++ work/drivers/pnp/pnpacpi/rsparser.c	2005-07-27 10:02:19.000000000 -0600
@@ -73,7 +73,7 @@
 }
 
 static void
-pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, int irq)
+pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, u32 irq)
 {
 	int i = 0;
 	while (!(res->irq_resource[i].flags & IORESOURCE_UNSET) &&
@@ -85,13 +85,13 @@
 			res->irq_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->irq_resource[i].start =(unsigned long) irq;
-		res->irq_resource[i].end = (unsigned long) irq;
+		res->irq_resource[i].start = irq;
+		res->irq_resource[i].end = irq;
 	}
 }
 
 static void
-pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, int dma)
+pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, u32 dma)
 {
 	int i = 0;
 	while (i < PNP_MAX_DMA &&
@@ -103,14 +103,14 @@
 			res->dma_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->dma_resource[i].start =(unsigned long) dma;
-		res->dma_resource[i].end = (unsigned long) dma;
+		res->dma_resource[i].start = dma;
+		res->dma_resource[i].end = dma;
 	}
 }
 
 static void
 pnpacpi_parse_allocated_ioresource(struct pnp_resource_table * res,
-	int io, int len)
+	u32 io, u32 len)
 {
 	int i = 0;
 	while (!(res->port_resource[i].flags & IORESOURCE_UNSET) &&
@@ -122,14 +122,14 @@
 			res->port_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->port_resource[i].start = (unsigned long) io;
-		res->port_resource[i].end = (unsigned long)(io + len - 1);
+		res->port_resource[i].start = io;
+		res->port_resource[i].end = io + len - 1;
 	}
 }
 
 static void
 pnpacpi_parse_allocated_memresource(struct pnp_resource_table * res,
-	int mem, int len)
+	u64 mem, u64 len)
 {
 	int i = 0;
 	while (!(res->mem_resource[i].flags & IORESOURCE_UNSET) &&
@@ -141,8 +141,8 @@
 			res->mem_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->mem_resource[i].start = (unsigned long) mem;
-		res->mem_resource[i].end = (unsigned long)(mem + len - 1);
+		res->mem_resource[i].start = mem;
+		res->mem_resource[i].end = mem + len - 1;
 	}
 }
 

Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Shaohua Li]

On Tue, 2005-08-02 at 09:55 -0600, Bjorn Helgaas wrote:
> Any objections to the patch below?  I posted it last Wednesday,
> but haven't heard anything.  Once we have this fix, 8250_pnp
> should have sufficient functionality that we can get rid of
> 8250_acpi.
> 
> 
> 
> Use types that match the ACPI resource structures.  Previously
> the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
> which corrupts the value.
> 
> This is one of the things that prevents 8250_pnp from working
> on HP ia64 boxes.  After 8250_pnp works, we will be able to
> remove 8250_acpi.c.
We might always use 'unsigned long'. Did you have plan to remove other
legacy acpi drivers?

Thanks,
Shaohua

Re: [ACPI] [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Kenji Kaneshige]

Hi Bjorn,

>  static void
> -pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, int irq)
> +pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, u32 irq)
>  {
>  	int i = 0;
>  	while (!(res->irq_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -85,13 +85,13 @@
>  			res->irq_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->irq_resource[i].start =(unsigned long) irq;
> -		res->irq_resource[i].end = (unsigned long) irq;
> +		res->irq_resource[i].start = irq;
> +		res->irq_resource[i].end = irq;
>  	}
>  }

This breaks the following patch that is already included into -mm
tree.

http://sourceforge.net/mailarchive/forum.php?thread_id=7844247&forum_id=6102

I think we need to check if acpi_register_gsi() succeeded or not.

Thanks,
Kenji Kaneshige



Bjorn Helgaas wrote:
> Any objections to the patch below?  I posted it last Wednesday,
> but haven't heard anything.  Once we have this fix, 8250_pnp
> should have sufficient functionality that we can get rid of
> 8250_acpi.
> 
> 
> 
> Use types that match the ACPI resource structures.  Previously
> the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
> which corrupts the value.
> 
> This is one of the things that prevents 8250_pnp from working
> on HP ia64 boxes.  After 8250_pnp works, we will be able to
> remove 8250_acpi.c.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work/drivers/pnp/pnpacpi/rsparser.c
> ===================================================================
> --- work.orig/drivers/pnp/pnpacpi/rsparser.c	2005-07-25 15:04:26.000000000 -0600
> +++ work/drivers/pnp/pnpacpi/rsparser.c	2005-07-27 10:02:19.000000000 -0600
> @@ -73,7 +73,7 @@
>  }
>  
>  static void
> -pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, int irq)
> +pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, u32 irq)
>  {
>  	int i = 0;
>  	while (!(res->irq_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -85,13 +85,13 @@
>  			res->irq_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->irq_resource[i].start =(unsigned long) irq;
> -		res->irq_resource[i].end = (unsigned long) irq;
> +		res->irq_resource[i].start = irq;
> +		res->irq_resource[i].end = irq;
>  	}
>  }
>  
>  static void
> -pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, int dma)
> +pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, u32 dma)
>  {
>  	int i = 0;
>  	while (i < PNP_MAX_DMA &&
> @@ -103,14 +103,14 @@
>  			res->dma_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->dma_resource[i].start =(unsigned long) dma;
> -		res->dma_resource[i].end = (unsigned long) dma;
> +		res->dma_resource[i].start = dma;
> +		res->dma_resource[i].end = dma;
>  	}
>  }
>  
>  static void
>  pnpacpi_parse_allocated_ioresource(struct pnp_resource_table * res,
> -	int io, int len)
> +	u32 io, u32 len)
>  {
>  	int i = 0;
>  	while (!(res->port_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -122,14 +122,14 @@
>  			res->port_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->port_resource[i].start = (unsigned long) io;
> -		res->port_resource[i].end = (unsigned long)(io + len - 1);
> +		res->port_resource[i].start = io;
> +		res->port_resource[i].end = io + len - 1;
>  	}
>  }
>  
>  static void
>  pnpacpi_parse_allocated_memresource(struct pnp_resource_table * res,
> -	int mem, int len)
> +	u64 mem, u64 len)
>  {
>  	int i = 0;
>  	while (!(res->mem_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -141,8 +141,8 @@
>  			res->mem_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->mem_resource[i].start = (unsigned long) mem;
> -		res->mem_resource[i].end = (unsigned long)(mem + len - 1);
> +		res->mem_resource[i].start = mem;
> +		res->mem_resource[i].end = mem + len - 1;
>  	}
>  }
>  
> 
> 
> -------------------------------------------------------
> SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
> from IBM. Find simple to follow Roadmaps, straightforward articles,
> informative Webcasts and more! Get everything you need to get up to
> speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
> _______________________________________________
> Acpi-devel mailing list
> Acpi-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/acpi-devel
> 

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Bjorn Helgaas]

On Tuesday 02 August 2005 7:01 pm, Shaohua Li wrote:
> On Tue, 2005-08-02 at 09:55 -0600, Bjorn Helgaas wrote:
> > Any objections to the patch below?  I posted it last Wednesday,
> > but haven't heard anything.  Once we have this fix, 8250_pnp
> > should have sufficient functionality that we can get rid of
> > 8250_acpi.
> > 
> > Use types that match the ACPI resource structures.  Previously
> > the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
> > which corrupts the value.
> > 
> > This is one of the things that prevents 8250_pnp from working
> > on HP ia64 boxes.  After 8250_pnp works, we will be able to
> > remove 8250_acpi.c.
> We might always use 'unsigned long'.

Do you have a reason for preferring 'unsigned long' over the
exact types used in the ACPI resource structures?  I thought
it was useful to use the exact types, because then whatever
conversion needs to happen is all in one place.

In the existing code, there's implicit conversion when you
call "pnpacpi_parse_allocated_memresource(..., int mem, int len)"
and pass u64 values as "mem" and "len".  You have to look both
at the call site and the called code.  And gcc doesn't even
complain about this truncation.

But I guess it doesn't matter much either way.

> Did you have plan to remove other 
> legacy acpi drivers?

No, I didn't -- which ones are you thinking about?  Looking at
the callers of acpi_bus_register_driver(), I see:

	arch/ia64/hp/common/sba_iommu.c
		Probably can't be converted because it needs the
		ACPI handle to extract a vendor-specific data
		item from _CRS.

	drivers/char/hpet.c
		This probably should be converted to PNP.  I'll
		look into doing this.

Then of course, there are a bunch of things in drivers/acpi/
(battery, button, fan, ec, etc).  I expect the reason they are
in drivers/acpi/ is because they need ACPI-specific functionality,
so they probably couldn't be converted to PNP.

Re: [ACPI] [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Bjorn Helgaas]

On Tuesday 02 August 2005 7:05 pm, Kenji Kaneshige wrote:
> This breaks the following patch that is already included into -mm
> tree.
> 
> http://sourceforge.net/mailarchive/forum.php?thread_id=7844247&forum_id=6102
> 
> I think we need to check if acpi_register_gsi() succeeded or not.

You're absolutely right.  I was just based off a Linus tree, non -mm,
and didn't notice that your patch conflicted.  How about the following
(based on 2.6.13-rc4-mm1)?  I moved the acpi_register_gsi() into
pnpacpi_parse_allocated_irqresource(), which I think is nice because
the test for failure is right next to the call.



PNPACPI: fix types when decoding ACPI resources

Use types that match the ACPI resource structures.  Previously
the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
which corrupts the value.

This is one of the things that prevents 8250_pnp from working
on HP ia64 boxes.  After 8250_pnp works, we will be able to
remove 8250_acpi.c.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- work-mm.orig/drivers/pnp/pnpacpi/rsparser.c	2005-08-02 09:39:25.000000000 -0600
+++ work-mm/drivers/pnp/pnpacpi/rsparser.c	2005-08-03 09:31:05.000000000 -0600
@@ -73,25 +73,28 @@
 }
 
 static void
-pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, int irq)
+pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, u32 gsi,
+	int edge_level, int active_high_low)
 {
 	int i = 0;
+	int irq;
 	while (!(res->irq_resource[i].flags & IORESOURCE_UNSET) &&
 			i < PNP_MAX_IRQ)
 		i++;
 	if (i < PNP_MAX_IRQ) {
 		res->irq_resource[i].flags = IORESOURCE_IRQ;  //Also clears _UNSET flag
+		irq = acpi_register_gsi(gsi, edge_level, active_high_low);
 		if (irq < 0) {
 			res->irq_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->irq_resource[i].start =(unsigned long) irq;
-		res->irq_resource[i].end = (unsigned long) irq;
+		res->irq_resource[i].start = irq;
+		res->irq_resource[i].end = irq;
 	}
 }
 
 static void
-pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, int dma)
+pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, u32 dma)
 {
 	int i = 0;
 	while (i < PNP_MAX_DMA &&
@@ -103,14 +106,14 @@
 			res->dma_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->dma_resource[i].start =(unsigned long) dma;
-		res->dma_resource[i].end = (unsigned long) dma;
+		res->dma_resource[i].start = dma;
+		res->dma_resource[i].end = dma;
 	}
 }
 
 static void
 pnpacpi_parse_allocated_ioresource(struct pnp_resource_table * res,
-	int io, int len)
+	u32 io, u32 len)
 {
 	int i = 0;
 	while (!(res->port_resource[i].flags & IORESOURCE_UNSET) &&
@@ -122,14 +125,14 @@
 			res->port_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->port_resource[i].start = (unsigned long) io;
-		res->port_resource[i].end = (unsigned long)(io + len - 1);
+		res->port_resource[i].start = io;
+		res->port_resource[i].end = io + len - 1;
 	}
 }
 
 static void
 pnpacpi_parse_allocated_memresource(struct pnp_resource_table * res,
-	int mem, int len)
+	u64 mem, u64 len)
 {
 	int i = 0;
 	while (!(res->mem_resource[i].flags & IORESOURCE_UNSET) &&
@@ -141,8 +144,8 @@
 			res->mem_resource[i].flags |= IORESOURCE_DISABLED;
 			return;
 		}
-		res->mem_resource[i].start = (unsigned long) mem;
-		res->mem_resource[i].end = (unsigned long)(mem + len - 1);
+		res->mem_resource[i].start = mem;
+		res->mem_resource[i].end = mem + len - 1;
 	}
 }
 
@@ -156,10 +159,10 @@
 	case ACPI_RSTYPE_IRQ:
 		if ((res->data.irq.number_of_interrupts > 0) &&
 			valid_IRQ(res->data.irq.interrupts[0])) {
-			pnpacpi_parse_allocated_irqresource(res_table, 
-				acpi_register_gsi(res->data.irq.interrupts[0],
-					res->data.irq.edge_level,
-					res->data.irq.active_high_low));
+			pnpacpi_parse_allocated_irqresource(res_table,
+				res->data.irq.interrupts[0],
+				res->data.irq.edge_level,
+				res->data.irq.active_high_low);
 			pcibios_penalize_isa_irq(res->data.irq.interrupts[0], 1);
 		}
 		break;
@@ -167,10 +170,10 @@
 	case ACPI_RSTYPE_EXT_IRQ:
 		if ((res->data.extended_irq.number_of_interrupts > 0) &&
 			valid_IRQ(res->data.extended_irq.interrupts[0])) {
-			pnpacpi_parse_allocated_irqresource(res_table, 
-				acpi_register_gsi(res->data.extended_irq.interrupts[0],
-					res->data.extended_irq.edge_level,
-					res->data.extended_irq.active_high_low));
+			pnpacpi_parse_allocated_irqresource(res_table,
+				res->data.extended_irq.interrupts[0],
+				res->data.extended_irq.edge_level,
+				res->data.extended_irq.active_high_low);
 			pcibios_penalize_isa_irq(res->data.extended_irq.interrupts[0], 1);
 		}
 		break;

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[matthieu castet]

Hi,

Bjorn Helgaas wrote:
 > On Tuesday 02 August 2005 7:01 pm, Shaohua Li wrote:
 >

 >
 >>Did you have plan to remove other
 >>legacy acpi drivers?
 >
 >
 > No, I didn't -- which ones are you thinking about?  Looking at
 > the callers of acpi_bus_register_driver(), I see:
looking for METHOD_NAME__CRS is more acurate.
 >
 > 	arch/ia64/hp/common/sba_iommu.c
 > 		Probably can't be converted because it needs the
 > 		ACPI handle to extract a vendor-specific data
 > 		item from _CRS.
 >
 > 	drivers/char/hpet.c
 > 		This probably should be converted to PNP.  I'll
 > 		look into doing this.
IIRC, I am not sure that the pnp layer was able to pass the 64 bits 
memory adress for hpet correctly. But it would be nice if it works.

There are drivers/acpi/motherboard.c that done some stuff already handle 
by pnp/system.c.

There was an extention of a floppy driver in order to use acpi in -mm, 
but it seems to have been dropped.

 >
 > Then of course, there are a bunch of things in drivers/acpi/
 > (battery, button, fan, ec, etc).  I expect the reason they are
 > in drivers/acpi/ is because they need ACPI-specific functionality,
 > so they probably couldn't be converted to PNP.
yes.


Matthieu

PS : I saw in acpi ols paper that you plan once all dupe acpi drivers 
will be removed to register again the pnp device in acpi layer. Do you 
plan to add more check and for example add only device that have a CRS 
in pnp layer ?

PPS : is there any plan to integrate 
http://marc.theaimsgroup.com/?l=linux-kernel&m=111827568001255&w=2 that 
seem to fix some init problem ?

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Bjorn Helgaas]

On Wednesday 03 August 2005 3:16 pm, matthieu castet wrote:
> Bjorn Helgaas wrote:
>  > On Tuesday 02 August 2005 7:01 pm, Shaohua Li wrote:
>  >>Did you have plan to remove other
>  >>legacy acpi drivers?
>  > No, I didn't -- which ones are you thinking about?  Looking at
>  > the callers of acpi_bus_register_driver(), I see:
> looking for METHOD_NAME__CRS is more acurate.

I didn't see any new ones when I looked for METHOD_NAME__CRS.

>  > 	drivers/char/hpet.c
>  > 		This probably should be converted to PNP.  I'll
>  > 		look into doing this.
> IIRC, I am not sure that the pnp layer was able to pass the 64 bits 
> memory adress for hpet correctly. But it would be nice if it works.

You're right, this was broken.  But I've been pushing a PNPACPI
patch to fix this.

> There was an extention of a floppy driver in order to use acpi in -mm, 
> but it seems to have been dropped.

Yeah, I did that, and it was a huge mistake.  The point was to avoid
blind probing for the device, but my concern was for ia64, and no ia64
boxes have floppy, so it's much easier to just not build the driver.

> PS : I saw in acpi ols paper that you plan once all dupe acpi drivers 
> will be removed to register again the pnp device in acpi layer. Do you 
> plan to add more check and for example add only device that have a CRS 
> in pnp layer ?

If you mean the last paragraph of section 6, I don't really understand
it.  But it mentions 8250_acpi.c as an obstacle, and I do know that we
are very close to being able to remove that.  I've already posted two
patches (one to PNPACPI to fix the 64-bit address problem, and one to
8250_pnp to add support for MMIO UARTs).  Once those are accepted, we
should be able to remove 8250_acpi.c.  I think only ia64 really relies
on it anyway.

> PPS : is there any plan to integrate 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=111827568001255&w=2

I'm afraid I don't know anything about this one.

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Shaohua Li]

On Wed, 2005-08-03 at 09:20 -0600, Bjorn Helgaas wrote:
> On Tuesday 02 August 2005 7:01 pm, Shaohua Li wrote:
> > On Tue, 2005-08-02 at 09:55 -0600, Bjorn Helgaas wrote:
> > > Any objections to the patch below?  I posted it last Wednesday,
> > > but haven't heard anything.  Once we have this fix, 8250_pnp
> > > should have sufficient functionality that we can get rid of
> > > 8250_acpi.
> > > 
> > > Use types that match the ACPI resource structures.  Previously
> > > the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
> > > which corrupts the value.
> > > 
> > > This is one of the things that prevents 8250_pnp from working
> > > on HP ia64 boxes.  After 8250_pnp works, we will be able to
> > > remove 8250_acpi.c.
> > We might always use 'unsigned long'.
> 
> Do you have a reason for preferring 'unsigned long' over the
> exact types used in the ACPI resource structures?  I thought
> it was useful to use the exact types, because then whatever
> conversion needs to happen is all in one place.
> 
> In the existing code, there's implicit conversion when you
> call "pnpacpi_parse_allocated_memresource(..., int mem, int len)"
> and pass u64 values as "mem" and "len".  You have to look both
> at the call site and the called code.  And gcc doesn't even
> complain about this truncation.
> 
> But I guess it doesn't matter much either way.
Either is ok to me.

> 
> > Did you have plan to remove other 
> > legacy acpi drivers?
> 
> No, I didn't -- which ones are you thinking about?  Looking at
> the callers of acpi_bus_register_driver(), I see:
> 
> 	arch/ia64/hp/common/sba_iommu.c
> 		Probably can't be converted because it needs the
> 		ACPI handle to extract a vendor-specific data
> 		item from _CRS.
> 
> 	drivers/char/hpet.c
> 		This probably should be converted to PNP.  I'll
> 		look into doing this.
Great! After then we could push the ACPIPNP hotplug staff.

Thanks,
Shaohua

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Shaohua Li]

On Wed, 2005-08-03 at 23:16 +0200, matthieu castet wrote:
> 
> There are drivers/acpi/motherboard.c that done some stuff already handle 
> by pnp/system.c.
Yes, it should be disabled if pnpacpi is enabled. The only concern is
motherboard.c also request some ACPI resources, which might not declaim
in ACPI motherboard device, but it's completely BIOS dependent. We might
could try remove it at -mm tree to see if it breaks any system later.

> 
> PS : I saw in acpi ols paper that you plan once all dupe acpi drivers 
> will be removed to register again the pnp device in acpi layer. Do you 
> plan to add more check and for example add only device that have a CRS 
> in pnp layer ?
For detecting PNP device? it's worthy trying.

Thanks,
Shaohua

Re: [ACPI] [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Kenji Kaneshige]

Hi Bjorn,

Thank you very much for the new patch
and I'm very sorry for troubling you.

The patch looks very good to me.

Thanks,
Kenji Kaneshige


Bjorn Helgaas wrote:
> On Tuesday 02 August 2005 7:05 pm, Kenji Kaneshige wrote:
> 
>>This breaks the following patch that is already included into -mm
>>tree.
>>
>>http://sourceforge.net/mailarchive/forum.php?thread_id=7844247&forum_id=6102
>>
>>I think we need to check if acpi_register_gsi() succeeded or not.
> 
> 
> You're absolutely right.  I was just based off a Linus tree, non -mm,
> and didn't notice that your patch conflicted.  How about the following
> (based on 2.6.13-rc4-mm1)?  I moved the acpi_register_gsi() into
> pnpacpi_parse_allocated_irqresource(), which I think is nice because
> the test for failure is right next to the call.
> 
> 
> 
> PNPACPI: fix types when decoding ACPI resources
> 
> Use types that match the ACPI resource structures.  Previously
> the u64 value from an RSTYPE_ADDRESS64 was passed as an int,
> which corrupts the value.
> 
> This is one of the things that prevents 8250_pnp from working
> on HP ia64 boxes.  After 8250_pnp works, we will be able to
> remove 8250_acpi.c.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work-mm/drivers/pnp/pnpacpi/rsparser.c
> ===================================================================
> --- work-mm.orig/drivers/pnp/pnpacpi/rsparser.c	2005-08-02 09:39:25.000000000 -0600
> +++ work-mm/drivers/pnp/pnpacpi/rsparser.c	2005-08-03 09:31:05.000000000 -0600
> @@ -73,25 +73,28 @@
>  }
>  
>  static void
> -pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, int irq)
> +pnpacpi_parse_allocated_irqresource(struct pnp_resource_table * res, u32 gsi,
> +	int edge_level, int active_high_low)
>  {
>  	int i = 0;
> +	int irq;
>  	while (!(res->irq_resource[i].flags & IORESOURCE_UNSET) &&
>  			i < PNP_MAX_IRQ)
>  		i++;
>  	if (i < PNP_MAX_IRQ) {
>  		res->irq_resource[i].flags = IORESOURCE_IRQ;  //Also clears _UNSET flag
> +		irq = acpi_register_gsi(gsi, edge_level, active_high_low);
>  		if (irq < 0) {
>  			res->irq_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->irq_resource[i].start =(unsigned long) irq;
> -		res->irq_resource[i].end = (unsigned long) irq;
> +		res->irq_resource[i].start = irq;
> +		res->irq_resource[i].end = irq;
>  	}
>  }
>  
>  static void
> -pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, int dma)
> +pnpacpi_parse_allocated_dmaresource(struct pnp_resource_table * res, u32 dma)
>  {
>  	int i = 0;
>  	while (i < PNP_MAX_DMA &&
> @@ -103,14 +106,14 @@
>  			res->dma_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->dma_resource[i].start =(unsigned long) dma;
> -		res->dma_resource[i].end = (unsigned long) dma;
> +		res->dma_resource[i].start = dma;
> +		res->dma_resource[i].end = dma;
>  	}
>  }
>  
>  static void
>  pnpacpi_parse_allocated_ioresource(struct pnp_resource_table * res,
> -	int io, int len)
> +	u32 io, u32 len)
>  {
>  	int i = 0;
>  	while (!(res->port_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -122,14 +125,14 @@
>  			res->port_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->port_resource[i].start = (unsigned long) io;
> -		res->port_resource[i].end = (unsigned long)(io + len - 1);
> +		res->port_resource[i].start = io;
> +		res->port_resource[i].end = io + len - 1;
>  	}
>  }
>  
>  static void
>  pnpacpi_parse_allocated_memresource(struct pnp_resource_table * res,
> -	int mem, int len)
> +	u64 mem, u64 len)
>  {
>  	int i = 0;
>  	while (!(res->mem_resource[i].flags & IORESOURCE_UNSET) &&
> @@ -141,8 +144,8 @@
>  			res->mem_resource[i].flags |= IORESOURCE_DISABLED;
>  			return;
>  		}
> -		res->mem_resource[i].start = (unsigned long) mem;
> -		res->mem_resource[i].end = (unsigned long)(mem + len - 1);
> +		res->mem_resource[i].start = mem;
> +		res->mem_resource[i].end = mem + len - 1;
>  	}
>  }
>  
> @@ -156,10 +159,10 @@
>  	case ACPI_RSTYPE_IRQ:
>  		if ((res->data.irq.number_of_interrupts > 0) &&
>  			valid_IRQ(res->data.irq.interrupts[0])) {
> -			pnpacpi_parse_allocated_irqresource(res_table, 
> -				acpi_register_gsi(res->data.irq.interrupts[0],
> -					res->data.irq.edge_level,
> -					res->data.irq.active_high_low));
> +			pnpacpi_parse_allocated_irqresource(res_table,
> +				res->data.irq.interrupts[0],
> +				res->data.irq.edge_level,
> +				res->data.irq.active_high_low);
>  			pcibios_penalize_isa_irq(res->data.irq.interrupts[0], 1);
>  		}
>  		break;
> @@ -167,10 +170,10 @@
>  	case ACPI_RSTYPE_EXT_IRQ:
>  		if ((res->data.extended_irq.number_of_interrupts > 0) &&
>  			valid_IRQ(res->data.extended_irq.interrupts[0])) {
> -			pnpacpi_parse_allocated_irqresource(res_table, 
> -				acpi_register_gsi(res->data.extended_irq.interrupts[0],
> -					res->data.extended_irq.edge_level,
> -					res->data.extended_irq.active_high_low));
> +			pnpacpi_parse_allocated_irqresource(res_table,
> +				res->data.extended_irq.interrupts[0],
> +				res->data.extended_irq.edge_level,
> +				res->data.extended_irq.active_high_low);
>  			pcibios_penalize_isa_irq(res->data.extended_irq.interrupts[0], 1);
>  		}
>  		break;
> 

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[matthieu castet]

Hi,

Bjorn Helgaas wrote:
> On Wednesday 03 August 2005 3:16 pm, matthieu castet wrote:
> 
>>Bjorn Helgaas wrote:
>> > 	drivers/char/hpet.c
>> > 		This probably should be converted to PNP.  I'll
>> > 		look into doing this.
>>IIRC, I am not sure that the pnp layer was able to pass the 64 bits 
>>memory adress for hpet correctly. But it would be nice if it works.
> 
> 
> You're right, this was broken.  But I've been pushing a PNPACPI
> patch to fix this.
> 
> 
Yes but is ACPI_RSTYPE_ADDRESS64 possible on 32 bit machine ?
In this case your patch won't work as res->mem_resource[i].start and 
res->mem_resource[i].end are unsigned long, and 64 bit value won't fit.

Matthieu

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[Bjorn Helgaas]

On Thursday 04 August 2005 6:38 am, matthieu castet wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 03 August 2005 3:16 pm, matthieu castet wrote:
> >>Bjorn Helgaas wrote:
> >> > 	drivers/char/hpet.c
> >> > 		This probably should be converted to PNP.  I'll
> >> > 		look into doing this.
> >>IIRC, I am not sure that the pnp layer was able to pass the 64 bits 
> >>memory adress for hpet correctly. But it would be nice if it works.
> > 
> > You're right, this was broken.  But I've been pushing a PNPACPI
> > patch to fix this.
> > 
> Yes but is ACPI_RSTYPE_ADDRESS64 possible on 32 bit machine ?

I can't think of a case where that would make sense, but I don't
actually know the answer.

> In this case your patch won't work as res->mem_resource[i].start and 
> res->mem_resource[i].end are unsigned long, and 64 bit value won't fit.

True.  But fixing that would be pretty far-reaching (changing struct
resource), so I'm not worried until it is shown to be a problem.

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[matthieu castet]

Bjorn Helgaas wrote:
> On Thursday 04 August 2005 6:38 am, matthieu castet wrote:
> 
>>Bjorn Helgaas wrote:
>>
>>>On Wednesday 03 August 2005 3:16 pm, matthieu castet wrote:
>>>
>>>>Bjorn Helgaas wrote:
>>>>
>>>>>	drivers/char/hpet.c
>>>>>		This probably should be converted to PNP.  I'll
>>>>>		look into doing this.
>>>>
>>>>IIRC, I am not sure that the pnp layer was able to pass the 64 bits 
>>>>memory adress for hpet correctly. But it would be nice if it works.
>>>
>>>You're right, this was broken.  But I've been pushing a PNPACPI
>>>patch to fix this.
>>>
>>
>>Yes but is ACPI_RSTYPE_ADDRESS64 possible on 32 bit machine ?
> 
> 
> I can't think of a case where that would make sense, but I don't
> actually know the answer.
> 
> 
>>In this case your patch won't work as res->mem_resource[i].start and 
>>res->mem_resource[i].end are unsigned long, and 64 bit value won't fit.
> 
> 
> True.  But fixing that would be pretty far-reaching (changing struct
> resource), so I'm not worried until it is shown to be a problem.
> 
Ok, may be you could add a BUG_ON(sizeof(long)==4) for 
ACPI_RSTYPE_ADDRESS64.

Matthieu

Re: [ACPI] Re: [PATCH] PNPACPI: fix types when decoding ACPI resources [resend]

[matthieu castet]

Hi,

Shaohua Li wrote:
> On Wed, 2005-08-03 at 23:16 +0200, matthieu castet wrote:
> 
>>There are drivers/acpi/motherboard.c that done some stuff already handle 
>>by pnp/system.c.
> 
> Yes, it should be disabled if pnpacpi is enabled. 
But even if pnpacpi is disabled, pnp/system.c sould work with pnpbios.
> The only concern is
> motherboard.c also request some ACPI resources, which might not declaim
> in ACPI motherboard device, but it's completely BIOS dependent. We might
> could try remove it at -mm tree to see if it breaks any system later.
> 
> 
Ok,
may be we should split in 2 modules : the one that request some ACPI 
resources and the other that use PNP resource.
>>PS : I saw in acpi ols paper that you plan once all dupe acpi drivers 
>>will be removed to register again the pnp device in acpi layer. Do you 
>>plan to add more check and for example add only device that have a CRS 
>>in pnp layer ?
> 
> For detecting PNP device? it's worthy trying.
> 

I will send a patch for that.

Matthieu