diff mbox

[RFC] drm/radeon: combios tables on older cards

Message ID 201307211521.r6LFLSsa005334@glazunov.sibelius.xs4all.nl (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Kettenis July 21, 2013, 3:21 p.m. UTC 
Noticed that my old Radeon 7500 hung after printing

   drm: GPU not posted. posting now...

when it wasn't selected as the primary card the BIOS.  Some digging
revealed that it was hanging in combios_parse_mmio_table() while
parsing the ASIC INIT 3 table.  Looking at the BIOS ROM for the card,
it becomes obvious that there is no ASIC INIT 3 table in the BIOS.
The code is just processing random garbage.  No surprise it hangs!

Why do I say that there is no ASIC INIT 3 table is the BIOS?  This
table is found through the MISC INFO table.  The MISC INFO table can
be found at offset 0x5e in the COMBIOS header.  But the header is
smaller than that.  The COMBIOS header starts at offset 0x126.  The
standard PCI Data Structure (the bit that starts with 'PCIR') lives at
offset 0x180.  That means that the COMBIOS header can not be larger
than 0x5a bytes and therefore cannot contain a MISC INFO table.

I looked at a dozen or so BIOS images, some my own, some downloaded from:

    <http://www.techpowerup.com/vgabios/index.php?manufacturer=ATI&page=1>

It is fairly obvious that the size of the COMBIOS header can be found
at offset 0x6 of the header.  Not sure if it is a 16-bit number or
just an 8-bit number, but that doesn't really matter since the tables
seems to be always smaller than 256 bytes.

So I think combios_get_table_offset() should check if the requested
table is present.  This can be done by checking the offset against the
size of the header.  See the diff below.  The diff is against the WIP
OpenBSD codebase that roughly corresponds to Linux 3.8.13 at this
point.  But I don't think this bit of the code changed much since
then.

For what it is worth:

Signed-off-by: Mark Kettenis <kettenis@openbsd.org>

Comments

Alex Deucher July 21, 2013, 8:46 p.m. UTC | #1 
On Sun, Jul 21, 2013 at 11:21 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> Noticed that my old Radeon 7500 hung after printing
>
>    drm: GPU not posted. posting now...
>
> when it wasn't selected as the primary card the BIOS.  Some digging
> revealed that it was hanging in combios_parse_mmio_table() while
> parsing the ASIC INIT 3 table.  Looking at the BIOS ROM for the card,
> it becomes obvious that there is no ASIC INIT 3 table in the BIOS.
> The code is just processing random garbage.  No surprise it hangs!
>
> Why do I say that there is no ASIC INIT 3 table is the BIOS?  This
> table is found through the MISC INFO table.  The MISC INFO table can
> be found at offset 0x5e in the COMBIOS header.  But the header is
> smaller than that.  The COMBIOS header starts at offset 0x126.  The
> standard PCI Data Structure (the bit that starts with 'PCIR') lives at
> offset 0x180.  That means that the COMBIOS header can not be larger
> than 0x5a bytes and therefore cannot contain a MISC INFO table.
>
> I looked at a dozen or so BIOS images, some my own, some downloaded from:
>
>     <http://www.techpowerup.com/vgabios/index.php?manufacturer=ATI&page=1>
>
> It is fairly obvious that the size of the COMBIOS header can be found
> at offset 0x6 of the header.  Not sure if it is a 16-bit number or
> just an 8-bit number, but that doesn't really matter since the tables
> seems to be always smaller than 256 bytes.
>
> So I think combios_get_table_offset() should check if the requested
> table is present.  This can be done by checking the offset against the
> size of the header.  See the diff below.  The diff is against the WIP
> OpenBSD codebase that roughly corresponds to Linux 3.8.13 at this
> point.  But I don't think this bit of the code changed much since
> then.


Your assessment is correct.  I've gone ahead and applied the patch.

Thanks!

Alex

>
> For what it is worth:
>
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
>
>
> diff --git a/sys/dev/pci/drm/radeon/radeon_combios.c b/sys/dev/pci/drm/radeon/radeon_combios.c
> index c2b3535..81c3f00 100644
> --- a/sys/dev/pci/drm/radeon/radeon_combios.c
> +++ b/sys/dev/pci/drm/radeon/radeon_combios.c
> @@ -144,7 +144,7 @@ static uint16_t combios_get_table_offset(struct drm_device *dev,
>                                          enum radeon_combios_table_offset table)
>  {
>         struct radeon_device *rdev = dev->dev_private;
> -       int rev;
> +       int rev, size;
>         uint16_t offset = 0, check_offset;
>
>         if (!rdev->bios)
> @@ -153,174 +153,106 @@ static uint16_t combios_get_table_offset(struct drm_device *dev,
>         switch (table) {
>                 /* absolute offset tables */
>         case COMBIOS_ASIC_INIT_1_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0xc);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0xc;
>                 break;
>         case COMBIOS_BIOS_SUPPORT_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x14);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x14;
>                 break;
>         case COMBIOS_DAC_PROGRAMMING_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x2a);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x2a;
>                 break;
>         case COMBIOS_MAX_COLOR_DEPTH_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x2c);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x2c;
>                 break;
>         case COMBIOS_CRTC_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x2e);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x2e;
>                 break;
>         case COMBIOS_PLL_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x30);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x30;
>                 break;
>         case COMBIOS_TV_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x32);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x32;
>                 break;
>         case COMBIOS_DFP_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x34);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x34;
>                 break;
>         case COMBIOS_HW_CONFIG_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x36);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x36;
>                 break;
>         case COMBIOS_MULTIMEDIA_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x38);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x38;
>                 break;
>         case COMBIOS_TV_STD_PATCH_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x3e);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x3e;
>                 break;
>         case COMBIOS_LCD_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x40);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x40;
>                 break;
>         case COMBIOS_MOBILE_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x42);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x42;
>                 break;
>         case COMBIOS_PLL_INIT_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x46);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x46;
>                 break;
>         case COMBIOS_MEM_CONFIG_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x48);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x48;
>                 break;
>         case COMBIOS_SAVE_MASK_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x4a);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x4a;
>                 break;
>         case COMBIOS_HARDCODED_EDID_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x4c);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x4c;
>                 break;
>         case COMBIOS_ASIC_INIT_2_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x4e);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x4e;
>                 break;
>         case COMBIOS_CONNECTOR_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x50);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x50;
>                 break;
>         case COMBIOS_DYN_CLK_1_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x52);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x52;
>                 break;
>         case COMBIOS_RESERVED_MEM_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x54);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x54;
>                 break;
>         case COMBIOS_EXT_TMDS_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x58);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x58;
>                 break;
>         case COMBIOS_MEM_CLK_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x5a);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x5a;
>                 break;
>         case COMBIOS_EXT_DAC_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x5c);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x5c;
>                 break;
>         case COMBIOS_MISC_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x5e);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x5e;
>                 break;
>         case COMBIOS_CRT_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x60);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x60;
>                 break;
>         case COMBIOS_INTEGRATED_SYSTEM_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x62);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x62;
>                 break;
>         case COMBIOS_COMPONENT_VIDEO_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x64);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x64;
>                 break;
>         case COMBIOS_FAN_SPEED_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x66);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x66;
>                 break;
>         case COMBIOS_OVERDRIVE_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x68);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x68;
>                 break;
>         case COMBIOS_OEM_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x6a);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x6a;
>                 break;
>         case COMBIOS_DYN_CLK_2_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x6c);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x6c;
>                 break;
>         case COMBIOS_POWER_CONNECTOR_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x6e);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x6e;
>                 break;
>         case COMBIOS_I2C_INFO_TABLE:
> -               check_offset = RBIOS16(rdev->bios_header_start + 0x70);
> -               if (check_offset)
> -                       offset = check_offset;
> +               check_offset = 0x70;
>                 break;
>                 /* relative offset tables */
>         case COMBIOS_ASIC_INIT_3_TABLE: /* offset from misc info */
> @@ -439,8 +371,11 @@ static uint16_t combios_get_table_offset(struct drm_device *dev,
>                 break;
>         }
>
> -       return offset;
> +       size = RBIOS8(rdev->bios_header_start + 0x6);
> +       if (table < COMBIOS_ASIC_INIT_3_TABLE && check_offset < size)
> +               offset = RBIOS16(rdev->bios_header_start + check_offset);
>
> +       return offset;
>  }
>
>  bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/sys/dev/pci/drm/radeon/radeon_combios.c b/sys/dev/pci/drm/radeon/radeon_combios.c
index c2b3535..81c3f00 100644
--- a/sys/dev/pci/drm/radeon/radeon_combios.c
+++ b/sys/dev/pci/drm/radeon/radeon_combios.c
@@ -144,7 +144,7 @@  static uint16_t combios_get_table_offset(struct drm_device *dev,
 					 enum radeon_combios_table_offset table)
 {
 	struct radeon_device *rdev = dev->dev_private;
-	int rev;
+	int rev, size;
 	uint16_t offset = 0, check_offset;
 
 	if (!rdev->bios)
@@ -153,174 +153,106 @@  static uint16_t combios_get_table_offset(struct drm_device *dev,
 	switch (table) {
 		/* absolute offset tables */
 	case COMBIOS_ASIC_INIT_1_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0xc);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0xc;
 		break;
 	case COMBIOS_BIOS_SUPPORT_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x14);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x14;
 		break;
 	case COMBIOS_DAC_PROGRAMMING_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x2a);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x2a;
 		break;
 	case COMBIOS_MAX_COLOR_DEPTH_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x2c);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x2c;
 		break;
 	case COMBIOS_CRTC_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x2e);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x2e;
 		break;
 	case COMBIOS_PLL_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x30);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x30;
 		break;
 	case COMBIOS_TV_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x32);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x32;
 		break;
 	case COMBIOS_DFP_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x34);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x34;
 		break;
 	case COMBIOS_HW_CONFIG_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x36);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x36;
 		break;
 	case COMBIOS_MULTIMEDIA_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x38);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x38;
 		break;
 	case COMBIOS_TV_STD_PATCH_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x3e);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x3e;
 		break;
 	case COMBIOS_LCD_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x40);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x40;
 		break;
 	case COMBIOS_MOBILE_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x42);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x42;
 		break;
 	case COMBIOS_PLL_INIT_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x46);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x46;
 		break;
 	case COMBIOS_MEM_CONFIG_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x48);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x48;
 		break;
 	case COMBIOS_SAVE_MASK_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x4a);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x4a;
 		break;
 	case COMBIOS_HARDCODED_EDID_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x4c);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x4c;
 		break;
 	case COMBIOS_ASIC_INIT_2_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x4e);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x4e;
 		break;
 	case COMBIOS_CONNECTOR_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x50);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x50;
 		break;
 	case COMBIOS_DYN_CLK_1_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x52);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x52;
 		break;
 	case COMBIOS_RESERVED_MEM_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x54);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x54;
 		break;
 	case COMBIOS_EXT_TMDS_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x58);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x58;
 		break;
 	case COMBIOS_MEM_CLK_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x5a);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x5a;
 		break;
 	case COMBIOS_EXT_DAC_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x5c);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x5c;
 		break;
 	case COMBIOS_MISC_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x5e);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x5e;
 		break;
 	case COMBIOS_CRT_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x60);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x60;
 		break;
 	case COMBIOS_INTEGRATED_SYSTEM_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x62);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x62;
 		break;
 	case COMBIOS_COMPONENT_VIDEO_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x64);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x64;
 		break;
 	case COMBIOS_FAN_SPEED_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x66);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x66;
 		break;
 	case COMBIOS_OVERDRIVE_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x68);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x68;
 		break;
 	case COMBIOS_OEM_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x6a);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x6a;
 		break;
 	case COMBIOS_DYN_CLK_2_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x6c);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x6c;
 		break;
 	case COMBIOS_POWER_CONNECTOR_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x6e);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x6e;
 		break;
 	case COMBIOS_I2C_INFO_TABLE:
-		check_offset = RBIOS16(rdev->bios_header_start + 0x70);
-		if (check_offset)
-			offset = check_offset;
+		check_offset = 0x70;
 		break;
 		/* relative offset tables */
 	case COMBIOS_ASIC_INIT_3_TABLE:	/* offset from misc info */
@@ -439,8 +371,11 @@  static uint16_t combios_get_table_offset(struct drm_device *dev,
 		break;
 	}
 
-	return offset;
+	size = RBIOS8(rdev->bios_header_start + 0x6);
+	if (table < COMBIOS_ASIC_INIT_3_TABLE && check_offset < size)
+		offset = RBIOS16(rdev->bios_header_start + check_offset);
 
+	return offset;
 }
 
 bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev)