linux-kernel - Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 18 Jun 2024 13:15:45 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jayesh Choudhary <j-choudhary@...com>
Cc: dianders@...omium.org, andrzej.hajda@...el.com, neil.armstrong@...aro.org, 
	rfoss@...nel.org, Laurent.pinchart@...asonboard.com, mripard@...nel.org, 
	linux-kernel@...r.kernel.org, jonas@...boo.se, jernej.skrabec@...il.com, 
	maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de, airlied@...il.com, 
	daniel@...ll.ch, spanda@...eaurora.org, a-bhatia1@...com, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook
 for the bridge

On Tue, 18 Jun 2024 at 12:56, Jayesh Choudhary <j-choudhary@...com> wrote:
>
> Hello Dmitry,
>
> Thanks for the review.
>
> On 18/06/24 14:29, Dmitry Baryshkov wrote:
> > On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
> >> Add the atomic_check hook to ensure that the parameters are within the
> >> valid range.
> >> As of now, dsi clock freqency is being calculated in bridge_enable but
> >> this needs to be checked in atomic_check which is called before
> >> bridge_enable so move this calculation to atomic_check and write the
> >> register value in bridge_enable as it is.
> >>
> >> For now, add mode clock check for the max resolution supported by the
> >> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
> >> check for SN_DSIA_CLK_FREQ_REG.
> >> According to the datasheet[0], the minimum value for that reg is 0x08
> >> and the maximum value is 0x96. So add check for that.
> >>
> >> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
> >>
> >> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
> >> ---
> >>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
> >>   1 file changed, 46 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> index 84698a0b27a8..d13b42d7c512 100644
> >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> @@ -113,6 +113,20 @@
> >>
>
> [...]
>
> >>
> >> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
> >> +                                 struct drm_bridge_state *bridge_state,
> >> +                                 struct drm_crtc_state *crtc_state,
> >> +                                 struct drm_connector_state *conn_state)
> >> +{
> >> +    struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >> +    struct drm_display_mode *mode = &crtc_state->mode;
> >> +    unsigned int bit_rate_mhz, clk_freq_mhz;
> >> +
> >> +    /* Pixel clock check */
> >> +    if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
> >> +            return -EINVAL;
> >> +
> >> +    bit_rate_mhz = (mode->clock / 1000) *
> >> +                    mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> >> +    clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> >> +
> >> +    /* for each increment in dsi_clk_range, frequency increases by 5MHz */
> >> +    pdata->dsi_clk_range = (MIN_DSI_CLK_FREQ_MHZ / 5) +
> >> +            (((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
> >
> > atomic_check might be called several times, it might be called to test
> > the state. As such, it should not modify anything outside of the
> > state variables.
> >
>
> If not in atomic_check, then where should I move this calculation and check?
> mode_valid with returning MODE_BAD in case of failure?

I didn't write that it's the wrong place for math. I wrote that you
should not be modifying global structure.

So you have to subclass drm_bridge_state for the driver and store the
value there. Or just add a helper function and call it from
atomic_check(), mode_valid() and set_dsi_rate(). It really looks like
a simpler solution here.

Note, there is a significant difference between mode_valid() and
atomic_check(). The former function is used for filtering the modes,
while the latter one is used for actually checking that the parameters
passed from the client are correct.

>
> I had to move it from bridge_enable based on the comments on v1:
> https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801
>
> Warm Regards,
> Jayesh
>
> [...]



-- 
With best wishes
Dmitry