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 5D4B9EB64DD for ; Mon, 7 Aug 2023 07:08:18 +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=Q3RY8MXFtQUCuc17cfWuS1otzIhIqAteYwcGpXCFf28=; b=I6LzNR0j+uRBp3 cdUeau+vPUpsTPV5eM4UcVSQzPalUfHWeqoOpGwq4r7ncapXE/y/dvRIU1I51SlTOopGW3kgukfby qjmYRNektD8/UrRfWoiKuVFjzXsjRtZk+YTVW7i+vf1wSMgS6jGfjRV7uAYuclmaRXt0yiAWBIe+g MDvQE6CQbSvB4Qi3zWbBAFtFskoyg2OLODzSWKBm2SCA14la3V1VAVEFGnMCP8EEa94q4i9BYKyFQ XMgMPhJ2Di0c2t6E6oyUuoChVEXOOJTfnBA9yi047jZ4EpVY1P5X0skIQQk8JX9LAW8a/8sHVR5Sd fBmgQjTsM3qijVrTKtKg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qSuLI-00GKHR-2R; Mon, 07 Aug 2023 07:07:52 +0000 Received: from mail-lf1-x135.google.com ([2a00:1450:4864:20::135]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qSuLD-00GKGj-04 for linux-arm-kernel@lists.infradead.org; Mon, 07 Aug 2023 07:07:51 +0000 Received: by mail-lf1-x135.google.com with SMTP id 2adb3069b0e04-4fe4762173bso6725206e87.3 for ; Mon, 07 Aug 2023 00:07:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20221208.gappssmtp.com; s=20221208; t=1691392064; x=1691996864; 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=cWWnXKOt1mBeIEynylWCqyYZNRdvBS5hxgZ4awLikYQ=; b=ZP2rNYEzKbJiw4/twCr06gCfoDpq1kXEI6dvZvET6awgzhG2NFhu6iJh6DPYZOiSd+ G65bI+0KemUsir2VCkGUiYsUnCrFBCQU7G9OO6XRwMZv+uTUITopgzAEseibWMrusxmw 0H2daFCmx0/bKK1kZmVFQO8Lse9Ru11dNVj4bIxT5MovDHvOFOCfT40GcZKt8ysiwBGd C4UcSOCx5grh0r5U8dS/ebAG6CoGAU42FWq0VRF3cmn6WetA332g7r2eqRbkszi2SMpV 0VDM5rsXnpc0n3awdyb0FS+ggO6wYAUXnLi/Yszv22vnrJ/YOUa/gjVIiuXIwtdvzlJY nOJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691392064; x=1691996864; 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=cWWnXKOt1mBeIEynylWCqyYZNRdvBS5hxgZ4awLikYQ=; b=i5Ze7h1A1yL7bPaPDmSca5/Z8uwcjny1YGN+pvBIhE/m3KgaLAxgE5NOgWHIwuu+pz QxGkrqjCzZ2b4U8kxs8yZrtDuO2xqM+RKgkdvqKUMuMwZna20NrU45qP6ib20dvG3PxA rEh5u/b7LpQwUO3MYHDc202Go6THq3A5T23NEQASLwXWlyacwZe/P2plxBvcR8hPuFYI eABnqMVhoQySAZ0IfUUhSFjN4D+paMrk8gK57APIweyikiBEILODqAk/IzbmzhtuHxFO qkgkXKGpiX5My8x4/iVBzWMphIe4akIsgOdbe4i/Yak9gEkSJk5xl8LhQRupXv/Dvrxr 1hUQ== X-Gm-Message-State: AOJu0Yy6v1Ith02DrHGTZIKGZbjJ8DvtntqXc6ymH0xfzHzQBlZc9kcE 2Uenk69HgcLMlbH0CJ59QpNJmw== X-Google-Smtp-Source: AGHT+IHqu92CAl5eucjDnqV8FrzoalX0QSA+DPma7ktAhbEBbnPB9ME04dfagA2jYn4ZmDIGK40XTw== X-Received: by 2002:a05:6512:108b:b0:4fb:8bfd:32e4 with SMTP id j11-20020a056512108b00b004fb8bfd32e4mr5637893lfg.13.1691392063541; Mon, 07 Aug 2023 00:07:43 -0700 (PDT) Received: from localhost ([212.23.236.67]) by smtp.gmail.com with ESMTPSA id u3-20020adfed43000000b0031759e6b43fsm9531115wro.39.2023.08.07.00.07.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Aug 2023 00:07:42 -0700 (PDT) Date: Mon, 7 Aug 2023 09:07:41 +0200 From: Jiri Pirko To: Vadim Fedorenko Cc: Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Milena Olech , Michal Michalik , linux-arm-kernel@lists.infradead.org, poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-clk@vger.kernel.org, Bart Van Assche , intel-wired-lan@lists.osuosl.org Subject: Re: [PATCH net-next v2 7/9] ice: implement dpll interface to control cgu Message-ID: References: <20230804190454.394062-1-vadim.fedorenko@linux.dev> <20230804190454.394062-8-vadim.fedorenko@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230804190454.394062-8-vadim.fedorenko@linux.dev> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230807_000747_272476_71CAA574 X-CRM114-Status: GOOD ( 16.38 ) 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 Fri, Aug 04, 2023 at 09:04:52PM CEST, vadim.fedorenko@linux.dev wrote: >From: Arkadiusz Kubalewski [...] >+/** >+ * ice_dpll_deinit_worker - deinitialize dpll kworker >+ * @pf: board private structure >+ * >+ * Stop dpll's kworker, release it's resources. >+ */ >+static void ice_dpll_deinit_worker(struct ice_pf *pf) >+{ >+ struct ice_dplls *d = &pf->dplls; >+ >+ kthread_cancel_delayed_work_sync(&d->work); >+ kthread_destroy_worker(d->kworker); >+} >+ >+/** >+ * ice_dpll_init_worker - Initialize DPLLs periodic worker >+ * @pf: board private structure >+ * >+ * Create and start DPLLs periodic worker. >+ * >+ * Context: Shall be called after pf->dplls.lock is initialized and >+ * ICE_FLAG_DPLL flag is set. >+ * Return: >+ * * 0 - success >+ * * negative - create worker failure >+ */ >+static int ice_dpll_init_worker(struct ice_pf *pf) >+{ >+ struct ice_dplls *d = &pf->dplls; >+ struct kthread_worker *kworker; >+ >+ ice_dpll_update_state(pf, &d->eec, true); >+ ice_dpll_update_state(pf, &d->pps, true); >+ kthread_init_delayed_work(&d->work, ice_dpll_periodic_work); >+ kworker = kthread_create_worker(0, "ice-dplls-%s", >+ dev_name(ice_pf_to_dev(pf))); >+ if (IS_ERR(kworker)) >+ return PTR_ERR(kworker); >+ d->kworker = kworker; >+ d->cgu_state_acq_err_num = 0; >+ kthread_queue_delayed_work(d->kworker, &d->work, 0); >+ >+ return 0; >+} >+ [...] >+/** >+ * ice_dpll_deinit - Disable the driver/HW support for dpll subsystem >+ * the dpll device. >+ * @pf: board private structure >+ * >+ * Handles the cleanup work required after dpll initialization, freeing >+ * resources and unregistering the dpll, pin and all resources used for >+ * handling them. >+ * >+ * Context: Destroys pf->dplls.lock mutex. >+ */ >+void ice_dpll_deinit(struct ice_pf *pf) >+{ >+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU); >+ >+ if (!test_bit(ICE_FLAG_DPLL, pf->flags)) >+ return; >+ clear_bit(ICE_FLAG_DPLL, pf->flags); >+ Please be symmetric with the init path and move ice_dpll_deinit_worker() call here. That would not only lead to nicer code, also, that will assure that the worker thread can only access initialized object. And as: 1) worked thread can only access initialized objects 2) dpll callbacks can only be called on initialized and registered objects You can remove the check for ICE_FLAG_DPLL flag from ice_dpll_cb_lock() as there would be no longer any possibility when this check could be evaluated as "true". Then, as an unexpected side effect (:O), ice_dpll_cb_lock() basically reduces to just calling mutex_lock(&pf->dplls.lock). So you can remove the thin wrappers of ice_dpll_cb_lock() and ice_dpll_cb_unlock() and instead of doing this obfuscation, you can call mutex_lock(&pf->dplls.lock) and mutex_unlock(&pf->dplls.lock) directly. That is what I'm trying to explain from the beginning. Is it clear now or do we need another iteration? Thanks! >+ ice_dpll_deinit_pins(pf, cgu); >+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu); >+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu); >+ ice_dpll_deinit_info(pf); >+ if (cgu) >+ ice_dpll_deinit_worker(pf); >+ mutex_destroy(&pf->dplls.lock); >+} >+ >+/** >+ * ice_dpll_init - initialize support for dpll subsystem >+ * @pf: board private structure >+ * >+ * Set up the device dplls, register them and pins connected within Linux dpll >+ * subsystem. Allow userspace to obtain state of DPLL and handling of DPLL >+ * configuration requests. >+ * >+ * Context: Initializes pf->dplls.lock mutex. >+ */ >+void ice_dpll_init(struct ice_pf *pf) >+{ >+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU); >+ struct ice_dplls *d = &pf->dplls; >+ int err = 0; >+ >+ err = ice_dpll_init_info(pf, cgu); >+ if (err) >+ goto err_exit; >+ err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC); >+ if (err) >+ goto deinit_info; >+ err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS); >+ if (err) >+ goto deinit_eec; >+ err = ice_dpll_init_pins(pf, cgu); >+ if (err) >+ goto deinit_pps; >+ mutex_init(&d->lock); >+ set_bit(ICE_FLAG_DPLL, pf->flags); Why can't you move the flag set to the end of this function and avoid calling clear_bi on the error path? If you can't, please fix the clear_bit() position (should be at the beginning of deinit_pins label section). >+ if (cgu) { >+ err = ice_dpll_init_worker(pf); >+ if (err) >+ goto deinit_pins; >+ } >+ >+ return; >+ >+deinit_pins: >+ ice_dpll_deinit_pins(pf, cgu); >+deinit_pps: >+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu); >+deinit_eec: >+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu); >+deinit_info: >+ ice_dpll_deinit_info(pf); >+err_exit: >+ clear_bit(ICE_FLAG_DPLL, pf->flags); >+ mutex_destroy(&d->lock); >+ dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err); >+} [...] >diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c >index 2e80d5cd9f56..4adc74f1cb1f 100644 >--- a/drivers/net/ethernet/intel/ice/ice_main.c >+++ b/drivers/net/ethernet/intel/ice/ice_main.c >@@ -4627,6 +4627,10 @@ static void ice_init_features(struct ice_pf *pf) > if (ice_is_feature_supported(pf, ICE_F_GNSS)) > ice_gnss_init(pf); > >+ if (ice_is_feature_supported(pf, ICE_F_CGU) || >+ ice_is_feature_supported(pf, ICE_F_PHY_RCLK)) >+ ice_dpll_init(pf); >+ > /* Note: Flow director init failure is non-fatal to load */ > if (ice_init_fdir(pf)) > dev_err(dev, "could not initialize flow director\n"); >@@ -4653,6 +4657,9 @@ static void ice_deinit_features(struct ice_pf *pf) > ice_gnss_exit(pf); > if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) > ice_ptp_release(pf); >+ if (ice_is_feature_supported(pf, ICE_F_PHY_RCLK) || >+ ice_is_feature_supported(pf, ICE_F_CGU)) As you internally depend on ICE_FLAG_DPLL flag, this check is redundant. >+ ice_dpll_deinit(pf); > } > > static void ice_init_wakeup(struct ice_pf *pf) >-- >2.27.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel