From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roland Scheidegger Date: Tue, 14 Jul 2020 01:39:13 +0000 Subject: Re: [PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests Message-Id: <77f0761a-11e6-e321-2245-700258d54924@vmware.com> List-Id: References: <20200626103959.GC314359@mwanda> In-Reply-To: <20200626103959.GC314359@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter , VMware Graphics Cc: David Airlie , kernel-janitors@vger.kernel.org, dri-devel@lists.freedesktop.org Am 26.06.20 um 12:39 schrieb Dan Carpenter: > These if statements are supposed to be true if we ended the > list_for_each_entry() loops without hitting a break statement but they > don't work. > > In the first loop, we increment "i" after the "if (i = unit)" condition > so we don't necessarily know that "i" is not equal to unit at the end of > the loop. So, if I understand this right, this would only really be a problem if there's no list entries at all, right? That is i = unit = 0. Not sure if that can actually happen, but in any case the fix looks correct. > > In the second loop we exit when mode is not pointing to a valid > drm_display_mode struct so it doesn't make sense to check "mode->type". Looks good to me too, condition order seems fine to me as well, though I wouldn't particularly care. Applied to vmwgfx-next as well, thanks. Roland > > Fixes: a278724aa23c ("drm/vmwgfx: Implement fbdev on kms v2") > Signed-off-by: Dan Carpenter > --- > I reversed the second condition as well, just because I was copy and > pasting the exit condition. Plus I always feel like error handling is > better than success handling. If anyone feel strongly, then I can send > a v2. > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 3c97654b5a43..44168a7d7b44 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -2576,7 +2576,7 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv, > ++i; > } > > - if (i != unit) { > + if (&con->head = &dev_priv->dev->mode_config.connector_list) { > DRM_ERROR("Could not find initial display unit.\n"); > ret = -EINVAL; > goto out_unlock; > @@ -2600,13 +2600,13 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv, > break; > } > > - if (mode->type & DRM_MODE_TYPE_PREFERRED) > - *p_mode = mode; > - else { > + if (&mode->head = &con->modes) { > WARN_ONCE(true, "Could not find initial preferred mode.\n"); > *p_mode = list_first_entry(&con->modes, > struct drm_display_mode, > head); > + } else { > + *p_mode = mode; > } > > out_unlock: > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0E75AC433E0 for ; Tue, 14 Jul 2020 01:39:18 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E10292192A for ; Tue, 14 Jul 2020 01:39:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E10292192A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=vmware.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2E5F06E869; Tue, 14 Jul 2020 01:39:17 +0000 (UTC) Received: from EX13-EDG-OU-002.vmware.com (ex13-edg-ou-002.vmware.com [208.91.0.190]) by gabe.freedesktop.org (Postfix) with ESMTPS id 033356E869 for ; Tue, 14 Jul 2020 01:39:15 +0000 (UTC) Received: from sc9-mailhost3.vmware.com (10.113.161.73) by EX13-EDG-OU-002.vmware.com (10.113.208.156) with Microsoft SMTP Server id 15.0.1156.6; Mon, 13 Jul 2020 18:39:13 -0700 Received: from [0.0.0.0] (oddjob.vmware.com [10.253.4.32]) by sc9-mailhost3.vmware.com (Postfix) with ESMTP id 0CAC540978; Mon, 13 Jul 2020 18:39:13 -0700 (PDT) Subject: Re: [PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests To: Dan Carpenter , VMware Graphics References: <20200626103959.GC314359@mwanda> From: Roland Scheidegger Message-ID: <77f0761a-11e6-e321-2245-700258d54924@vmware.com> Date: Tue, 14 Jul 2020 03:39:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20200626103959.GC314359@mwanda> Content-Language: de-DE Received-SPF: None (EX13-EDG-OU-002.vmware.com: sroland@vmware.com does not designate permitted sender hosts) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , kernel-janitors@vger.kernel.org, dri-devel@lists.freedesktop.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Am 26.06.20 um 12:39 schrieb Dan Carpenter: > These if statements are supposed to be true if we ended the > list_for_each_entry() loops without hitting a break statement but they > don't work. > > In the first loop, we increment "i" after the "if (i == unit)" condition > so we don't necessarily know that "i" is not equal to unit at the end of > the loop. So, if I understand this right, this would only really be a problem if there's no list entries at all, right? That is i == unit == 0. Not sure if that can actually happen, but in any case the fix looks correct. > > In the second loop we exit when mode is not pointing to a valid > drm_display_mode struct so it doesn't make sense to check "mode->type". Looks good to me too, condition order seems fine to me as well, though I wouldn't particularly care. Applied to vmwgfx-next as well, thanks. Roland > > Fixes: a278724aa23c ("drm/vmwgfx: Implement fbdev on kms v2") > Signed-off-by: Dan Carpenter > --- > I reversed the second condition as well, just because I was copy and > pasting the exit condition. Plus I always feel like error handling is > better than success handling. If anyone feel strongly, then I can send > a v2. > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 3c97654b5a43..44168a7d7b44 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -2576,7 +2576,7 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv, > ++i; > } > > - if (i != unit) { > + if (&con->head == &dev_priv->dev->mode_config.connector_list) { > DRM_ERROR("Could not find initial display unit.\n"); > ret = -EINVAL; > goto out_unlock; > @@ -2600,13 +2600,13 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv, > break; > } > > - if (mode->type & DRM_MODE_TYPE_PREFERRED) > - *p_mode = mode; > - else { > + if (&mode->head == &con->modes) { > WARN_ONCE(true, "Could not find initial preferred mode.\n"); > *p_mode = list_first_entry(&con->modes, > struct drm_display_mode, > head); > + } else { > + *p_mode = mode; > } > > out_unlock: > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel