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