linux-kernel - Re: [PATCH v2 2/2] drm/bridge: adv7511: get edid in hpd

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAN6tsi4wxfTdG+qMr2fcwkou5F1xpxeh5S7CoAUzwi4jMFP_kw@mail.gmail.com>
Date: Mon, 22 Jan 2024 16:29:31 +0100
From: Robert Foss <rfoss@...nel.org>
To: Alvin Šipraga <alvin@...s.dk>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Hans Verkuil <hverkuil-cisco@...all.nl>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH v2 2/2] drm/bridge: adv7511: get edid in hpd_work to
 update CEC phys address

On Fri, Nov 24, 2023 at 4:20 PM Alvin Šipraga <alvin@...s.dk> wrote:
>
> From: Alvin Šipraga <alsi@...g-olufsen.dk>
>
> The adv7511 driver is solely responsible for setting the physical
> address of its CEC adapter. To do this, it must read the EDID. However,
> EDID is only read when either the drm_bridge_funcs :: get_edid or
> drm_connector_helper_funcs :: get_modes ops are called. Without loss of
> generality, it cannot be assumed that these ops are called when a sink
> gets attached. Therefore there exist scenarios in which the CEC physical
> address will be invalid (f.f.f.f), rendering the CEC adapter inoperable.
>
> Address this problem by always fetching the EDID in the HPD work when we
> detect a connection. The CEC physical address is set in the process.
> This is done by moving the EDID DRM helper into an internal helper
> function so that it can be cleanly called from an earlier section of
> the code. The EDID getter has not changed in practice.
>
> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 74 ++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 5ffc5904bd59..1f1d3a440895 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -542,6 +542,36 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>         return 0;
>  }
>
> +static struct edid *__adv7511_get_edid(struct adv7511 *adv7511,
> +                                      struct drm_connector *connector)
> +{
> +       struct edid *edid;
> +
> +       /* Reading the EDID only works if the device is powered */
> +       if (!adv7511->powered) {
> +               unsigned int edid_i2c_addr =
> +                                       (adv7511->i2c_edid->addr << 1);
> +
> +               __adv7511_power_on(adv7511);
> +
> +               /* Reset the EDID_I2C_ADDR register as it might be cleared */
> +               regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +                            edid_i2c_addr);
> +       }
> +
> +       edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> +
> +       if (!adv7511->powered)
> +               __adv7511_power_off(adv7511);
> +
> +       adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> +                              drm_detect_hdmi_monitor(edid));
> +
> +       cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> +
> +       return edid;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Hotplug handling
>   */
> @@ -595,8 +625,24 @@ static void adv7511_hpd_work(struct work_struct *work)
>                 adv7511->connector.status = status;
>
>                 if (adv7511->connector.dev) {
> -                       if (status == connector_status_disconnected)
> +                       if (status == connector_status_disconnected) {
>                                 cec_phys_addr_invalidate(adv7511->cec_adap);
> +                       } else {
> +                               struct edid *edid;
> +
> +                               /*
> +                                * Get the updated EDID so that the CEC
> +                                * subsystem gets informed of any change in CEC
> +                                * address. The helper returns a newly allocated
> +                                * edid structure, so free it to prevent
> +                                * leakage.
> +                                */
> +                               edid = __adv7511_get_edid(adv7511,
> +                                                         &adv7511->connector);
> +                               if (edid)
> +                                       kfree(edid);

kfree(NULL) is safe, so the if statement can be removed.

With this fixed, feel free to add my r-b to this full series.

Reviewed-by: Robert Foss <rfoss@...nel.org>