linux-kernel - Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode

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]

Message-ID: <419670d5-bba9-d5e3-9ad6-f7b3324099c5@linux.intel.com>
Date:   Fri, 3 Aug 2018 14:24:25 +0200
From:   Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To:     Satendra Singh Thakur <satendra.t@...sung.com>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Cc:     vineet.j@...sung.com, hemanshu.s@...sung.com, sst2005@...il.com
Subject: Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc

Op 03-08-18 om 13:43 schreef Satendra Singh Thakur:
> Following changes are done to this func:
> 1. The declaration of plane and it's assignment plane = crtc->primary
> are only used when mode_valid is set. Therefore, moved it inside
> the if(mode_valid) statement.
>
> 2. The declaration of connector and set_connectors_ptr and out_id
> are moved inside the for loop, as their scope is limited within
> that block.
>
> 3. Currently, there are 3 checks on count_connectors
> and 4 checks on mode related params (mode_valid, mode, fb).
> if (crtc_req->mode_valid) {
> if (crtc_req->count_connectors == 0 && mode) {
> if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
> if (crtc_req->count_connectors > 0) {
>
> In the modified code, there are just 1 check on mode_valid and
> 2 checks  count_connectors.
> Checks on mode and fb are not needed as these variables will
> be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
> If mode_valid is clear, mode and fb will be NULL.
> Therefore, we just check mode_valid and NOT mode or fb.
>
> 4. Moved kfree inside if statement
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@...sung.com>
>
> ---
>  
>  v1: Hi Mr Maarten, Thanks for the comments.
>      I have fixed some of them and done more modifications to the patch.
>      Please review.
Could you read the suggestions on https://patchwork.freedesktop.org/patch/241508/ ?
I'm not saying this code is incorrect, I think that if you want to improve drm then you should tackle something bigger than just function readability. :)

Cheers,
~Maarten