drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset

The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id enum transform 1:1 into bit positions within the 'enabled' bitset as assembled by evergreen_hpd_init():

  enabled |= 1 << radeon_connector->hpd.hpd;

However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN reports

  UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
  shift exponent 255 is too large for 32-bit type 'int'
  [...]
  Call Trace:
   [<ffffffff818c4d35>] dump_stack+0xbc/0x117
   [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
   [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
   [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
   [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
   [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
   [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
   [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
   [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
   [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
   [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
   [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
   [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
   [<ffffffff814b83f0>] ? kfree+0x220/0x370
   [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
   [...]
  =====================================================================
  radeon 0000:01:00.0: No connectors reported connected with modes

At least on x86, there should be no user-visible impact as there

1 << 0xff == 1 << (0xff & 31) == 1 << 31

holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.

All of the above applies analogously to evergreen_hpd_fini().

Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset in evergreen_hpd_fini() respectively.

Signed-off-by Nicolai Stange <[email protected]>

---
Applicable to linux-next-20160321.

Changes to v1:
- Turn commit message's impact part into a non-impact part.

 drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/radeon/evergreen.c +4 -3

index 76c4bdf..6360717 100644
@@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
                        break;
                }
                radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
-               enabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       enabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_enable_hpd(rdev, enabled);
 }

@@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
                default:
                        break;
                }
-               disabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       disabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_disable_hpd(rdev, disabled);
 }
--

2.7.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 42 lines · Nicolai Stange

The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id enum transform 1:1 into bit positions within the 'enabled' bitset as assembled by evergreen_hpd_init():

  enabled |= 1 << radeon_connector->hpd.hpd;

However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN reports

  UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
  shift exponent 255 is too large for 32-bit type 'int'
  [...]
  Call Trace:
   [<ffffffff818c4d35>] dump_stack+0xbc/0x117
   [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
   [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
   [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
   [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
   [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
   [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
   [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
   [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
   [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
   [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
   [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
   [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
   [<ffffffff814b83f0>] ? kfree+0x220/0x370
   [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
   [...]
  =====================================================================
  radeon 0000:01:00.0: No connectors reported connected with modes

At least on x86, there should be no user-visible impact as there

1 << 0xff == 1 << (0xff & 31) == 1 << 31

holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.

All of the above applies analogously to evergreen_hpd_fini().

Quoted 5 lines

Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset in evergreen_hpd_fini() respectively.

Signed-off-by Nicolai Stange <[email protected]>

Can you please fix all instances of this? This same pattern is repeated in various asic families.

Quoted 41 lines

---
Applicable to linux-next-20160321.

Changes to v1:
- Turn commit message's impact part into a non-impact part.

 drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/radeon/evergreen.c +4 -3

index 76c4bdf..6360717 100644
@@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
                        break;
                }
                radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
-               enabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       enabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_enable_hpd(rdev, enabled);
 }

@@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
                default:
                        break;
                }
-               disabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       disabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_disable_hpd(rdev, disabled);
 }
--

2.7.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 43 lines · Alex Deucher

Quoted 42 lines · Nicolai Stange

The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id enum transform 1:1 into bit positions within the 'enabled' bitset as assembled by evergreen_hpd_init():

  enabled |= 1 << radeon_connector->hpd.hpd;

However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN reports

  UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
  shift exponent 255 is too large for 32-bit type 'int'
  [...]
  Call Trace:
   [<ffffffff818c4d35>] dump_stack+0xbc/0x117
   [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
   [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
   [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
   [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
   [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
   [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
   [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
   [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
   [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
   [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
   [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
   [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
   [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
   [<ffffffff814b83f0>] ? kfree+0x220/0x370
   [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
   [...]
  =====================================================================
  radeon 0000:01:00.0: No connectors reported connected with modes

At least on x86, there should be no user-visible impact as there

1 << 0xff == 1 << (0xff & 31) == 1 << 31

holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.

All of the above applies analogously to evergreen_hpd_fini().

Quoted 8 lines

Quoted 5 lines

Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset in evergreen_hpd_fini() respectively.

Signed-off-by Nicolai Stange <[email protected]>

Can you please fix all instances of this? This same pattern is repeated in various asic families.

Sure.

The places to fix are:

$ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/
drivers/gpu/drm/radeon/evergreen.c:1868:                        enabled |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/evergreen.c:1912:                        disabled |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r100.c:595:              enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r100.c:617:              disable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r600.c:1005:             enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r600.c:1058:             disable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/rs600.c:416:             enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/rs600.c:442:             disable |= 1 << radeon_connector->hpd.hpd;

May I throw all of those into one single patch?

Thanks,

Nicolai

Quoted 37 lines

Quoted 37 lines

---
Applicable to linux-next-20160321.

Changes to v1:
- Turn commit message's impact part into a non-impact part.

 drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/radeon/evergreen.c +4 -3

index 76c4bdf..6360717 100644
@@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
                        break;
                }
                radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
-               enabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       enabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_enable_hpd(rdev, enabled);
 }

@@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
                default:
                        break;
                }
-               disabled |= 1 << radeon_connector->hpd.hpd;
+               if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+                       disabled |= 1 << radeon_connector->hpd.hpd;
        }
        radeon_irq_kms_disable_hpd(rdev, disabled);
 }
--

2.7.3

Quoted 4 lines

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel