Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     Alvin =?utf-8?Q?=C5=A0ipraga?= <alvin@pqrs.dk>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
Cc:     linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        Alvin =?utf-8?Q?=C5=A0ipraga?= <alsi@bang-olufsen.dk>
Subject: Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update
 CEC phys address
In-Reply-To: <20231014-adv7511-cec-edid-v1-1-a58ceae0b57e@bang-olufsen.dk>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20231014-adv7511-cec-edid-v1-1-a58ceae0b57e@bang-olufsen.dk>
Date:   Mon, 23 Oct 2023 14:42:56 +0300
Message-ID: <87pm15bm4f.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, 14 Oct 2023, Alvin =C5=A0ipraga <alvin@pqrs.dk> wrote:
> From: Alvin =C5=A0ipraga <alsi@bang-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.
>
> Signed-off-by: Alvin =C5=A0ipraga <alsi@bang-olufsen.dk>
> ---
> Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did
> not want to clobber git history by rearranging a bunch of functions. If
> this is the preferred approach I will happily re-spin the patch.
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/d=
rm/bridge/adv7511/adv7511_drv.c
> index 2611afd2c1c1..3d32c109963c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
>  	return false;
>  }
>=20=20
> +static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
> +				     struct drm_connector *connector);
> +
>  static void adv7511_hpd_work(struct work_struct *work)
>  {
>  	struct adv7511 *adv7511 =3D container_of(work, struct adv7511, hpd_work=
);
> @@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
>  		if (adv7511->connector.dev) {
>  			if (status =3D=3D connector_status_disconnected)
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			else
> +				adv7511_get_edid(adv7511, &adv7511->connector);

This leaks the returned EDID.

BR,
Jani.

> +
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
>  			drm_bridge_hpd_notify(&adv7511->bridge, status);
>
> ---
> base-commit: 4366faf43308bd91c59a20c43a9f853a9c3bb6e4
> change-id: 20231014-adv7511-cec-edid-ff75bd3ac929
>

--=20
Jani Nikula, Intel