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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1F1BAC6FD20 for ; Fri, 24 Mar 2023 09:29:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232159AbjCXJ3Z (ORCPT ); Fri, 24 Mar 2023 05:29:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35150 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231503AbjCXJ3Q (ORCPT ); Fri, 24 Mar 2023 05:29:16 -0400 Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E8C61233D3 for ; Fri, 24 Mar 2023 02:29:04 -0700 (PDT) Received: by mail-ed1-x531.google.com with SMTP id t10so5180762edd.12 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=m9T1M+Jc2O/P+fmCZwikv5ZduaeDA5kubd6VQVgbzC2zjSA/XueX4kpm22iV8h3Icz XnYkowqGGdGaZOn7n21uZuEKrzNLaVXZy9w0yvdUgSmdOLu4+0rNz2g6DAEVV9QV4bwP 1hVlLrfle8jzN9C/rsKWOnYtWdNeS3utv3VXyAflxCx38md0gx06MSpn1sbXru+S6/We +rCRod997tA3xOycYm9lhLYZZXRSF5gzcngMnNd+bdHbGfflH/7unFK8D/TAe0x1hFz7 CQhLvFK+v9xG081ExvuMxPqtQ9mZ5QQ7q1FIB/cxhQifiIeHKL7u22x2JzVpec4FfhsO +kpQ== X-Gm-Message-State: AAQBX9dTcfJT+4NZybeZCkLmOnNqO+bwO9g/9ptRXjB9ETnd+kfh7DNi 8MDgxvMW+aoR2NYUtVIIMriYwA== 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-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230312022807.278528-3-vadfed@meta.com> Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.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; >+} >+