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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0DE75C433F5 for ; Thu, 4 Nov 2021 21:26:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DBA156124F for ; Thu, 4 Nov 2021 21:26:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230271AbhKDV24 (ORCPT ); Thu, 4 Nov 2021 17:28:56 -0400 Received: from mga18.intel.com ([134.134.136.126]:36571 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229924AbhKDV2z (ORCPT ); Thu, 4 Nov 2021 17:28:55 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10158"; a="218697594" X-IronPort-AV: E=Sophos;i="5.87,209,1631602800"; d="scan'208";a="218697594" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2021 14:26:16 -0700 X-IronPort-AV: E=Sophos;i="5.87,209,1631602800"; d="scan'208";a="730274965" Received: from wangli6-mobl.amr.corp.intel.com (HELO intel.com) ([10.252.138.130]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2021 14:26:16 -0700 Date: Thu, 4 Nov 2021 14:26:14 -0700 From: Ben Widawsky To: Dan Williams Cc: linux-cxl@vger.kernel.org, Chet Douglas , Alison Schofield , Ira Weiny , Jonathan Cameron , Vishal Verma Subject: Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver Message-ID: <20211104212614.ojcppcnqywao2a4g@intel.com> References: <20211022183709.1199701-1-ben.widawsky@intel.com> <20211022183709.1199701-9-ben.widawsky@intel.com> <20211104163727.ybgenphhwaruqhff@intel.com> <20211104194602.mhgnlbf65ilyul5c@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-cxl@vger.kernel.org On 21-11-04 13:00:35, Dan Williams wrote: > On Thu, Nov 4, 2021 at 12:46 PM Ben Widawsky wrote: > > > > On 21-11-04 12:17:48, Dan Williams wrote: > > > On Thu, Nov 4, 2021 at 9:37 AM Ben Widawsky wrote: > > > > > > > > On 21-10-29 18:37:36, Dan Williams wrote: > > > > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky wrote: > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > +void trigger_decoder_autoremove(void *data) > > > > > +{ > > > > > + struct cxl_decoder *cxld = data; > > > > > + struct device *host = get_cxl_topology_host(); > > > > > + > > > > > + /* The topology host driver beat us to the punch */ > > > > > + if (!host) > > > > > + return; > > > > > + > > > > > + devm_remove_action(host, cxld_unregister, &cxld->dev); > > > > > + put_cxl_topology_host(host); > > > > > +} > > > > > + > > > > > +/** > > > > > + * cxl_port_decoder_autoremove - remove decoders discovered by the port driver > > > > > + * @cxld: decoder to remove > > > > > + * > > > > > + * CXL.mem requires an intact port / decoder topology from root level > > > > > + * platform decoders to endpoint decoders. Arrange for decoders > > > > > + * enumerated by the port driver to be removed when either the root is > > > > > + * removed (in which the entire hierarchy is removed), or when an > > > > > + * individual port is disabled, in which case only that port's > > > > > + * sub-section of the hierarchy is removed. > > > > > + */ > > > > > +int cxl_port_decoder_autoremove(struct cxl_decoder *cxld) > > > > > +{ > > > > > + struct cxl_port *port = to_cxl_port(cxld->dev.parent); > > > > > + struct device *host = get_cxl_topology_host(); > > > > > + int rc; > > > > > + > > > > > + if (!host) > > > > > + return -ENODEV; > > > > > + > > > > > + if (!port->dev.driver) { > > > > > + rc = -ENXIO; > > > > > + goto out; > > > > > + } > > > > > + > > > > > + rc = cxl_decoder_autoremove(host, cxld); > > > > > + if (rc) > > > > > + goto out; > > > > > + > > > > > + rc = devm_add_action_or_reset(&port->dev, trigger_decoder_autoremove, > > > > > + cxld); > > > > > +out: > > > > > + put_cxl_topology_host(host); > > > > > + return rc; > > > > > +} > > > > > +EXPORT_SYMBOL_NS_GPL(cxl_port_decoder_autoremove, CXL); > > > > > > > > The only port which has decoders that aren't discovered by the port driver would > > > > be the "root port" because it's platform specific. However, that port still is > > > > treated similarly to the other ports. Therefore I don't see why you need > > > > cxl_decoder_autoremove() anymore. Could you please explain? I think the safety > > > > check in trigger_decoder_autoremove() makes this work for all cases. > > > > > > I don't think it's worth the games to explain why CXL sees fit to > > > register (here comes your favorite argument...) non-idiomatic devm > > > actions on devices that are not associated with the running device + > > > driver. So as long as the port driver auto-unloads its child devices > > > then we're golden. Is your concern that you want to have the CFMWS > > > decoders registers from cxl_port and not cxl_acpi? > > > > Hey! I'm all for idioms when it makes sense. To me, one of the coolest things > > about working on a new subsystem is you get to define some of your own idioms, > > but anyway... > > > > First, no I think the platform driver (cxl_acpi) should enumerate the platform > > decoders. I confused devm_remove_action() with devm_release_action(). I wonder > > though, if you made trigger_decoder_autoremove() call devm_release_action, would > > that be sufficient and then remove cxl_decoder_autoremove()? > > devm_release_action() is not needed if the devm is allowed to fire > naturally which was the realization I came to here: > > https://lore.kernel.org/r/CAPcyv4gpA=DH0SQvRdmF6dY01mZ1S-gGEWTSDbb+0ajYtyNv0A@mail.gmail.com > > I.e. I regret opening that pandora's box of registering "remote" devm > actions vs typical local devm actions. I wasn't suggesting release() fixes anything, but it did seem like we could get rid of cxl_decoder_autoremove() if we switched it to release(). Must of this gets too complex for me too quickly though, so if you think it won't work I'm happy to leave it at that.