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 2C0A6C001DF for ; Thu, 3 Aug 2023 08:03:23 +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=JOjnUXr+c30afwtr68qVzNEuZzAyHzfrVrQ0ql19F2c=; b=GOrykQLNAh8/Pb qnZ4726x+Fmm4LYoZ3fANIcWJ6L5/NRRwkSiaxB3e/VVry7Af5w5WciBlx1CS9Kq/JKT9Tj8V+Yzb XmJVBCNLRfRlKNWbVcLEJ6xTfzXB2958jFekY3LjI78dRRJzxAjqNFZJEmA11RANBw7YMpC4bSlc0 FFUqZT+L2L6HIKu0honx2YXXaZ1oyst9KDwU2uDQJ2iwh+IpSq/dT5WhoA9rhxcoqS57CE7wv9T7R uO5hvNn+wngV5T9CHfMhol51+Oiv/MJy3apHkbWEH8dDMTS9sCkQyXweT3cnIL0fDUOviZmT1Ta7X mRa0wTPLrJYw8SrCrbVA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qRTIL-006xKb-10; Thu, 03 Aug 2023 08:02:53 +0000 Received: from mail-lj1-x235.google.com ([2a00:1450:4864:20::235]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qRTIH-006xJ7-0V for linux-arm-kernel@lists.infradead.org; Thu, 03 Aug 2023 08:02:52 +0000 Received: by mail-lj1-x235.google.com with SMTP id 38308e7fff4ca-2b9bee2d320so9417771fa.1 for ; Thu, 03 Aug 2023 01:02:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20221208.gappssmtp.com; s=20221208; t=1691049760; x=1691654560; 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=sWjI4XbkdglCN8ZQ+xl/2dRug01IAQ+rZAOGhdrF4Ds=; b=QURGvvcGEVr4NsJwaPPk0f8L+qWGFsRIW9kJE6x1NLrHzXTekeX+1xXxtFhw4vPGrp SDA8rpH6xkOQoydND3q8Qi8H3GzflEgEPP85x3WbxNu3AytURbjwWiCcG5kTpfCEvnmI ExELbZ43a8zZOPsoyYrGl1iC19dfiX9GDL8Phd8AfqV5V7vjhBeMdoJ+lVTCoEh1rU1c fOA6U5xBevZhEkEWBPtv4eLnJZ+bBipcIooK3sRK8zN3PEDrbd9MFQSdQfDUpvONU4fq uuKX2f8rXCK2sVkAHvESCs8L7n8ZV2DIgE01wfFfqPFy3whp1ywqQ0Jo2Sn8LcMSPnZv jTkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691049760; x=1691654560; 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=sWjI4XbkdglCN8ZQ+xl/2dRug01IAQ+rZAOGhdrF4Ds=; b=O8TUXLoTB2PtYNBIHQOw4+ZaFV5fCzbHIV26BLZ3ViPqTFA/QuT4scVmkZ2aXprGIC wqc9/KqtLw5WeOJUE5s63sWFqB15XNrayuGdXm7PXtuCrE2HWrhH/Nwc60W3aCqtV2Wg bwTrVv0FRxa4nWtpeGIMDaesFH6qpGSUFvxmyQn3cNCvtpCSsL4XSAJhs16veJiw20Id pfG4KHY2UUu02e73dpuVof/lD3HPb33e8q3ZgJtKU2+tRX9JG2aiCNq0wT5ah46VS1E8 VZHd/jx7YdZZJYWxV1CxFYa3RFQwUztvMts6t5olVzPDYwimzcfolQoDw78Rl9IwbJ5p Skkg== X-Gm-Message-State: ABy/qLYHH22kpa6yLoCzGjKzUx2AZ9Eytx03bGozxcevqdvsWJcfsLyO mgo7ChJah4N6eOnRyzNc0kZpCvusr8vOQCx+j2It9A== X-Google-Smtp-Source: APBJJlFG3nyJLz++ssYp8bbbyIAy60WW2BTl3Sv6RDcrxGf3ztv8ov0tc9h4az4k3zbOiGeUjMteew== X-Received: by 2002:a2e:9f50:0:b0:2b9:ad7d:a144 with SMTP id v16-20020a2e9f50000000b002b9ad7da144mr6855771ljk.11.1691049760407; Thu, 03 Aug 2023 01:02:40 -0700 (PDT) Received: from localhost ([212.23.236.67]) by smtp.gmail.com with ESMTPSA id t12-20020a7bc3cc000000b003fbc30825fbsm3547854wmj.39.2023.08.03.01.02.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Aug 2023 01:02:39 -0700 (PDT) Date: Thu, 3 Aug 2023 10:02:38 +0200 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: Vadim Fedorenko , Jakub Kicinski , Jonathan Lemon , Paolo Abeni , "Olech, Milena" , "Michalik, Michal" , "linux-arm-kernel@lists.infradead.org" , poros , mschmidt , "netdev@vger.kernel.org" , "linux-clk@vger.kernel.org" , Bart Van Assche Subject: Re: [PATCH 09/11] ice: implement dpll interface to control cgu Message-ID: References: <20230720091903.297066-1-vadim.fedorenko@linux.dev> <20230720091903.297066-10-vadim.fedorenko@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230803_010249_410024_2458E831 X-CRM114-Status: GOOD ( 24.90 ) 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 Wed, Aug 02, 2023 at 05:48:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Wednesday, August 2, 2023 8:57 AM >> >>Tue, Aug 01, 2023 at 04:50:44PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko >>>>Sent: Monday, July 31, 2023 2:20 PM >>>> >>>>Sat, Jul 29, 2023 at 01:03:59AM CEST, arkadiusz.kubalewski@intel.com >>>>wrote: >>>>>>From: Jiri Pirko >>>>>>Sent: Friday, July 21, 2023 1:39 PM >>>>>> >>>>>>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko@linux.dev wrote: >>>>>>>From: Arkadiusz Kubalewski >>>>> >>>> >>>>[...] >>>> >>>> >>>>>>>+static int ice_dpll_cb_lock(struct ice_pf *pf, struct netlink_ext_ack >>>>>>>*extack) >>>>>>>+{ >>>>>>>+ int i; >>>>>>>+ >>>>>>>+ for (i = 0; i < ICE_DPLL_LOCK_TRIES; i++) { >>>>>>>+ if (!test_bit(ICE_FLAG_DPLL, pf->flags)) { >>>>>> >>>>>>And again, as I already told you, this flag checking is totally >>>>>>pointless. See below my comment to ice_dpll_init()/ice_dpll_deinit(). >>>>>> >>>>> >>>>>This is not pointless, will explain below. >>>>> >>>>>> >>>>>> >>>>> >>>>>[...] >>>>> >>>> >>>>[...] >>>> >>>> >>>>>>>+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); >>>>>>>+ >>>>>>>+ 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); >>>>>> >>>>>>Could you please order the ice_dpll_deinit() to be symmetrical to >>>>>>ice_dpll_init()? Then, you can drop ICE_FLAG_DPLL flag entirely, as the >>>>>>ice_dpll_periodic_work() function is the only reason why you need it >>>>>>currently. >>>>>> >>>>> >>>>>Not true. >>>>>The feature flag is common approach in ice. If the feature was >>>>>successfully >>>> >>>>The fact that something is common does not necessarily mean it is >>>>correct. 0 value argument. >>>> >>> >>>Like using functions that unwrap netlink attributes as unsigned when >>>they are in fact enums with possibility of being signed? >> >>Looks this is bothering you, sorry about that. >> > >Just poining out. > >> >>> >>>This is about consistent approach in ice driver. >>> >>>> >>>>>initialized the flag is set. It allows to determine if deinit of the >>>>>feature >>>>>is required on driver unload. >>>>> >>>>>Right now the check for the flag is not only in kworker but also in each >>>>>callback, if the flag were cleared the data shall be not accessed by >>>>>callbacks. >>>> >>>>Could you please draw me a scenario when this could actually happen? >>>>It is just a matter of ordering. Unregister dpll device/pins before you >>>>cleanup the related resources and you don't need this ridiculous flag. >>>> >>> >>>Flag allows to determine if dpll was successfully initialized and do >>>proper >>>deinit on rmmod only if it was initialized. That's all. >> >>You are not answering my question. I asked about how the flag helps is >>you do unregister dpll devices/pins and you free related resources in >>the correct order. Because that is why you claim you need this flag. >> > >I do not claim such thing, actually opposite, I said it helps a bit >but the reason for existence is different, yet you are still trying to >imply me this. > >>I'm tired of this. Keep your driver tangled for all I care, I'm trying >>to help you, obviously you are not interested. >> > >With review you are doing great job and many thanks for that. > >Already said it multiple times, the main reason of flag existence is not a >use in the callback but to determine successful dpll initialization. So use it only for this, nothing else. Use it only to check during cleanup that you need to do the cleanup as init was previously done. >As there is no need to call unregister on anything if it was not successfully >registered. > >> >>> >>>> >>>>>I know this is not required, but it helps on loading and unloading the >>>>>driver, >>>>>thanks to that, spam of pin-get dump is not slowing the driver >>>>>load/unload. >>>> >>>>? Could you plese draw me a scenario how such thing may actually happen? >>> >>>First of all I said it is not required. >>> >>>I already draw you this with above sentence. >>>You need spam pin-get asynchronously and unload driver, what is not clear? >>>Basically mutex in dpll is a bottleneck, with multiple requests waiting >>>for >>>mutex there is low change of driver getting mutex when doing unregisters. >> >>How exactly your flag helps you in this scenario? It does not. >> > >In this scenario it helps because it fails the callbacks when dpll subsystem >was partially initialized and callbacks can be already invoked, but in fact >the dpll initialization is not yet finished in the driver, and there will always >be the time between first and second dpll registration where we might wait for >the mutex to become available on dpll core part. Draw it to me, please, where exatly there is a problem. I'm still convinced that with the proper ordering of init/cleanup flows, you'll get all you need, without any flag use. > >> >>> >>>We actually need to redesign the mutex in dpll core/netlink, but I guess >>>after >>>initial submission. >> >>Why? >> > >The global mutex for accessing the data works just fine, but it is slow. >Maybe we could improve this by using rwlock instead. "it is slow" is quite vague description of what's wrong with the locking. > >Thank you! >Arkadiusz > >> >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>Thanks! >>>> >>>> >>>>> >>>>>> >>>>>>>+ mutex_destroy(&pf->dplls.lock); >>>>>>>+} >>>> >>>> >>>>[...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel