Message ID | 20230207073610.3290391-1-rajat.khandelwal@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2103308wrn; Sun, 5 Feb 2023 23:43:46 -0800 (PST) X-Google-Smtp-Source: AK7set9fd1E5Uw+JjtC7MJM8IgEJrdZhrKoWRW+Ger3CY6x+KA4lryEQtgX1btMn3uGcbxxBPyt6 X-Received: by 2002:a17:906:9c95:b0:87b:d2b3:67ca with SMTP id fj21-20020a1709069c9500b0087bd2b367camr19346318ejc.75.1675669426391; Sun, 05 Feb 2023 23:43:46 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675669426; cv=none; d=google.com; s=arc-20160816; b=OUSeAaVma66kuHGAv+Av2mogZBnUCOcQ7TngcRinDpAAugtbx7d22zuwO6fC0dW9M4 6BJh4Wdevst8hfWG3m5FZmtDxYszm76VklF5JrW9+D7Ndv7aFO6yQGAPrJQHgAtlyrp/ ebYhmADR61la45L0AjObOYWB8TnRlACUKghR2iWUwKqa1bunLlanDUVT6Xhvq2yYsVKw E7HstXUp9HA4ZhQ26dqsddBmki7SlWPj9i4ZJrNP+6pZ7et1Qj+RJu6bp8OfnmjkQHSe ndDe4e1tofMRjmrMfsrTAwx7WX49TW7Ik94fSzacScHAXYIsN+lKZUY1CXOP8n8fCQ// yx6w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=xfRAFNUg5XAVPwkq8J6B7nyDIZFeqdc8sGIkRJ3vfJA=; b=wCS9Xv55gC0MTRwWk+GCpV2GGs79Mru9mgtlhZppgbA4cXbS7n3ucf9cmDCTg1bkEm VOvD8yXXjF2xWv7S6cKLpQnbYr9/8N9MNixtY35eQMSelvaAkFPXwVYTglRYb/XLPeAc cB/gMRzvKTSdU9KxIK2nuAP8k3YGEfika5djzHX42ldPf5EMEqEHxKL/QCBJCQqXMVs8 /wRMYX7ZDeHp4ENLKHvUDJp+NoFcOMY4cxPN2hPM0dlOmytSmqCKipicmV/GOY+O+kim vahimvj20/9R/DiDfq+r5Ae89vjp0dHt5LtZOlrH6WY6qX/kYHr7CEW19lMoQk0SAEyz USZw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=LYbb8rpp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id kb22-20020a170907925600b00878560b5f52si11248729ejb.335.2023.02.05.23.43.23; Sun, 05 Feb 2023 23:43:46 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=LYbb8rpp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229683AbjBFHeD (ORCPT <rfc822;kmanaouilinux@gmail.com> + 99 others); Mon, 6 Feb 2023 02:34:03 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48742 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229567AbjBFHeB (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 6 Feb 2023 02:34:01 -0500 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C32B94EDF for <linux-kernel@vger.kernel.org>; Sun, 5 Feb 2023 23:33:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675668837; x=1707204837; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=X2WBQ9RJBzamhY9mmwq8nKykzM0EbKzq7klufUYGkLY=; b=LYbb8rppbhihxKmLiRibhXcPaKkXxx9yX+VDGNZuPOUIfv3gEhEiAnc3 0Dvcg7vuFjLWEQqX0mkfkkYdXgXgR5MIPOeu3yvT6zMG9q8U+1UhkO/KO AFzrI4f1/AegCEpw0wd4NCsSie3eK547m3xzmlkAzgeZ3Vj5hvQfsArwz pkBVEfOeBaZhyv1uhxhBeeGsEqDTf2AvmkjnReoRpvLCxLasdSGcapmE/ xX7tRm5Vr0L84Bz1O9580SOykUoLpJ8RAszPFQfrVMoQDLEZNwnY7FJBJ XHJvrDSO2n8nQISa8BtlF6HzbRP6JyA7ntSZcDiPV7g3P6JDUkSIkXxHK Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10612"; a="309481425" X-IronPort-AV: E=Sophos;i="5.97,276,1669104000"; d="scan'208";a="309481425" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Feb 2023 23:33:57 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10612"; a="698757186" X-IronPort-AV: E=Sophos;i="5.97,276,1669104000"; d="scan'208";a="698757186" Received: from unknown (HELO rajath-NUC10i7FNH..) ([10.223.165.88]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Feb 2023 23:33:54 -0800 From: Rajat Khandelwal <rajat.khandelwal@linux.intel.com> To: pmalani@chromium.org, bleung@chromium.org, groeck@chromium.org Cc: chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org, rajat.khandelwal@intel.com, Rajat Khandelwal <rajat.khandelwal@linux.intel.com> Subject: [PATCH] platform/chrome: Avoid fetching the type-C parameters again Date: Tue, 7 Feb 2023 13:06:10 +0530 Message-Id: <20230207073610.3290391-1-rajat.khandelwal@linux.intel.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DATE_IN_FUTURE_24_48, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1757066744561736576?= X-GMAIL-MSGID: =?utf-8?q?1757066744561736576?= |
Series |
platform/chrome: Avoid fetching the type-C parameters again
|
|
Commit Message
Rajat Khandelwal
Feb. 7, 2023, 7:36 a.m. UTC
The struct 'cros_typec_port' incorporates three type-C parameters,
'ori_sw', 'retimer', and 'mux'.
These parameters in the struct 'typec_port' are being already
configured when 'typec_register_port' is being called.
The bigger picture - 'typec_register_port' can return an error and
the type-C parameters could go unconfigured. However, the callback
will return EPROBE_DEFER and the driver will again be getting probed
trying to configure them again.
However, currently, they are again tried to get fetched for the
'cros_typec_port' struct, which sometimes could result in an error
and these parameters could go unconfigured (no provision of deferring
the probe in this case, so we are left with unconfigured parameters).
Hence, assign the parameters to the corresponding ones in the struct
'typec_port' after they are fetched in 'typec_register_port'.
Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
---
drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++---------------
1 file changed, 22 insertions(+), 36 deletions(-)
Comments
Hi Rajat, Please use the right header in the commit message. There are plenty of examples in the git log. On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal <rajat.khandelwal@linux.intel.com> wrote: > > The struct 'cros_typec_port' incorporates three type-C parameters, > 'ori_sw', 'retimer', and 'mux'. > These parameters in the struct 'typec_port' are being already > configured when 'typec_register_port' is being called. > > The bigger picture - 'typec_register_port' can return an error and > the type-C parameters could go unconfigured. However, the callback > will return EPROBE_DEFER and the driver will again be getting probed > trying to configure them again. > However, currently, they are again tried to get fetched for the > 'cros_typec_port' struct, which sometimes could result in an error > and these parameters could go unconfigured (no provision of deferring > the probe in this case, so we are left with unconfigured parameters). This is by design. If the switch handles cannot be obtained for any reason other that -EPROBE_DEFER, we will not re-attempt to acquire them, and should continue driver probe and carry on with limited functionality. If there is a sporadic error other than -EPROBE_DEFER, that points to a deeper issue in firmware which should be investigated. > > Hence, assign the parameters to the corresponding ones in the struct > 'typec_port' after they are fetched in 'typec_register_port'. > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com> > --- > drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++--------------- > 1 file changed, 22 insertions(+), 36 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index 001b0de95a46..0265c0d38bd6 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -24,6 +24,8 @@ > #include <linux/usb/typec_tbt.h> > #include <linux/usb/role.h> > > +#include "../../usb/typec/class.h" That is a local header. We cannot use it outside of drivers/usb/typec/ > + > #define DRV_NAME "cros-ec-typec" > > #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \ > @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap, > return 0; > } > > -static int cros_typec_get_switch_handles(struct cros_typec_port *port, > - struct fwnode_handle *fwnode, > - struct device *dev) > +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port, > + struct fwnode_handle *fwnode, > + struct device *dev) > { > - port->mux = fwnode_typec_mux_get(fwnode, NULL); > - if (IS_ERR(port->mux)) { > - dev_dbg(dev, "Mux handle not found.\n"); > - goto mux_err; > - } > - > - port->retimer = fwnode_typec_retimer_get(fwnode); > - if (IS_ERR(port->retimer)) { > - dev_dbg(dev, "Retimer handle not found.\n"); > - goto retimer_sw_err; > - } > - > - port->ori_sw = fwnode_typec_switch_get(fwnode); > - if (IS_ERR(port->ori_sw)) { > - dev_dbg(dev, "Orientation switch handle not found.\n"); > - goto ori_sw_err; > - } > - > port->role_sw = fwnode_usb_role_switch_get(fwnode); > if (IS_ERR(port->role_sw)) { > dev_dbg(dev, "USB role switch handle not found.\n"); > - goto role_sw_err; > + return -ENODEV; > } > > return 0; > - > -role_sw_err: > - typec_switch_put(port->ori_sw); > - port->ori_sw = NULL; > -ori_sw_err: > - typec_retimer_put(port->retimer); > - port->retimer = NULL; > -retimer_sw_err: > - typec_mux_put(port->mux); > - port->mux = NULL; > -mux_err: > - return -ENODEV; > } > > static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num, > @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > return 0; > } > > +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port) > +{ > + struct typec_port *port = cros_port->port; > + > + if (IS_ERR_OR_NULL(port)) > + return; > + > + cros_port->mux = port->mux; > + cros_port->retimer = port->retimer; > + cros_port->ori_sw = port->sw; > +} Sorry, but this is not the right approach. These handles are ref-counted. We intentionally take extra references to these using the provided API. Please don't use raw references like this. Also, if the fwnode_*_get() functions are failing, why are we to assume that they worked for the Type-C class code (that code uses the same functions) [1] ? This approach is not right. BR, -Prashant [1] https://elixir.bootlin.com/linux/v6.2-rc7/source/drivers/usb/typec/class.c#L2317
Hey Rajat, On Tue, Feb 7, 2023 at 12:53 AM Rajat Khandelwal <rajat.khandelwal@linux.intel.com> wrote: > > Hi Prashant, > Please find the comments inline. > > On 2/7/2023 1:25 AM, Prashant Malani wrote: > > Hi Rajat, > > Please use the right header in the commit message. There are plenty of > examples in the git log. > > Got it Prashant. > > On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal > <rajat.khandelwal@linux.intel.com> wrote: > > The struct 'cros_typec_port' incorporates three type-C parameters, > 'ori_sw', 'retimer', and 'mux'. > These parameters in the struct 'typec_port' are being already > configured when 'typec_register_port' is being called. > > The bigger picture - 'typec_register_port' can return an error and > the type-C parameters could go unconfigured. However, the callback > will return EPROBE_DEFER and the driver will again be getting probed > trying to configure them again. > However, currently, they are again tried to get fetched for the > 'cros_typec_port' struct, which sometimes could result in an error > and these parameters could go unconfigured (no provision of deferring > the probe in this case, so we are left with unconfigured parameters). > > This is by design. If the switch handles cannot be obtained for any reason > other that -EPROBE_DEFER, we will not re-attempt to acquire them, and > should continue driver probe and carry on with limited functionality. > > Why should we even try to acquire them in cros-ec-typec, when we can use them > natively populated? Those handles are internal to the Type-C class code. Anything outside shouldn't be accessing them (that applies to anything within struct typec_port); they are intentionally kept opaque to protect implementation internals. > > If there is a sporadic error other than -EPROBE_DEFER, that points to a > deeper issue in firmware which should be investigated. > > Hence, assign the parameters to the corresponding ones in the struct > 'typec_port' after they are fetched in 'typec_register_port'. > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com> > --- > drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++--------------- > 1 file changed, 22 insertions(+), 36 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index 001b0de95a46..0265c0d38bd6 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -24,6 +24,8 @@ > #include <linux/usb/typec_tbt.h> > #include <linux/usb/role.h> > > +#include "../../usb/typec/class.h" > > That is a local header. We cannot use it outside of drivers/usb/typec/ > > Ok, sure. To avoid restructuring the struct definitions, I simply put it for now. > This can be rectified after we arrive at a culmination. > > + > #define DRV_NAME "cros-ec-typec" > > #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \ > @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap, > return 0; > } > > -static int cros_typec_get_switch_handles(struct cros_typec_port *port, > - struct fwnode_handle *fwnode, > - struct device *dev) > +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port, > + struct fwnode_handle *fwnode, > + struct device *dev) > { > - port->mux = fwnode_typec_mux_get(fwnode, NULL); > - if (IS_ERR(port->mux)) { > - dev_dbg(dev, "Mux handle not found.\n"); > - goto mux_err; > - } > - > - port->retimer = fwnode_typec_retimer_get(fwnode); > - if (IS_ERR(port->retimer)) { > - dev_dbg(dev, "Retimer handle not found.\n"); > - goto retimer_sw_err; > - } > - > - port->ori_sw = fwnode_typec_switch_get(fwnode); > - if (IS_ERR(port->ori_sw)) { > - dev_dbg(dev, "Orientation switch handle not found.\n"); > - goto ori_sw_err; > - } > - > port->role_sw = fwnode_usb_role_switch_get(fwnode); > if (IS_ERR(port->role_sw)) { > dev_dbg(dev, "USB role switch handle not found.\n"); > - goto role_sw_err; > + return -ENODEV; > } > > return 0; > - > -role_sw_err: > - typec_switch_put(port->ori_sw); > - port->ori_sw = NULL; > -ori_sw_err: > - typec_retimer_put(port->retimer); > - port->retimer = NULL; > -retimer_sw_err: > - typec_mux_put(port->mux); > - port->mux = NULL; > -mux_err: > - return -ENODEV; > } > > static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num, > @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, > return 0; > } > > +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port) > +{ > + struct typec_port *port = cros_port->port; > + > + if (IS_ERR_OR_NULL(port)) > + return; > + > + cros_port->mux = port->mux; > + cros_port->retimer = port->retimer; > + cros_port->ori_sw = port->sw; > +} > > Sorry, but this is not the right approach. These handles are > ref-counted. We intentionally > take extra references to these using the provided API. Please don't > use raw references like this. > > Aah, didn't see they were getting ref-counted. Even so, I don't understand > the part that why do we need to take extra references? Can't we not use the > same reference to the nodes which were populated in the type-C driver? No; they have to be refcounted (that's the way all firmware node handles are used). Any "client" or "caller" that wants to use the fwnode handles has to use the correct APIs. Refcounting is necessary for the fwnode/property framework to ensure that there are no stale references for firmware device nodes (needed to prevent leaks during device creation/removal). If the fwnode framework doesn't know that you (driver) have a reference, it is free to invalidate that reference (and remove the underlying device) without you(driver) knowing anything about it (I'm sure I'm oversimplifying a lot over here, but that is the gist AFAIK). > > On another note, maybe remove these 3 handles from 'cros_typec_port' altogether, > and use the handles from 'typec_port' inside the 'cros_typec_struct'? >Other drivers > in the type-C domain use the same concept AFAIK. They just have the 'typec_port' from > which they use all the handles natively populated. What drivers are these? No port driver should be using these handles without refcounting them with the right API calls. > > Further, the same 'fwnode' will be matched in 'typec_mux_match'/'typec_switch_match', > irrespective of it being called from 'typec_register_port' or 'cros_typec_get_switch_handles'. > Thus, mux->mux_dev->dev and sw->sw_dev->dev will represent the same 'dev' struct. > Is there actually a point of referencing them again in cros-ec-typec? Even when we decrement the > ref-counter, it will be decremented of the same 'dev'! > Please correct me if I'm wrong. :) > > Also, if the fwnode_*_get() functions are failing, why are we to > assume that they worked for the > Type-C class code (that code uses the same functions) [1] ? > > They surely worked for the type-C class code, since if either of the 'mux' and 'switch' > handles are ERR, it will return an error. cros-ec-typec will then try to probe again. > Since the handling reaches after the 'typec_register_port', I think we can be sure that they > had to be succeeded for the type-C class driver. If those calls are succeeding when they are called within typec_register_port(), there is no reason why they should fail when called from cros-ec-typec (we are just taking references to existing handles after all). I would suggest looking into why the calls to get the firmware handles are failing from the port driver. On a unrelated note: Are you responding in plain text mode? I suspect you may not be, since the indentation of responses is getting messed up. BR, -Prashant
Hi Prashant, I think I got it now. Ultimately, the 'device' mux_dev has to be ref-incremented for another driver (in our case - cros-ec-typec). Was in confusion if that'd be necessary since one of the type-C drivers - ANX7411 registers the 'typec_port' inside its struct by 'typec_register_port', and uses the 'typec_mux' from within the struct rather than ref-incrementing it again. To be more specific, return typec_set_mode(ctx->typec.port, mode); //Here the port corresponds to 'typec_port' and ultimately it will use 'typec_mux' from //it and call 'typec_mux_set'. Maybe something else could be going on here, or I missed something? Not sure. Ok, raw assigning the pointers is not the solution, I agree. How about something like this? static void cros_typec_params_get(struct cros_typec_port *cros_port) { struct typec_port *port = cros_port->port; struct typec_switch_dev *sw_dev; struct typec_mux_dev *mux_dev; if (IS_ERR_OR_NULL(port)) return; cros_port->mux = port->mux; mux_dev = cros_port->mux->mux_dev; get_device(&mux_dev->dev); cros_port->ori_sw = port->sw; sw_dev = cros_port->ori_sw->sw_dev; get_device(&sw_dev->dev); } This will increment the ref-count of the device nodes (pardon me, if any compilation error :)). All I am pushing for is to not go through fwnode_match again and find the same mux_dev/sw_dev. If ref-count has to be incremented, we can do it like above. Should this not be a more robust way we assign the device nodes to our cros-ec-typec port node, which we have already populated? Thanks Rajat On 2/8/2023 12:06 AM, Prashant Malani wrote: > Hey Rajat, > > On Tue, Feb 7, 2023 at 12:53 AM Rajat Khandelwal > <rajat.khandelwal@linux.intel.com> wrote: >> Hi Prashant, >> Please find the comments inline. >> >> On 2/7/2023 1:25 AM, Prashant Malani wrote: >> >> Hi Rajat, >> >> Please use the right header in the commit message. There are plenty of >> examples in the git log. >> >> Got it Prashant. >> >> On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal >> <rajat.khandelwal@linux.intel.com> wrote: >> >> The struct 'cros_typec_port' incorporates three type-C parameters, >> 'ori_sw', 'retimer', and 'mux'. >> These parameters in the struct 'typec_port' are being already >> configured when 'typec_register_port' is being called. >> >> The bigger picture - 'typec_register_port' can return an error and >> the type-C parameters could go unconfigured. However, the callback >> will return EPROBE_DEFER and the driver will again be getting probed >> trying to configure them again. >> However, currently, they are again tried to get fetched for the >> 'cros_typec_port' struct, which sometimes could result in an error >> and these parameters could go unconfigured (no provision of deferring >> the probe in this case, so we are left with unconfigured parameters). >> >> This is by design. If the switch handles cannot be obtained for any reason >> other that -EPROBE_DEFER, we will not re-attempt to acquire them, and >> should continue driver probe and carry on with limited functionality. >> >> Why should we even try to acquire them in cros-ec-typec, when we can use them >> natively populated? > Those handles are internal to the Type-C class code. Anything outside shouldn't > be accessing them (that applies to anything within struct typec_port); they are > intentionally kept opaque to protect implementation internals. > >> If there is a sporadic error other than -EPROBE_DEFER, that points to a >> deeper issue in firmware which should be investigated. >> >> Hence, assign the parameters to the corresponding ones in the struct >> 'typec_port' after they are fetched in 'typec_register_port'. >> >> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com> >> --- >> drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++--------------- >> 1 file changed, 22 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c >> index 001b0de95a46..0265c0d38bd6 100644 >> --- a/drivers/platform/chrome/cros_ec_typec.c >> +++ b/drivers/platform/chrome/cros_ec_typec.c >> @@ -24,6 +24,8 @@ >> #include <linux/usb/typec_tbt.h> >> #include <linux/usb/role.h> >> >> +#include "../../usb/typec/class.h" >> >> That is a local header. We cannot use it outside of drivers/usb/typec/ >> >> Ok, sure. To avoid restructuring the struct definitions, I simply put it for now. >> This can be rectified after we arrive at a culmination. >> >> + >> #define DRV_NAME "cros-ec-typec" >> >> #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \ >> @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap, >> return 0; >> } >> >> -static int cros_typec_get_switch_handles(struct cros_typec_port *port, >> - struct fwnode_handle *fwnode, >> - struct device *dev) >> +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port, >> + struct fwnode_handle *fwnode, >> + struct device *dev) >> { >> - port->mux = fwnode_typec_mux_get(fwnode, NULL); >> - if (IS_ERR(port->mux)) { >> - dev_dbg(dev, "Mux handle not found.\n"); >> - goto mux_err; >> - } >> - >> - port->retimer = fwnode_typec_retimer_get(fwnode); >> - if (IS_ERR(port->retimer)) { >> - dev_dbg(dev, "Retimer handle not found.\n"); >> - goto retimer_sw_err; >> - } >> - >> - port->ori_sw = fwnode_typec_switch_get(fwnode); >> - if (IS_ERR(port->ori_sw)) { >> - dev_dbg(dev, "Orientation switch handle not found.\n"); >> - goto ori_sw_err; >> - } >> - >> port->role_sw = fwnode_usb_role_switch_get(fwnode); >> if (IS_ERR(port->role_sw)) { >> dev_dbg(dev, "USB role switch handle not found.\n"); >> - goto role_sw_err; >> + return -ENODEV; >> } >> >> return 0; >> - >> -role_sw_err: >> - typec_switch_put(port->ori_sw); >> - port->ori_sw = NULL; >> -ori_sw_err: >> - typec_retimer_put(port->retimer); >> - port->retimer = NULL; >> -retimer_sw_err: >> - typec_mux_put(port->mux); >> - port->mux = NULL; >> -mux_err: >> - return -ENODEV; >> } >> >> static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num, >> @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, >> return 0; >> } >> >> +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port) >> +{ >> + struct typec_port *port = cros_port->port; >> + >> + if (IS_ERR_OR_NULL(port)) >> + return; >> + >> + cros_port->mux = port->mux; >> + cros_port->retimer = port->retimer; >> + cros_port->ori_sw = port->sw; >> +} >> >> Sorry, but this is not the right approach. These handles are >> ref-counted. We intentionally >> take extra references to these using the provided API. Please don't >> use raw references like this. >> >> Aah, didn't see they were getting ref-counted. Even so, I don't understand >> the part that why do we need to take extra references? Can't we not use the >> same reference to the nodes which were populated in the type-C driver? > No; they have to be refcounted (that's the way all firmware node handles > are used). Any "client" or "caller" that wants to use the fwnode handles > has to use the correct APIs. Refcounting is necessary for the fwnode/property > framework to ensure that there are no stale references for firmware device > nodes (needed to prevent leaks during device creation/removal). If the fwnode > framework doesn't know that you (driver) have a reference, it is free > to invalidate > that reference (and remove the underlying device) without you(driver) > knowing anything about it (I'm sure I'm oversimplifying a lot over > here, but that > is the gist AFAIK). > >> On another note, maybe remove these 3 handles from 'cros_typec_port' altogether, >> and use the handles from 'typec_port' inside the 'cros_typec_struct'? >> Other drivers >> in the type-C domain use the same concept AFAIK. They just have the 'typec_port' from >> which they use all the handles natively populated. > What drivers are these? No port driver should be using these handles without > refcounting them with the right API calls. > >> Further, the same 'fwnode' will be matched in 'typec_mux_match'/'typec_switch_match', >> irrespective of it being called from 'typec_register_port' or 'cros_typec_get_switch_handles'. >> Thus, mux->mux_dev->dev and sw->sw_dev->dev will represent the same 'dev' struct. >> Is there actually a point of referencing them again in cros-ec-typec? Even when we decrement the >> ref-counter, it will be decremented of the same 'dev'! >> Please correct me if I'm wrong. :) >> >> Also, if the fwnode_*_get() functions are failing, why are we to >> assume that they worked for the >> Type-C class code (that code uses the same functions) [1] ? >> >> They surely worked for the type-C class code, since if either of the 'mux' and 'switch' >> handles are ERR, it will return an error. cros-ec-typec will then try to probe again. >> Since the handling reaches after the 'typec_register_port', I think we can be sure that they >> had to be succeeded for the type-C class driver. > If those calls are succeeding when they are called within typec_register_port(), > there is no reason why they should fail when called from cros-ec-typec (we > are just taking references to existing handles after all). > I would suggest looking into why the calls to get the firmware handles > are failing > from the port driver. > > > On a unrelated note: Are you responding in plain text mode? I suspect > you may not be, > since the indentation of responses is getting messed up. > > BR, > > -Prashant
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 001b0de95a46..0265c0d38bd6 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -24,6 +24,8 @@ #include <linux/usb/typec_tbt.h> #include <linux/usb/role.h> +#include "../../usb/typec/class.h" + #define DRV_NAME "cros-ec-typec" #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \ @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap, return 0; } -static int cros_typec_get_switch_handles(struct cros_typec_port *port, - struct fwnode_handle *fwnode, - struct device *dev) +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port, + struct fwnode_handle *fwnode, + struct device *dev) { - port->mux = fwnode_typec_mux_get(fwnode, NULL); - if (IS_ERR(port->mux)) { - dev_dbg(dev, "Mux handle not found.\n"); - goto mux_err; - } - - port->retimer = fwnode_typec_retimer_get(fwnode); - if (IS_ERR(port->retimer)) { - dev_dbg(dev, "Retimer handle not found.\n"); - goto retimer_sw_err; - } - - port->ori_sw = fwnode_typec_switch_get(fwnode); - if (IS_ERR(port->ori_sw)) { - dev_dbg(dev, "Orientation switch handle not found.\n"); - goto ori_sw_err; - } - port->role_sw = fwnode_usb_role_switch_get(fwnode); if (IS_ERR(port->role_sw)) { dev_dbg(dev, "USB role switch handle not found.\n"); - goto role_sw_err; + return -ENODEV; } return 0; - -role_sw_err: - typec_switch_put(port->ori_sw); - port->ori_sw = NULL; -ori_sw_err: - typec_retimer_put(port->retimer); - port->retimer = NULL; -retimer_sw_err: - typec_mux_put(port->mux); - port->mux = NULL; -mux_err: - return -ENODEV; } static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num, @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec, return 0; } +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port) +{ + struct typec_port *port = cros_port->port; + + if (IS_ERR_OR_NULL(port)) + return; + + cros_port->mux = port->mux; + cros_port->retimer = port->retimer; + cros_port->ori_sw = port->sw; +} + static int cros_typec_init_ports(struct cros_typec_data *typec) { struct device *dev = typec->dev; @@ -422,9 +406,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec) goto unregister_ports; } - ret = cros_typec_get_switch_handles(cros_port, fwnode, dev); + cros_typec_copy_port_params(cros_port); + + ret = cros_typec_get_role_switch_handle(cros_port, fwnode, dev); if (ret) - dev_dbg(dev, "No switch control for port %d\n", + dev_dbg(dev, "No role-switch control for port %d\n", port_num); ret = cros_typec_register_port_altmodes(typec, port_num);