* [PATCH] drm/i915: Check DVO reads for errors
@ 2015-02-26 17:10 Chris Wilson
2015-02-26 17:47 ` Quentin Casasnovas
2015-02-28 22:22 ` shuang.he
0 siblings, 2 replies; 5+ messages in thread
From: Chris Wilson @ 2015-02-26 17:10 UTC (permalink / raw)
To: intel-gfx; +Cc: Quentin Casasnovas
Not all of the DVO functions were checking the return value from their
i2c routines when reading registers. This could lead to us feeding
garbage values back into the hardware, possible causing further
failures. In some cases the uninitialised stack values were being
written into the kernel log.
Quentin Casasnovas suggested the simple solution of just initialising
the output parameter to zero in all cases, but we may as well spend the
extra few moments to fix it correctly.
Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
---
drivers/gpu/drm/i915/dvo_ch7017.c | 23 ++++++++-----
drivers/gpu/drm/i915/dvo_ch7xxx.c | 36 ++++++++++----------
drivers/gpu/drm/i915/dvo_ivch.c | 64 +++++++++++++++---------------------
drivers/gpu/drm/i915/dvo_sil164.c | 49 +++++++++++++--------------
drivers/gpu/drm/i915/dvo_tfp410.c | 69 ++++++++++++++++++++++-----------------
5 files changed, 120 insertions(+), 121 deletions(-)
diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
index 86b27d1d90c2..452f0144e6a2 100644
--- a/drivers/gpu/drm/i915/dvo_ch7017.c
+++ b/drivers/gpu/drm/i915/dvo_ch7017.c
@@ -335,7 +335,8 @@ static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable)
{
uint8_t val;
- ch7017_read(dvo, CH7017_LVDS_POWER_DOWN, &val);
+ if (!ch7017_read(dvo, CH7017_LVDS_POWER_DOWN, &val))
+ return;
/* Turn off TV/VGA, and never turn it on since we don't support it. */
ch7017_write(dvo, CH7017_POWER_MANAGEMENT,
@@ -363,7 +364,8 @@ static bool ch7017_get_hw_state(struct intel_dvo_device *dvo)
{
uint8_t val;
- ch7017_read(dvo, CH7017_LVDS_POWER_DOWN, &val);
+ if (!ch7017_read(dvo, CH7017_LVDS_POWER_DOWN, &val))
+ return false;
if (val & CH7017_LVDS_POWER_DOWN_EN)
return false;
@@ -371,16 +373,20 @@ static bool ch7017_get_hw_state(struct intel_dvo_device *dvo)
return true;
}
-static void ch7017_dump_regs(struct intel_dvo_device *dvo)
+static void ch7017_dump_reg(struct intel_dvo_device *dvo,
+ int reg, const char *name)
{
uint8_t val;
-#define DUMP(reg) \
-do { \
- ch7017_read(dvo, reg, &val); \
- DRM_DEBUG_KMS(#reg ": %02x\n", val); \
-} while (0)
+ if (ch7017_read(dvo, reg, &val))
+ DRM_DEBUG_KMS("%s: %02x\n", name, val);
+ else
+ DRM_DEBUG_KMS("%s: failed\n", name);
+}
+static void ch7017_dump_regs(struct intel_dvo_device *dvo)
+{
+#define DUMP(reg) ch7017_dump_reg(dvo, reg, #reg)
DUMP(CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT);
DUMP(CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT);
DUMP(CH7017_VERTICAL_ACTIVE_LINE_OUTPUT);
@@ -390,6 +396,7 @@ do { \
DUMP(CH7017_LVDS_CONTROL_2);
DUMP(CH7017_OUTPUTS_ENABLE);
DUMP(CH7017_LVDS_POWER_DOWN);
+#undef DUMP
}
static void ch7017_destroy(struct intel_dvo_device *dvo)
diff --git a/drivers/gpu/drm/i915/dvo_ch7xxx.c b/drivers/gpu/drm/i915/dvo_ch7xxx.c
index 80449f475960..fb23b16eaf07 100644
--- a/drivers/gpu/drm/i915/dvo_ch7xxx.c
+++ b/drivers/gpu/drm/i915/dvo_ch7xxx.c
@@ -248,20 +248,20 @@ static enum drm_connector_status ch7xxx_detect(struct intel_dvo_device *dvo)
{
uint8_t cdet, orig_pm, pm;
- ch7xxx_readb(dvo, CH7xxx_PM, &orig_pm);
+ if (!ch7xxx_readb(dvo, CH7xxx_PM, &orig_pm))
+ return connector_status_disconnected;
pm = orig_pm;
pm &= ~CH7xxx_PM_FPD;
pm |= CH7xxx_PM_DVIL | CH7xxx_PM_DVIP;
+ cdet = 0;
ch7xxx_writeb(dvo, CH7xxx_PM, pm);
-
ch7xxx_readb(dvo, CH7xxx_CONNECTION_DETECT, &cdet);
-
ch7xxx_writeb(dvo, CH7xxx_PM, orig_pm);
-
if (cdet & CH7xxx_CDET_DVI)
return connector_status_connected;
+
return connector_status_disconnected;
}
@@ -300,16 +300,16 @@ static void ch7xxx_mode_set(struct intel_dvo_device *dvo,
ch7xxx_writeb(dvo, CH7xxx_TLPF, tlpf);
ch7xxx_writeb(dvo, CH7xxx_TCT, 0x00);
- ch7xxx_readb(dvo, CH7xxx_IDF, &idf);
+ if (ch7xxx_readb(dvo, CH7xxx_IDF, &idf)) {
+ idf &= ~(CH7xxx_IDF_HSP | CH7xxx_IDF_VSP);
+ if (mode->flags & DRM_MODE_FLAG_PHSYNC)
+ idf |= CH7xxx_IDF_HSP;
- idf &= ~(CH7xxx_IDF_HSP | CH7xxx_IDF_VSP);
- if (mode->flags & DRM_MODE_FLAG_PHSYNC)
- idf |= CH7xxx_IDF_HSP;
+ if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+ idf |= CH7xxx_IDF_VSP;
- if (mode->flags & DRM_MODE_FLAG_PVSYNC)
- idf |= CH7xxx_IDF_VSP;
-
- ch7xxx_writeb(dvo, CH7xxx_IDF, idf);
+ ch7xxx_writeb(dvo, CH7xxx_IDF, idf);
+ }
}
/* set the CH7xxx power state */
@@ -323,14 +323,9 @@ static void ch7xxx_dpms(struct intel_dvo_device *dvo, bool enable)
static bool ch7xxx_get_hw_state(struct intel_dvo_device *dvo)
{
- u8 val;
-
+ u8 val = 0;
ch7xxx_readb(dvo, CH7xxx_PM, &val);
-
- if (val & (CH7xxx_PM_DVIL | CH7xxx_PM_DVIP))
- return true;
- else
- return false;
+ return val & (CH7xxx_PM_DVIL | CH7xxx_PM_DVIP);
}
static void ch7xxx_dump_regs(struct intel_dvo_device *dvo)
@@ -339,8 +334,11 @@ static void ch7xxx_dump_regs(struct intel_dvo_device *dvo)
for (i = 0; i < CH7xxx_NUM_REGS; i++) {
uint8_t val;
+
if ((i % 8) == 0)
DRM_DEBUG_KMS("\n %02X: ", i);
+
+ val = 0;
ch7xxx_readb(dvo, i, &val);
DRM_DEBUG_KMS("%02X ", val);
}
diff --git a/drivers/gpu/drm/i915/dvo_ivch.c b/drivers/gpu/drm/i915/dvo_ivch.c
index 0f2587ff347c..4908b3a00cac 100644
--- a/drivers/gpu/drm/i915/dvo_ivch.c
+++ b/drivers/gpu/drm/i915/dvo_ivch.c
@@ -151,8 +151,6 @@
struct ivch_priv {
bool quiet;
-
- uint16_t width, height;
};
@@ -263,9 +261,6 @@ static bool ivch_init(struct intel_dvo_device *dvo,
goto out;
}
- ivch_read(dvo, VR20, &priv->width);
- ivch_read(dvo, VR21, &priv->height);
-
return true;
out:
@@ -372,46 +367,41 @@ static void ivch_mode_set(struct intel_dvo_device *dvo,
ivch_dump_regs(dvo);
}
-static void ivch_dump_regs(struct intel_dvo_device *dvo)
+static void ivch_dump_reg(struct intel_dvo_device *dvo,
+ int reg, const char *name)
{
uint16_t val;
- ivch_read(dvo, VR00, &val);
- DRM_DEBUG_KMS("VR00: 0x%04x\n", val);
- ivch_read(dvo, VR01, &val);
- DRM_DEBUG_KMS("VR01: 0x%04x\n", val);
- ivch_read(dvo, VR30, &val);
- DRM_DEBUG_KMS("VR30: 0x%04x\n", val);
- ivch_read(dvo, VR40, &val);
- DRM_DEBUG_KMS("VR40: 0x%04x\n", val);
+ if (ivch_read(dvo, reg, &val))
+ DRM_DEBUG_KMS("%s: 0x%04x\n", name, val);
+ else
+ DRM_DEBUG_KMS("%s: failed\n", name);
+}
+
+static void ivch_dump_regs(struct intel_dvo_device *dvo)
+{
+#define DUMP(R) ivch_dump_reg(dvo, R, #R)
+ DUMP(VR00);
+ DUMP(VR01);
+ DUMP(VR30);
+ DUMP(VR40);
/* GPIO registers */
- ivch_read(dvo, VR80, &val);
- DRM_DEBUG_KMS("VR80: 0x%04x\n", val);
- ivch_read(dvo, VR81, &val);
- DRM_DEBUG_KMS("VR81: 0x%04x\n", val);
- ivch_read(dvo, VR82, &val);
- DRM_DEBUG_KMS("VR82: 0x%04x\n", val);
- ivch_read(dvo, VR83, &val);
- DRM_DEBUG_KMS("VR83: 0x%04x\n", val);
- ivch_read(dvo, VR84, &val);
- DRM_DEBUG_KMS("VR84: 0x%04x\n", val);
- ivch_read(dvo, VR85, &val);
- DRM_DEBUG_KMS("VR85: 0x%04x\n", val);
- ivch_read(dvo, VR86, &val);
- DRM_DEBUG_KMS("VR86: 0x%04x\n", val);
- ivch_read(dvo, VR87, &val);
- DRM_DEBUG_KMS("VR87: 0x%04x\n", val);
- ivch_read(dvo, VR88, &val);
- DRM_DEBUG_KMS("VR88: 0x%04x\n", val);
+ DUMP(VR80);
+ DUMP(VR81);
+ DUMP(VR82);
+ DUMP(VR83);
+ DUMP(VR84);
+ DUMP(VR85);
+ DUMP(VR86);
+ DUMP(VR87);
+ DUMP(VR88);
/* Scratch register 0 - AIM Panel type */
- ivch_read(dvo, VR8E, &val);
- DRM_DEBUG_KMS("VR8E: 0x%04x\n", val);
-
+ DUMP(VR8E);
/* Scratch register 1 - Status register */
- ivch_read(dvo, VR8F, &val);
- DRM_DEBUG_KMS("VR8F: 0x%04x\n", val);
+ DUMP(VR8F);
+#undef DUMP
}
static void ivch_destroy(struct intel_dvo_device *dvo)
diff --git a/drivers/gpu/drm/i915/dvo_sil164.c b/drivers/gpu/drm/i915/dvo_sil164.c
index fa0114967076..23aac3dd5883 100644
--- a/drivers/gpu/drm/i915/dvo_sil164.c
+++ b/drivers/gpu/drm/i915/dvo_sil164.c
@@ -134,7 +134,7 @@ static bool sil164_init(struct intel_dvo_device *dvo,
{
/* this will detect the SIL164 chip on the specified i2c bus */
struct sil164_priv *sil;
- unsigned char ch;
+ uint8_t ch;
sil = kzalloc(sizeof(struct sil164_priv), GFP_KERNEL);
if (sil == NULL)
@@ -175,8 +175,8 @@ static enum drm_connector_status sil164_detect(struct intel_dvo_device *dvo)
{
uint8_t reg9;
+ reg9 = 0;
sil164_readb(dvo, SIL164_REG9, ®9);
-
if (reg9 & SIL164_9_HTPLG)
return connector_status_connected;
else
@@ -210,11 +210,9 @@ static void sil164_mode_set(struct intel_dvo_device *dvo,
/* set the SIL164 power state */
static void sil164_dpms(struct intel_dvo_device *dvo, bool enable)
{
- int ret;
- unsigned char ch;
+ uint8_t ch;
- ret = sil164_readb(dvo, SIL164_REG8, &ch);
- if (ret == false)
+ if (!sil164_readb(dvo, SIL164_REG8, &ch))
return;
if (enable)
@@ -223,38 +221,35 @@ static void sil164_dpms(struct intel_dvo_device *dvo, bool enable)
ch &= ~SIL164_8_PD;
sil164_writeb(dvo, SIL164_REG8, ch);
- return;
}
static bool sil164_get_hw_state(struct intel_dvo_device *dvo)
{
- int ret;
- unsigned char ch;
+ uint8_t ch = 0;
+ sil164_readb(dvo, SIL164_REG8, &ch);
+ return ch & SIL164_8_PD;
+}
- ret = sil164_readb(dvo, SIL164_REG8, &ch);
- if (ret == false)
- return false;
+static void sil164_dump_reg(struct intel_dvo_device *dvo,
+ int reg, const char *name)
+{
+ uint8_t val;
- if (ch & SIL164_8_PD)
- return true;
+ if (sil164_readb(dvo, reg, &val))
+ DRM_DEBUG_KMS("%s: 0x%02x\n", name, val);
else
- return false;
+ DRM_DEBUG_KMS("%s: failed\n", name);
}
static void sil164_dump_regs(struct intel_dvo_device *dvo)
{
- uint8_t val;
-
- sil164_readb(dvo, SIL164_FREQ_LO, &val);
- DRM_DEBUG_KMS("SIL164_FREQ_LO: 0x%02x\n", val);
- sil164_readb(dvo, SIL164_FREQ_HI, &val);
- DRM_DEBUG_KMS("SIL164_FREQ_HI: 0x%02x\n", val);
- sil164_readb(dvo, SIL164_REG8, &val);
- DRM_DEBUG_KMS("SIL164_REG8: 0x%02x\n", val);
- sil164_readb(dvo, SIL164_REG9, &val);
- DRM_DEBUG_KMS("SIL164_REG9: 0x%02x\n", val);
- sil164_readb(dvo, SIL164_REGC, &val);
- DRM_DEBUG_KMS("SIL164_REGC: 0x%02x\n", val);
+#define DUMP(R) sil164_dump_reg(dvo, R, #R)
+ DUMP(SIL164_FREQ_LO);
+ DUMP(SIL164_FREQ_HI);
+ DUMP(SIL164_REG8);
+ DUMP(SIL164_REG9);
+ DUMP(SIL164_REGC);
+#undef DUMP
}
static void sil164_destroy(struct intel_dvo_device *dvo)
diff --git a/drivers/gpu/drm/i915/dvo_tfp410.c b/drivers/gpu/drm/i915/dvo_tfp410.c
index 7853719a0e81..c24e5a5b320f 100644
--- a/drivers/gpu/drm/i915/dvo_tfp410.c
+++ b/drivers/gpu/drm/i915/dvo_tfp410.c
@@ -262,38 +262,47 @@ static bool tfp410_get_hw_state(struct intel_dvo_device *dvo)
return false;
}
+static void tfp410_dump_reg8(struct intel_dvo_device *dvo,
+ int reg, const char *name)
+{
+ uint8_t val;
+
+ if (tfp410_readb(dvo, reg, &val))
+ DRM_DEBUG_KMS("%s: 0x%02X\n", name, val);
+ else
+ DRM_DEBUG_KMS("%s: failed\n", name);
+}
+
+static void tfp410_dump_reg16(struct intel_dvo_device *dvo,
+ int reg1, int reg2, const char *name)
+{
+ uint8_t val[2];
+
+ if (tfp410_readb(dvo, reg1, &val[0]) &&
+ tfp410_readb(dvo, reg2, &val[1]))
+ DRM_DEBUG_KMS("%s: 0x%02X%02X\n", name, val[1], val[0]);
+ else
+ DRM_DEBUG_KMS("%s: failed\n", name);
+}
+
static void tfp410_dump_regs(struct intel_dvo_device *dvo)
{
- uint8_t val, val2;
-
- tfp410_readb(dvo, TFP410_REV, &val);
- DRM_DEBUG_KMS("TFP410_REV: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_CTL_1, &val);
- DRM_DEBUG_KMS("TFP410_CTL1: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_CTL_2, &val);
- DRM_DEBUG_KMS("TFP410_CTL2: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_CTL_3, &val);
- DRM_DEBUG_KMS("TFP410_CTL3: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_USERCFG, &val);
- DRM_DEBUG_KMS("TFP410_USERCFG: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_DE_DLY, &val);
- DRM_DEBUG_KMS("TFP410_DE_DLY: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_DE_CTL, &val);
- DRM_DEBUG_KMS("TFP410_DE_CTL: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_DE_TOP, &val);
- DRM_DEBUG_KMS("TFP410_DE_TOP: 0x%02X\n", val);
- tfp410_readb(dvo, TFP410_DE_CNT_LO, &val);
- tfp410_readb(dvo, TFP410_DE_CNT_HI, &val2);
- DRM_DEBUG_KMS("TFP410_DE_CNT: 0x%02X%02X\n", val2, val);
- tfp410_readb(dvo, TFP410_DE_LIN_LO, &val);
- tfp410_readb(dvo, TFP410_DE_LIN_HI, &val2);
- DRM_DEBUG_KMS("TFP410_DE_LIN: 0x%02X%02X\n", val2, val);
- tfp410_readb(dvo, TFP410_H_RES_LO, &val);
- tfp410_readb(dvo, TFP410_H_RES_HI, &val2);
- DRM_DEBUG_KMS("TFP410_H_RES: 0x%02X%02X\n", val2, val);
- tfp410_readb(dvo, TFP410_V_RES_LO, &val);
- tfp410_readb(dvo, TFP410_V_RES_HI, &val2);
- DRM_DEBUG_KMS("TFP410_V_RES: 0x%02X%02X\n", val2, val);
+#define DUMP8(R) tfp410_dump_reg8(dvo, R, #R)
+#define DUMP16(R1, R2) tfp410_dump_reg16(dvo, R1, R2, #R1)
+ DUMP8(TFP410_REV);
+ DUMP8(TFP410_CTL_1);
+ DUMP8(TFP410_CTL_2);
+ DUMP8(TFP410_CTL_3);
+ DUMP8(TFP410_USERCFG);
+ DUMP8(TFP410_DE_DLY);
+ DUMP8(TFP410_DE_CTL);
+ DUMP8(TFP410_DE_TOP);
+ DUMP16(TFP410_DE_CNT_LO, TFP410_DE_CNT_HI);
+ DUMP16(TFP410_DE_LIN_LO, TFP410_DE_LIN_HI);
+ DUMP16(TFP410_H_RES_LO, TFP410_H_RES_HI);
+ DUMP16(TFP410_V_RES_LO, TFP410_V_RES_HI);
+#undef DUMP16
+#undef DUMP8
}
static void tfp410_destroy(struct intel_dvo_device *dvo)
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Check DVO reads for errors
2015-02-26 17:10 [PATCH] drm/i915: Check DVO reads for errors Chris Wilson
@ 2015-02-26 17:47 ` Quentin Casasnovas
2015-02-26 22:09 ` Chris Wilson
2015-02-28 22:22 ` shuang.he
1 sibling, 1 reply; 5+ messages in thread
From: Quentin Casasnovas @ 2015-02-26 17:47 UTC (permalink / raw)
To: Chris Wilson; +Cc: Quentin Casasnovas, intel-gfx
On Thu, Feb 26, 2015 at 05:10:17PM +0000, Chris Wilson wrote:
> Not all of the DVO functions were checking the return value from their
> i2c routines when reading registers. This could lead to us feeding
> garbage values back into the hardware, possible causing further
> failures. In some cases the uninitialised stack values were being
> written into the kernel log.
>
> Quentin Casasnovas suggested the simple solution of just initialising
> the output parameter to zero in all cases, but we may as well spend the
> extra few moments to fix it correctly.
I'm not sure your patch would be -stable material mainly because of the
diffstat. Given the security implications, I would still rather have my
patch merged first so it can easily be back-ported to -stable and distro
kernels easily, and then have your patch on top when it gets properly
reviewed. Especially since your patch looks like it's doing other
not strictly related stuffs like these:
> --- a/drivers/gpu/drm/i915/dvo_ivch.c
> +++ b/drivers/gpu/drm/i915/dvo_ivch.c
> @@ -151,8 +151,6 @@
> struct ivch_priv {
> bool quiet;
> -
> - uint16_t width, height;
> };
>
>
> @@ -263,9 +261,6 @@ static bool ivch_init(struct intel_dvo_device *dvo,
> goto out;
> }
>
> - ivch_read(dvo, VR20, &priv->width);
> - ivch_read(dvo, VR21, &priv->height);
> -
> return true;
So again, I think my fix as a start would be preferable since it's quite
small and easily reviewable.
Quentin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Check DVO reads for errors
2015-02-26 17:47 ` Quentin Casasnovas
@ 2015-02-26 22:09 ` Chris Wilson
2015-02-27 8:42 ` Quentin Casasnovas
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2015-02-26 22:09 UTC (permalink / raw)
To: Quentin Casasnovas; +Cc: intel-gfx
On Thu, Feb 26, 2015 at 06:47:14PM +0100, Quentin Casasnovas wrote:
> On Thu, Feb 26, 2015 at 05:10:17PM +0000, Chris Wilson wrote:
> > Not all of the DVO functions were checking the return value from their
> > i2c routines when reading registers. This could lead to us feeding
> > garbage values back into the hardware, possible causing further
> > failures. In some cases the uninitialised stack values were being
> > written into the kernel log.
> >
> > Quentin Casasnovas suggested the simple solution of just initialising
> > the output parameter to zero in all cases, but we may as well spend the
> > extra few moments to fix it correctly.
>
> I'm not sure your patch would be -stable material mainly because of the
> diffstat. Given the security implications, I would still rather have my
> patch merged first so it can easily be back-ported to -stable and distro
> kernels easily, and then have your patch on top when it gets properly
> reviewed. Especially since your patch looks like it's doing other
> not strictly related stuffs like these:
I don't agree that your patch is stable material since it is not
obviously correct (it potentially changes values written to hw), hasn't
been tested and doesn't qualify as a "real bug that impacts people".
To fix the security concerns you expressed, you could have equally
removed the DRM_DEBUG_KMS().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Check DVO reads for errors
2015-02-26 22:09 ` Chris Wilson
@ 2015-02-27 8:42 ` Quentin Casasnovas
0 siblings, 0 replies; 5+ messages in thread
From: Quentin Casasnovas @ 2015-02-27 8:42 UTC (permalink / raw)
To: Chris Wilson, Quentin Casasnovas, intel-gfx
On Thu, Feb 26, 2015 at 10:09:49PM +0000, Chris Wilson wrote:
> On Thu, Feb 26, 2015 at 06:47:14PM +0100, Quentin Casasnovas wrote:
> > On Thu, Feb 26, 2015 at 05:10:17PM +0000, Chris Wilson wrote:
> > > Not all of the DVO functions were checking the return value from their
> > > i2c routines when reading registers. This could lead to us feeding
> > > garbage values back into the hardware, possible causing further
> > > failures. In some cases the uninitialised stack values were being
> > > written into the kernel log.
> > >
> > > Quentin Casasnovas suggested the simple solution of just initialising
> > > the output parameter to zero in all cases, but we may as well spend the
> > > extra few moments to fix it correctly.
> >
> > I'm not sure your patch would be -stable material mainly because of the
> > diffstat. Given the security implications, I would still rather have my
> > patch merged first so it can easily be back-ported to -stable and distro
> > kernels easily, and then have your patch on top when it gets properly
> > reviewed. Especially since your patch looks like it's doing other
> > not strictly related stuffs like these:
>
> I don't agree that your patch is stable material since it is not
> obviously correct (it potentially changes values written to hw), hasn't
> been tested and doesn't qualify as a "real bug that impacts people".
>
I must admit I'm not quite sure what you mean by saying it potentially
changes values written to the hardware. IIRC, in the current code, we
might be writting garbage values from the stack to the hardware (so hard to
tell in that case what is it we are writting..), and use that junk to test
some flags/print some debug output. I thought my patch was helping in that
matter by at least not using/printing that junk.
> To fix the security concerns you expressed, you could have equally
> removed the DRM_DEBUG_KMS().
Agreed, though we'd still be using some garbage values in some conditional
expressions and potentially write them back to the hardware. But yeah I'm
no expert so up to you really, I just thought your patch was quite big and
still maybe not complete since it does check every *read() return values -
we might as well take my approach as a first baby step and you can then go
ahead and properly fix the callers to check for the return value?
Not saying this is critical material anyway, just that the code looked
wrong so I tried a minimal fix so it could be easily backported.
Quentin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Check DVO reads for errors
2015-02-26 17:10 [PATCH] drm/i915: Check DVO reads for errors Chris Wilson
2015-02-26 17:47 ` Quentin Casasnovas
@ 2015-02-28 22:22 ` shuang.he
1 sibling, 0 replies; 5+ messages in thread
From: shuang.he @ 2015-02-28 22:22 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, chris
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5843
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -1 282/282 281/282
ILK 308/308 308/308
SNB 326/326 326/326
IVB 379/379 379/379
BYT 294/294 294/294
HSW 387/387 387/387
BDW -1 316/316 315/316
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gem_userptr_blits_minor-normal-sync PASS(3) DMESG_WARN(1)PASS(1)
*BDW igt_gem_gtt_hog PASS(15) DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-02-28 22:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-26 17:10 [PATCH] drm/i915: Check DVO reads for errors Chris Wilson
2015-02-26 17:47 ` Quentin Casasnovas
2015-02-26 22:09 ` Chris Wilson
2015-02-27 8:42 ` Quentin Casasnovas
2015-02-28 22:22 ` shuang.he
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox