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

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 12:13, Simon Ser <contact@xxxxxxxxxxx> wrote:
>
> On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > heikki.krogerus@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxx
> > > > > > ---
> > > > > > 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.

True. It seems I mixed KMS and xrandr clients in my head.
Just 'typec:' then? This way userspace will still know that it is a
USB-C connector (and can stop there) and if it needs more information
(e.g. physical location) it can further look for the symlink in the
sysfs.

>
> > 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



-- 
With best wishes
Dmitry