| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120326152234.GV4014@phenom.ffwll.local>
Date: Mon, 26 Mar 2012 17:22:34 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Daniel Kurtz <djkurtz@...omium.org>
Cc: Keith Packard <keithp@...thp.com>, David Airlie <airlied@...ux.ie>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Daniel Vetter <daniel@...ll.ch>,
Chris Wilson <chris@...is-wilson.co.uk>,
Benson Leung <bleung@...omium.org>,
Yufeng Shen <miletus@...omium.org>
Subject: Re: [PATCH 06/11 v3] drm/i915/intel_i2c: refactor using
intel_gmbus_get_adapter
On Mon, Mar 26, 2012 at 10:26:45PM +0800, Daniel Kurtz wrote:
> Instead of letting other modules directly access the ->gmbus array,
> introduce a new API, intel_gmbus_get_adapter(), to lookup an i2c_adapter
> for a given gmbus pin pair identifier. This API enables later refactoring
> of the gmbus pin pair list.
>
> Note: The gmbus pin must be checked for validity before requesting the
> corresponding gmbus port. This is especially true when using a pin that
> has been read from VBIOS, as an improperly or uninitialied BIOS might
> indicate '0' for the 'disabled' pin pair, which doesn't actually exist. In
> the case of an invalid pin, the driver falls back to using a safer default
> pin pair.
>
> Signed-off-by: Daniel Kurtz <djkurtz@...omium.org>
With the cavet of the patch ordering issue, this looks ok. One comment
below.
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
> drivers/gpu/drm/i915/intel_bios.c | 4 ++--
> drivers/gpu/drm/i915/intel_crt.c | 14 ++++++++------
> drivers/gpu/drm/i915/intel_dvo.c | 6 +++---
> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++---
> drivers/gpu/drm/i915/intel_i2c.c | 8 ++++++++
> drivers/gpu/drm/i915/intel_lvds.c | 7 ++++---
> drivers/gpu/drm/i915/intel_modes.c | 3 ++-
> drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++----
> 9 files changed, 45 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b132677..64e0ae1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1346,6 +1346,13 @@ extern int i915_restore_state(struct drm_device *dev);
> /* intel_i2c.c */
> extern int intel_setup_gmbus(struct drm_device *dev);
> extern void intel_teardown_gmbus(struct drm_device *dev);
> +extern inline bool intel_gmbus_is_port_valid(unsigned port)
> +{
> + return (port >= GMBUS_PORT_SSC && port <= GMBUS_PORT_DPD);
> +}
> +
> +extern struct i2c_adapter *intel_gmbus_get_adapter(
> + struct drm_i915_private *dev_priv, unsigned port);
> extern void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed);
> extern void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit);
> extern inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index e4317da..871aa27 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -372,11 +372,11 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
> if (block_size >= sizeof(*general)) {
> int bus_pin = general->crt_ddc_gmbus_pin;
> DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin);
> - if (bus_pin >= 1 && bus_pin <= 6)
> + if (intel_gmbus_is_port_valid(bus_pin))
> dev_priv->crt_ddc_pin = bus_pin;
> } else {
> DRM_DEBUG_KMS("BDB_GD too small (%d). Invalid.\n",
> - block_size);
> + block_size);
> }
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 4d3d736..d54d232 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -278,9 +278,10 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
> if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) {
> struct edid *edid;
> bool is_digital = false;
> + struct i2c_adapter *i2c;
>
> - edid = drm_get_edid(connector,
> - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
> + i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
> + edid = drm_get_edid(connector, i2c);
> /*
> * This may be a DVI-I connector with a shared DDC
> * link between analog and digital outputs, so we
> @@ -483,15 +484,16 @@ static int intel_crt_get_modes(struct drm_connector *connector)
> struct drm_device *dev = connector->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
> + struct i2c_adapter *i2c;
>
> - ret = intel_ddc_get_modes(connector,
> - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
> + i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
> + ret = intel_ddc_get_modes(connector, i2c);
> if (ret || !IS_G4X(dev))
> return ret;
>
> /* Try to probe digital port for output in DVI-I -> VGA mode. */
> - return intel_ddc_get_modes(connector,
> - &dev_priv->gmbus[GMBUS_PORT_DPB].adapter);
> + i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);
> + return intel_ddc_get_modes(connector, i2c);
> }
>
> static int intel_crt_set_property(struct drm_connector *connector,
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 020a7d7..60ba50b9 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -243,7 +243,7 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
> * that's not the case.
> */
> intel_ddc_get_modes(connector,
> - &dev_priv->gmbus[GMBUS_PORT_DPC].adapter);
> + intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPC));
> if (!list_empty(&connector->probed_modes))
> return 1;
>
> @@ -375,7 +375,7 @@ void intel_dvo_init(struct drm_device *dev)
> * special cases, but otherwise default to what's defined
> * in the spec.
> */
> - if (dvo->gpio != 0)
> + if (intel_gmbus_is_port_valid(dvo->gpio))
> gpio = dvo->gpio;
> else if (dvo->type == INTEL_DVO_CHIP_LVDS)
> gpio = GMBUS_PORT_SSC;
> @@ -386,7 +386,7 @@ void intel_dvo_init(struct drm_device *dev)
> * It appears that everything is on GPIOE except for panels
> * on i830 laptops, which are on GPIOB (DVOA).
> */
> - i2c = &dev_priv->gmbus[gpio].adapter;
> + i2c = intel_gmbus_get_adapter(dev_priv, gpio);
>
> intel_dvo->dev = *dvo;
> if (!dvo->dev_ops->init(&intel_dvo->dev, i2c))
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index cae3e5f..1d00f61 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -334,7 +334,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> intel_hdmi->has_hdmi_sink = false;
> intel_hdmi->has_audio = false;
> edid = drm_get_edid(connector,
> - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
> + intel_gmbus_get_adapter(dev_priv,
> + intel_hdmi->ddc_bus));
>
> if (edid) {
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> @@ -367,7 +368,8 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
> */
>
> return intel_ddc_get_modes(connector,
> - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
> + intel_gmbus_get_adapter(dev_priv,
> + intel_hdmi->ddc_bus));
> }
>
> static bool
> @@ -379,7 +381,8 @@ intel_hdmi_detect_audio(struct drm_connector *connector)
> bool has_audio = false;
>
> edid = drm_get_edid(connector,
> - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
> + intel_gmbus_get_adapter(dev_priv,
> + intel_hdmi->ddc_bus));
> if (edid) {
> if (edid->input & DRM_EDID_INPUT_DIGITAL)
> has_audio = drm_detect_monitor_audio(edid);
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index aa0670e..a04f773 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -412,6 +412,14 @@ err:
> return ret;
> }
>
> +struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
> + unsigned port)
> +{
> + BUG_ON(!intel_gmbus_is_port_valid(port));
WARN_ON highly preferred, safe when it's guaranteed that the code will
later on blow up anyway (e.g. due to a NULL pointer deref).
> + /* -1 to map pin pair to gmbus index */
> + return &dev_priv->gmbus[port - 1].adapter;
> +}
> +
> void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
> {
> struct intel_gmbus *bus = to_intel_gmbus(adapter);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index c5c0973..4f92a11 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -837,8 +837,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
> child->device_type != DEVICE_TYPE_LFP)
> continue;
>
> - if (child->i2c_pin)
> - *i2c_pin = child->i2c_pin;
> + if (intel_gmbus_is_port_valid(child->i2c_pin))
> + *i2c_pin = child->i2c_pin;
>
> /* However, we cannot trust the BIOS writers to populate
> * the VBT correctly. Since LVDS requires additional
> @@ -979,7 +979,8 @@ bool intel_lvds_init(struct drm_device *dev)
> * preferred mode is the right one.
> */
> intel_lvds->edid = drm_get_edid(connector,
> - &dev_priv->gmbus[pin].adapter);
> + intel_gmbus_get_adapter(dev_priv,
> + pin));
> if (intel_lvds->edid) {
> if (drm_add_edid_modes(connector,
> intel_lvds->edid)) {
> diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> index 2978a3f..cc682a0 100644
> --- a/drivers/gpu/drm/i915/intel_modes.c
> +++ b/drivers/gpu/drm/i915/intel_modes.c
> @@ -55,7 +55,8 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus)
> }
> };
>
> - return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2;
> + return i2c_transfer(intel_gmbus_get_adapter(dev_priv, ddc_bus),
> + msgs, 2) == 2;
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 70fb275..830c8b5 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1254,7 +1254,8 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector)
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
>
> return drm_get_edid(connector,
> - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
> + intel_gmbus_get_adapter(dev_priv,
> + dev_priv->crt_ddc_pin));
> }
>
> enum drm_connector_status
> @@ -1922,12 +1923,12 @@ intel_sdvo_select_i2c_bus(struct drm_i915_private *dev_priv,
> if (mapping->initialized)
> pin = mapping->i2c_pin;
>
> - if (pin < GMBUS_NUM_PORTS) {
> - sdvo->i2c = &dev_priv->gmbus[pin].adapter;
> + if (intel_gmbus_is_port_valid(pin)) {
> + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, pin);
> intel_gmbus_set_speed(sdvo->i2c, GMBUS_RATE_1MHZ);
> intel_gmbus_force_bit(sdvo->i2c, true);
> } else {
> - sdvo->i2c = &dev_priv->gmbus[GMBUS_PORT_DPB].adapter;
> + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);
> }
> }
>
> --
> 1.7.7.3
>
--
Daniel Vetter
Mail: daniel@...ll.ch
Mobile: +41 (0)79 365 57 48
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists