From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f178.google.com (mail-lj1-f178.google.com [209.85.208.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7C87AAD5C for ; Wed, 26 Oct 2022 22:02:28 +0000 (UTC) Received: by mail-lj1-f178.google.com with SMTP id bn35so15656382ljb.5 for ; Wed, 26 Oct 2022 15:02:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=hVGfulv077UBxsCtfjWKjttGMkhjPh4/Hf9rDhRbZDsum/itTcad9RTU1pC69hYnH4 9k1YuY9UxLIiVPXb3HwL3UPoWvKhihpgwxTEZ5ch2rFIzc2KDyTz1PXgYzEOoTIAxWji 0fcw9CBPwvkD7Cu/10sUAMuFJjZp52i9aJ9hD6THhpmT2E2bInxB89sJ1qw6GH7xMcBb dkjH9kLpoPcLQlOyqka5xGNGAuLoz8+aa3SovNXgokAk4K8jhW/88cPvotghUpvcGFa3 BamcsNgnvR9P8Nsdd7EYxrLctpprENAiLlNtyDm9dgj0xUD1gsJ4EQB9bOkmGVAbO8Iy aqDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=IxAEO0MnoshVvtRgcTL5yRP/WTiOU/se2eJPYgKFx0qKba+RnKX9JFSCegRmOBh68g 2hS9KOyUhl2lbhG8ckt0fVWDSuBtbU3QHqSw+ojz5QtDVxkTdQ8Emo1zt0qPoT2OL8PA /4f6TBfqQENK/HlLOQenpIY+BP3ssaegLEsj28o0m4/9cw2stsqXj0dOPAHNDN30L9R/ 4Up0+XV9jOv1BQeb0XbjZur6EndIYfNKc3ul0t1yPTMsYtPsMtqlhT0Lvd1pe+dUqgLO aTfwVSihkJk+9M3kMs9f/fymjQPkoufUrOMGgaYVSDacLY/5Aipm1fPXnHuIF2fsNZrX r82g== X-Gm-Message-State: ACrzQf0CcCLduo3R/aBe3wk8QZPKNYaN9iUe1FHxus2iuAMI6MYhjX5z YhkoCeKHmUjDt2sKe4svAV8= X-Google-Smtp-Source: AMsMyM5+7FXwwLPlb9qIuDs7qKIPm1MbION3iWvH0DJZ+41PbaxEGy2+MUlPXsrTQOO45XWlwDc8OQ== X-Received: by 2002:a2e:9e43:0:b0:25d:d8e9:7b15 with SMTP id g3-20020a2e9e43000000b0025dd8e97b15mr17315108ljk.234.1666821746508; Wed, 26 Oct 2022 15:02:26 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:c898:de98:30b3:a07? ([2a02:a31a:a240:1700:c898:de98:30b3:a07]) by smtp.googlemail.com with ESMTPSA id m17-20020a056512359100b004ac269b7792sm987675lfr.127.2022.10.26.15.02.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Oct 2022 15:02:26 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Date: Thu, 27 Oct 2022 00:02:24 +0200 Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Content-Language: pl To: maxime@cerno.tech, Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Maxime Ripard , Joonas Lahtinen , Lyude Paul Cc: linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org, Phil Elwell , linux-arm-kernel@lists.infradead.org, nouveau@lists.freedesktop.org, Hans de Goede , Dom Cobley , dri-devel@lists.freedesktop.org, Dave Stevenson , linux-kernel@vger.kernel.org, =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Geert Uytterhoeven References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id E367AC38A2D for ; Wed, 26 Oct 2022 22:02:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 41C4D10E55E; Wed, 26 Oct 2022 22:02:34 +0000 (UTC) Received: from mail-lj1-x235.google.com (mail-lj1-x235.google.com [IPv6:2a00:1450:4864:20::235]) by gabe.freedesktop.org (Postfix) with ESMTPS id 83BC810E53B; Wed, 26 Oct 2022 22:02:28 +0000 (UTC) Received: by mail-lj1-x235.google.com with SMTP id s24so13856796ljs.11; Wed, 26 Oct 2022 15:02:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=hVGfulv077UBxsCtfjWKjttGMkhjPh4/Hf9rDhRbZDsum/itTcad9RTU1pC69hYnH4 9k1YuY9UxLIiVPXb3HwL3UPoWvKhihpgwxTEZ5ch2rFIzc2KDyTz1PXgYzEOoTIAxWji 0fcw9CBPwvkD7Cu/10sUAMuFJjZp52i9aJ9hD6THhpmT2E2bInxB89sJ1qw6GH7xMcBb dkjH9kLpoPcLQlOyqka5xGNGAuLoz8+aa3SovNXgokAk4K8jhW/88cPvotghUpvcGFa3 BamcsNgnvR9P8Nsdd7EYxrLctpprENAiLlNtyDm9dgj0xUD1gsJ4EQB9bOkmGVAbO8Iy aqDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=Pxx4pWsfeuaimfxkv3lDIlqNqz4/+Y0WwLZtuFQ9eX3jKLXZ+PgRgShsLvMvtWDB9+ cObGn/dhCytCvi5ZqlbYqyJevi7/tOpP8k7A3Qy1tDvBFkVIk8Wy4pSm2BdAGSxJHaht GTHovYVq9CYmKcQD9wsYTXKwiZQ3VctssmLRvNQRRZLJGjWcPG3YTNDlxeFgTBylyxPF 3+iRz5YMLZw55vRoevvr+Q8FlGtRU9WtYHrDpykIIQ7Q05b1S9Pb3f3lcyscIcI+3WPz JifXhmc95tF6wkZYXAoQx8CJTC2mOrKxKt/2QEnokQHR+a3kesUIZv0VX0ZAsJIxGwMw odxw== X-Gm-Message-State: ACrzQf0a9Tw5Gc19NB5FXBdaR0D3H8OigqHTRJcGeAbRCnJSBmg0GiGv sQqtThkDDAcV7tnhIlHzt1w= X-Google-Smtp-Source: AMsMyM5+7FXwwLPlb9qIuDs7qKIPm1MbION3iWvH0DJZ+41PbaxEGy2+MUlPXsrTQOO45XWlwDc8OQ== X-Received: by 2002:a2e:9e43:0:b0:25d:d8e9:7b15 with SMTP id g3-20020a2e9e43000000b0025dd8e97b15mr17315108ljk.234.1666821746508; Wed, 26 Oct 2022 15:02:26 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:c898:de98:30b3:a07? ([2a02:a31a:a240:1700:c898:de98:30b3:a07]) by smtp.googlemail.com with ESMTPSA id m17-20020a056512359100b004ac269b7792sm987675lfr.127.2022.10.26.15.02.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Oct 2022 15:02:26 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Date: Thu, 27 Oct 2022 00:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Content-Language: pl To: maxime@cerno.tech, Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Maxime Ripard , Joonas Lahtinen , Lyude Paul References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Nouveau] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper X-BeenThere: nouveau@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Nouveau development list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Dom Cobley , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Phil Elwell , Hans de Goede , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Geert Uytterhoeven , linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 5E2A3C38A2D for ; Wed, 26 Oct 2022 22:02:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CFB6310E53E; Wed, 26 Oct 2022 22:02:31 +0000 (UTC) Received: from mail-lj1-x235.google.com (mail-lj1-x235.google.com [IPv6:2a00:1450:4864:20::235]) by gabe.freedesktop.org (Postfix) with ESMTPS id 83BC810E53B; Wed, 26 Oct 2022 22:02:28 +0000 (UTC) Received: by mail-lj1-x235.google.com with SMTP id s24so13856796ljs.11; Wed, 26 Oct 2022 15:02:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=hVGfulv077UBxsCtfjWKjttGMkhjPh4/Hf9rDhRbZDsum/itTcad9RTU1pC69hYnH4 9k1YuY9UxLIiVPXb3HwL3UPoWvKhihpgwxTEZ5ch2rFIzc2KDyTz1PXgYzEOoTIAxWji 0fcw9CBPwvkD7Cu/10sUAMuFJjZp52i9aJ9hD6THhpmT2E2bInxB89sJ1qw6GH7xMcBb dkjH9kLpoPcLQlOyqka5xGNGAuLoz8+aa3SovNXgokAk4K8jhW/88cPvotghUpvcGFa3 BamcsNgnvR9P8Nsdd7EYxrLctpprENAiLlNtyDm9dgj0xUD1gsJ4EQB9bOkmGVAbO8Iy aqDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=Pxx4pWsfeuaimfxkv3lDIlqNqz4/+Y0WwLZtuFQ9eX3jKLXZ+PgRgShsLvMvtWDB9+ cObGn/dhCytCvi5ZqlbYqyJevi7/tOpP8k7A3Qy1tDvBFkVIk8Wy4pSm2BdAGSxJHaht GTHovYVq9CYmKcQD9wsYTXKwiZQ3VctssmLRvNQRRZLJGjWcPG3YTNDlxeFgTBylyxPF 3+iRz5YMLZw55vRoevvr+Q8FlGtRU9WtYHrDpykIIQ7Q05b1S9Pb3f3lcyscIcI+3WPz JifXhmc95tF6wkZYXAoQx8CJTC2mOrKxKt/2QEnokQHR+a3kesUIZv0VX0ZAsJIxGwMw odxw== X-Gm-Message-State: ACrzQf0a9Tw5Gc19NB5FXBdaR0D3H8OigqHTRJcGeAbRCnJSBmg0GiGv sQqtThkDDAcV7tnhIlHzt1w= X-Google-Smtp-Source: AMsMyM5+7FXwwLPlb9qIuDs7qKIPm1MbION3iWvH0DJZ+41PbaxEGy2+MUlPXsrTQOO45XWlwDc8OQ== X-Received: by 2002:a2e:9e43:0:b0:25d:d8e9:7b15 with SMTP id g3-20020a2e9e43000000b0025dd8e97b15mr17315108ljk.234.1666821746508; Wed, 26 Oct 2022 15:02:26 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:c898:de98:30b3:a07? ([2a02:a31a:a240:1700:c898:de98:30b3:a07]) by smtp.googlemail.com with ESMTPSA id m17-20020a056512359100b004ac269b7792sm987675lfr.127.2022.10.26.15.02.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Oct 2022 15:02:26 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Date: Thu, 27 Oct 2022 00:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Content-Language: pl To: maxime@cerno.tech, Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Maxime Ripard , Joonas Lahtinen , Lyude Paul References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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: Dom Cobley , Dave Stevenson , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Phil Elwell , Hans de Goede , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Geert Uytterhoeven , linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 4FAFDC38A2D for ; Wed, 26 Oct 2022 22:02:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3338F10E554; Wed, 26 Oct 2022 22:02:32 +0000 (UTC) Received: from mail-lj1-x235.google.com (mail-lj1-x235.google.com [IPv6:2a00:1450:4864:20::235]) by gabe.freedesktop.org (Postfix) with ESMTPS id 83BC810E53B; Wed, 26 Oct 2022 22:02:28 +0000 (UTC) Received: by mail-lj1-x235.google.com with SMTP id s24so13856796ljs.11; Wed, 26 Oct 2022 15:02:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=hVGfulv077UBxsCtfjWKjttGMkhjPh4/Hf9rDhRbZDsum/itTcad9RTU1pC69hYnH4 9k1YuY9UxLIiVPXb3HwL3UPoWvKhihpgwxTEZ5ch2rFIzc2KDyTz1PXgYzEOoTIAxWji 0fcw9CBPwvkD7Cu/10sUAMuFJjZp52i9aJ9hD6THhpmT2E2bInxB89sJ1qw6GH7xMcBb dkjH9kLpoPcLQlOyqka5xGNGAuLoz8+aa3SovNXgokAk4K8jhW/88cPvotghUpvcGFa3 BamcsNgnvR9P8Nsdd7EYxrLctpprENAiLlNtyDm9dgj0xUD1gsJ4EQB9bOkmGVAbO8Iy aqDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=Pxx4pWsfeuaimfxkv3lDIlqNqz4/+Y0WwLZtuFQ9eX3jKLXZ+PgRgShsLvMvtWDB9+ cObGn/dhCytCvi5ZqlbYqyJevi7/tOpP8k7A3Qy1tDvBFkVIk8Wy4pSm2BdAGSxJHaht GTHovYVq9CYmKcQD9wsYTXKwiZQ3VctssmLRvNQRRZLJGjWcPG3YTNDlxeFgTBylyxPF 3+iRz5YMLZw55vRoevvr+Q8FlGtRU9WtYHrDpykIIQ7Q05b1S9Pb3f3lcyscIcI+3WPz JifXhmc95tF6wkZYXAoQx8CJTC2mOrKxKt/2QEnokQHR+a3kesUIZv0VX0ZAsJIxGwMw odxw== X-Gm-Message-State: ACrzQf0a9Tw5Gc19NB5FXBdaR0D3H8OigqHTRJcGeAbRCnJSBmg0GiGv sQqtThkDDAcV7tnhIlHzt1w= X-Google-Smtp-Source: AMsMyM5+7FXwwLPlb9qIuDs7qKIPm1MbION3iWvH0DJZ+41PbaxEGy2+MUlPXsrTQOO45XWlwDc8OQ== X-Received: by 2002:a2e:9e43:0:b0:25d:d8e9:7b15 with SMTP id g3-20020a2e9e43000000b0025dd8e97b15mr17315108ljk.234.1666821746508; Wed, 26 Oct 2022 15:02:26 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:c898:de98:30b3:a07? ([2a02:a31a:a240:1700:c898:de98:30b3:a07]) by smtp.googlemail.com with ESMTPSA id m17-20020a056512359100b004ac269b7792sm987675lfr.127.2022.10.26.15.02.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Oct 2022 15:02:26 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Date: Thu, 27 Oct 2022 00:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Content-Language: pl To: maxime@cerno.tech, Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Maxime Ripard , Joonas Lahtinen , Lyude Paul References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Dom Cobley , Dave Stevenson , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Phil Elwell , Hans de Goede , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Geert Uytterhoeven , linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 340EEC38A2D for ; Wed, 26 Oct 2022 22:03:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:Cc:To:Subject: MIME-Version:Date:Message-ID:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OWP7zsaOEnQgAn2EMgOfrxKwTqT+698dlgisHDDRCPw=; b=IocKHATlcv5sZF ZsAYSBbOh3HArz6Weg+en81v4eP/P2kS6exLo7irxZk8B6wqS3ujR9jHzOp/YehHEakBe1MdzCx7r WnZXv0SjiGAmXfubwu8GBuATfqCFplDtH1zXa/UKn+33gDnaKdGm+QdWNY+0xZAqqzyar7tFndzkY T1/TYmVwCU2upbI2nkVPbMoJ7a2p1OJJKBJy1e4JEuGc2wlF7CazakvNgKeX6wx5mMmlULMoGbrsc sIH9yLXYp7rCa/yhutUI5sDB9hk9d8H9gMt0nqy6BW2+QykcJkQ+1pydWrQRwJH8NBLijMaHR41Ys 89orn9xVGLFn7K7Fsoow==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1onoTs-00B6gn-PQ; Wed, 26 Oct 2022 22:02:36 +0000 Received: from mail-lj1-x22e.google.com ([2a00:1450:4864:20::22e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1onoTo-00B6fI-Mt for linux-arm-kernel@lists.infradead.org; Wed, 26 Oct 2022 22:02:34 +0000 Received: by mail-lj1-x22e.google.com with SMTP id d3so17884967ljl.1 for ; Wed, 26 Oct 2022 15:02:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=hVGfulv077UBxsCtfjWKjttGMkhjPh4/Hf9rDhRbZDsum/itTcad9RTU1pC69hYnH4 9k1YuY9UxLIiVPXb3HwL3UPoWvKhihpgwxTEZ5ch2rFIzc2KDyTz1PXgYzEOoTIAxWji 0fcw9CBPwvkD7Cu/10sUAMuFJjZp52i9aJ9hD6THhpmT2E2bInxB89sJ1qw6GH7xMcBb dkjH9kLpoPcLQlOyqka5xGNGAuLoz8+aa3SovNXgokAk4K8jhW/88cPvotghUpvcGFa3 BamcsNgnvR9P8Nsdd7EYxrLctpprENAiLlNtyDm9dgj0xUD1gsJ4EQB9bOkmGVAbO8Iy aqDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vdgNzDxQTO6bdUosOm0eh7BG6o7NyvcQ+Csx9d4p3mU=; b=8CF+3D0vRQFpwDd/KnkI4JY+UWfRs8C/lQgo2aREnRIzmAS4neS2N9aEcbhaoSgBMs zd0qUkfC7dOo8Wgs0E3wa4LULarNEiz7VbYXhAq7MWKUdQOuHhWYSxbY5ClR//eg9TFQ juZ+5WZ1h37pkTd033MldI0w/pCFyOx5T7c2tLc6Y9aG44Pyco00lmYBu/UJcwdcPfRt FUMk63MqU5/2M8QzVhI2HIAppIvsMgbWxVBUwq/nn/akW4H/UbafId5LPNh8/Hw6J8tF bocXTnGSPaS9xQsVoNmvSmJfh360PqQdABgZEMiRaiC8nG79XQd91XtQUV4tjnIBRDUq e2Rg== X-Gm-Message-State: ACrzQf0reB/7FBNI70i0fL3gSCa0dRfE4w9dg/VasMYT0LZ/K844pctC /7/bJ2UBXqM4Ona+vg8NcE8= X-Google-Smtp-Source: AMsMyM5+7FXwwLPlb9qIuDs7qKIPm1MbION3iWvH0DJZ+41PbaxEGy2+MUlPXsrTQOO45XWlwDc8OQ== X-Received: by 2002:a2e:9e43:0:b0:25d:d8e9:7b15 with SMTP id g3-20020a2e9e43000000b0025dd8e97b15mr17315108ljk.234.1666821746508; Wed, 26 Oct 2022 15:02:26 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:c898:de98:30b3:a07? ([2a02:a31a:a240:1700:c898:de98:30b3:a07]) by smtp.googlemail.com with ESMTPSA id m17-20020a056512359100b004ac269b7792sm987675lfr.127.2022.10.26.15.02.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Oct 2022 15:02:26 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Date: Thu, 27 Oct 2022 00:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Content-Language: pl To: maxime@cerno.tech, Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Maxime Ripard , Joonas Lahtinen , Lyude Paul Cc: linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org, Phil Elwell , linux-arm-kernel@lists.infradead.org, nouveau@lists.freedesktop.org, Hans de Goede , Dom Cobley , dri-devel@lists.freedesktop.org, Dave Stevenson , linux-kernel@vger.kernel.org, =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Geert Uytterhoeven References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221026_150232_777582_CDF69931 X-CRM114-Status: GOOD ( 18.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel