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

[Simon Ser]

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

> On Mon, 30 Oct 2023 at 12:13, Simon Ser contact at emersion.fr wrote:
> 
> > 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.
> 
> 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.

It sounds like an abuse of the PATH property. PATH is supposed to
contain an actual path, not just some connector type.

User-space can directly look into sysfs if it wants to figure out
whether a connector is typec.