On Mon, 30 Oct 2023 at 12:27, Simon Ser <contact@xxxxxxxxxxx> wrote: > > On Monday, October 30th, 2023 at 11:22, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > > > 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. > > 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. typec:N, where N is an id of the port? -- With best wishes Dmitry
