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 225A2C76195 for ; Fri, 24 Mar 2023 09:30:10 +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:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=KVHPYKa5I3+gzopjs5pMUpPIEtdp3HrMq60B5elkEcY=; b=VDyO0D7lgmaiCc ETnH/tqzeGdQ8Umj5jpRj6iUgpKdsI9NcRdgdjCFcK21pTMAYkmffNBAtlqTRKWwka8aqYtjl18wy cwPECqMCjka3cuKc0DT5drcOSg4NdmBlEPJ/mSYVDxka6QwWiPEztk026Acnx9UA+zvoj/YlCg3J6 pm6je3e47py7qjsJI321ClPcsfmpIqkqgkWGxGnUy6mrunt80mgqQpJIEMDUILhjqZZDZgpcppz3i EZvfnvRyINQpD3ILSsyh2NU4HslU4Oj97pEY1Gz/KkcZu+SEjg7OOE1BtBxW2PnKCDVCf9Dl5uQGi kkrBaZ3TcnKjE6niZ7bQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pfdjV-0041SF-0l; Fri, 24 Mar 2023 09:29:13 +0000 Received: from mail-ed1-x52f.google.com ([2a00:1450:4864:20::52f]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pfdjP-0041Pb-2J for linux-arm-kernel@lists.infradead.org; Fri, 24 Mar 2023 09:29:10 +0000 Received: by mail-ed1-x52f.google.com with SMTP id r11so5302218edd.5 for ; Fri, 24 Mar 2023 02:29:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1679650143; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=RXJ1YNbcC6sxgPer8ae1FszGABSLQrMdlXHnXNm+WPE=; b=FjoPg5qeIqGeU/oBTXgxVJ9niW0CzJwZ8jIubenpszHugo9uwYCcLWH+0Jt+iU7CiZ +c4htM12XFHfXR3ZlXkKSWyN2XBvy2TrbNTLl6t1G8qv+wcPWd+XSuyAinIkQRi7BhyC 6KYNzlW+BkJAWOetzY5BXNluhWp1ZPj8LVl+lpxtlurajvmXMmS1reN8pmDmtKa/Pwg4 tZAZugGThN+niFOcz/bA9ZlGLueLKMNu+gXRT4+gcMKA3GotKbj9oWvm7LlJnXfiosit KOsnSeWqf8KG+wBCJysD+BsuqtHFOBLHKaOnwKaFfRTsoFZDgDwH6FlIxBmxEPYLYxgp 2bLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679650143; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=RXJ1YNbcC6sxgPer8ae1FszGABSLQrMdlXHnXNm+WPE=; b=JVlvB3BldqrQq6dm38OUbMtATLVEWdljCystvatFg4fVuHe2DHj/LTe3tvYcbp5mE5 MB5JD6pTdGEesECkPQ1hoVKmLLHvvgZGlIWeX/4hVpF7mILuNVshqBtiCX4ylgRTRCD8 0WQejRvnbMiJL/0Jdyum/y7iTwzUo1mDNYjN23bOaaViSGfsLtys0ddfIEi+mn/0JAiG AEArajr8e9ViYWP4t+tSAGjI0xbaiJz/0COc/WCJkCAJ06Zz/plMJFAme8Qp3KIJJYsK OIkEGS5Gek63k+20aJWCJCy/8m003yyQJIJZH/El73v/8RnUDEuuGkI9JoJCccSmANck W+Cg== X-Gm-Message-State: AAQBX9eB3zvvgzhEUn8Lcd8vR5CCDpKVtHShdqCgUpd+zkuWQ2PvSVzf vZPW6qv0b3uCszA3dglmyqsFMw== X-Google-Smtp-Source: AKy350YHIauhajDhC/4NuJPejujuV/yRpYwlD7rIGrAi2xdw0atMEnHnUWicJHGrCkmTZ2PLnlp6pA== X-Received: by 2002:a50:ef01:0:b0:4fb:de7d:b05a with SMTP id m1-20020a50ef01000000b004fbde7db05amr1764298eds.40.1679650143228; Fri, 24 Mar 2023 02:29:03 -0700 (PDT) Received: from localhost ([86.61.181.4]) by smtp.gmail.com with ESMTPSA id o2-20020a509b02000000b004faa1636758sm10376879edi.68.2023.03.24.02.29.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Mar 2023 02:29:02 -0700 (PDT) Date: Fri, 24 Mar 2023 10:29:01 +0100 From: Jiri Pirko To: Vadim Fedorenko Cc: Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Vadim Fedorenko , poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, Milena Olech , Michal Michalik Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-3-vadfed@meta.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230312022807.278528-3-vadfed@meta.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230324_022907_973663_D25DA149 X-CRM114-Status: GOOD ( 13.03 ) 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 Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote: [...] >+static int >+dpll_msg_add_pin_direction(struct sk_buff *msg, const struct dpll_pin *pin, >+ struct netlink_ext_ack *extack) >+{ >+ enum dpll_pin_direction direction; >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) { >+ if (ref && ref->ops && ref->dpll) >+ break; >+ } >+ if (!ref || !ref->ops || !ref->dpll) >+ return -ENODEV; >+ if (!ref->ops->direction_get) >+ return -EOPNOTSUPP; >+ if (ref->ops->direction_get(pin, ref->dpll, &direction, extack)) >+ return -EFAULT; >+ if (nla_put_u8(msg, DPLL_A_PIN_DIRECTION, direction)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int >+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin, >+ struct netlink_ext_ack *extack, bool dump_any_freq) >+{ >+ enum dpll_pin_freq_supp fs; >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ u32 freq; >+ >+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) { >+ if (ref && ref->ops && ref->dpll) Checking for "ref" is nonsense here, as xa_for_each fills it up for every iteration. ref->dpll is always filled. Also pointless check. Does it make sense to register with ops==NULL? I think we should forbid it and make this just xa_find(0) to get the first item in the xarray. I'm doing this in my patch, as it is dependency on some other patch I do in this area. >+ break; >+ } >+ if (!ref || !ref->ops || !ref->dpll) >+ return -ENODEV; >+ if (!ref->ops->frequency_get) >+ return -EOPNOTSUPP; >+ if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack)) >+ return -EFAULT; >+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq)) >+ return -EMSGSIZE; >+ if (!dump_any_freq) >+ return 0; >+ for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1; >+ fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) { >+ if (test_bit(fs, &pin->prop.freq_supported)) { >+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED, >+ dpll_pin_freq_value[fs])) >+ return -EMSGSIZE; >+ } >+ } >+ if (pin->prop.any_freq_min && pin->prop.any_freq_max) { >+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN, >+ pin->prop.any_freq_min)) >+ return -EMSGSIZE; >+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX, >+ pin->prop.any_freq_max)) >+ return -EMSGSIZE; >+ } >+ >+ return 0; >+} >+ [...] >+static int >+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin, >+ struct dpll_device *dpll, >+ struct netlink_ext_ack *extack) >+{ >+ struct dpll_pin_ref *ref; >+ int ret; >+ >+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id)) >+ return -EMSGSIZE; >+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description)) >+ return -EMSGSIZE; >+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type)) >+ return -EMSGSIZE; >+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities)) >+ return -EMSGSIZE; >+ ret = dpll_msg_add_pin_direction(msg, pin, extack); >+ if (ret) Why -EOPNOTSUPP here is not ok, as for the others below? >+ return ret; >+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll); >+ if (!ref) How this can happen? I don't think it could. >+ return -EFAULT; >+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ret = dpll_msg_add_pin_parents(msg, pin, extack); >+ if (ret) >+ return ret; >+ if (pin->rclk_dev_name) >+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE, >+ pin->rclk_dev_name)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int >+__dpll_cmd_pin_dump_one(struct sk_buff *msg, struct dpll_pin *pin, >+ struct netlink_ext_ack *extack, bool dump_dpll) >+{ >+ int ret; >+ >+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id)) >+ return -EMSGSIZE; >+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description)) >+ return -EMSGSIZE; >+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type)) >+ return -EMSGSIZE; >+ ret = dpll_msg_add_pin_direction(msg, pin, extack); >+ if (ret) >+ return ret; >+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ret = dpll_msg_add_pins_on_pin(msg, pin, extack); >+ if (ret) >+ return ret; >+ if (!xa_empty(&pin->dpll_refs) && dump_dpll) { How dpll refs could be empty? I don't think it is possible. Overall, whole the code has very odd habit of checking for conditions that are obviously impossible to happen. Only confuses reader as he naturally expects that the check is there for a reason. >+ ret = dpll_msg_add_pin_dplls(msg, pin, extack); >+ if (ret) >+ return ret; >+ } >+ if (pin->rclk_dev_name) >+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE, >+ pin->rclk_dev_name)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel