[Freedreno] [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Simon Ser]

On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov at linaro.org> wrote:

> On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> heikki.krogerus at linux.intel.com wrote:
> 
> > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > 
> > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus at linux.intel.com wrote:
> > > 
> > > > Hi Dmitry,
> > > > 
> > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > 
> > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > export the corresponding port's name as the bridge's path field.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov at linaro.org
> > > > > ---
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > index b9d4856101c7..452dc6437861 100644
> > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > struct device_node *np = dev->of_node;
> > > > > const struct pmic_typec_resources *res;
> > > > > struct regmap *regmap;
> > > > > + char *tcpm_name;
> > > > > u32 base[2];
> > > > > int ret;
> > > > > 
> > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > mutex_init(&tcpm->lock);
> > > > > platform_set_drvdata(pdev, tcpm);
> > > > > 
> > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > -
> > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > if (!tcpm->tcpc.fwnode)
> > > > > return -EINVAL;
> > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > goto fwnode_remove;
> > > > > }
> > > > > 
> > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > 
> > > > So I got some questions and concerns off-list. This was one of the
> > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > afraid this is not acceptable.
> > > > 
> > > > You can't use device name as a reference, ever. There is no way to
> > > > guarantee that a device with a specific name is what you meant it to
> > > > be by the time it's accessed.
> > > 
> > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > device being destroyed behind userspace's back? At least for MSM this will be
> > > a huge problem already, with the bridge driver suddenly being removed.
> > 
> > The race exists even in your case, but please do not look at this as a
> > solution for only your platform.
> 
> Yes!
> 
> > This is about showing the user space a link between two device
> > instances (struct device), and the way you do that is by creating a
> > symlink. That way the kernel can take care of reference counting and
> > guarantee that the link always points to the correct device. That way
> > the link will also be always visible in user space without requirement
> > for any specific ABI like it should.
> 
> I'm fine with the symlink approach (and I'll follow that up after
> finishing the UCSI glue driver rework). However I feel several
> deficiencies there:
> 
> 1) It creates asymmetry with the DP MST case. Do we want to have
> symlinks in each of the MST connectors? Or do we follow the PATH
> properties in the MST case until we find the root port, which has
> symlink? Please note, that fine X11 renames DP MST connectors
> internally, so in xrandr I see DP-2-1, which maps to
> /sys/class/drm/card0-DP-2. Kind of hard to follow.
> 
> 2) For the multi-card cases, one has to remap the connector to the
> card index + connector path. And this needs to be done by all user
> space applications, which would like to present this kind of
> information for the user.
> 
> 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> and MHL used simple micro-USB connectors) there would be a completely
> new uABI. And any external port / wrapper will also require a
> completely new symlink kind.
> 
> I understand your concerns regarding mentioning external device in the
> PATH property. However I think we should make it easier for the
> userspace app to determine the kind of the external connector. What
> would you think about extending the PATH property in the following
> way:
> 
> For the USB-C connectors the PATH property has the value of
> `typec:cardN-DP-m` value. Userspace app can then look for the
> typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> find the information about the corresponding USB-C port.

This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
physical path of the connector. All of the parts of this string are
exposed elsewhere in the KMS uAPI already.

> In future this will allow us to define e.g.:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
> called /sys/class/drm/cardN-DP-m/micro_usb_connector.
> 
> Or:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
> /sys/class/drm/cardN-DP-m/mydp_connector.
> 
> 
> --
> With best wishes
> Dmitry