Re: [PATCH 03/10] drm/sysfs: Add mstpath attribute to connector devices

["Li, Sun peng (Leo)" <Sunpeng.Li@amd.com>]

On 2019-07-04 3:33 p.m., Ville Syrjälä wrote:
> On Thu, Jul 04, 2019 at 03:05:12PM -0400, sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> This can be used to create more descriptive symlinks for MST aux
>> devices. Consider the following udev rule:
>>
>> SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*",
>> 	SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}"
>>
>> The following symlinks will be created (depending on your MST topology):
>>
>> $ ls /dev/drm_dp_aux/by-path/
>> card0-mst:0-1  card0-mst:0-1-1  card0-mst:0-1-8  card0-mst:0-8
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>  drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index ad10810bc972..53fd1f71e900 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -236,16 +236,39 @@ static ssize_t modes_show(struct device *device,
>>  	return written;
>>  }
>>  
>> +static ssize_t mstpath_show(struct device *device,
>> +			    struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	struct drm_connector *connector = to_drm_connector(device);
>> +	ssize_t ret = 0;
>> +	char *path;
>> +
>> +	mutex_lock(&connector->dev->mode_config.mutex);
> 
> The blob is populated when the connector is created so I don't think
> this lock is actually doing anything for us.

Right, will drop this.

> 
> One would also hope that device_unregister() protects us from racing
> with the removal of the attribute. Eg. if you hold a file descriptor
> open to the sysfs file does device_unregister() block until the fd is
> closed?

For dpcd transactions, as long as the aux device is unregistered through
drm_dp_aux_unregister_devnode(), we should be good. There's an atomic_t
use_count that syncs against reads and writes.

Although I'm not sure what would happen if the device was ripped out
from underneath us, say, if the parent connector device is removed
before calling aux_unregister_devnode(). If this does happen, I think
it's more of an order-of-operations issue from the driver. V2 of patch 2
(and 7-10) actually addresses a specific occurence of this during driver
unload.

Regarding sysfs attrs, the kernfs underneath does seem to guarantee that
any r/w is completed before removal. See kernfs_drain(), called as part
of kernfs_remove().

Leo

> 
>> +	if (!connector->path_blob_ptr)
>> +		goto unlock;
>> +
>> +	path = connector->path_blob_ptr->data;
>> +	ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
>> +		       connector->dev->primary->index, path);
>> +
>> +unlock:
>> +	mutex_unlock(&connector->dev->mode_config.mutex);
>> +	return ret;
>> +}
>> +
>>  static DEVICE_ATTR_RW(status);
>>  static DEVICE_ATTR_RO(enabled);
>>  static DEVICE_ATTR_RO(dpms);
>>  static DEVICE_ATTR_RO(modes);
>> +static DEVICE_ATTR_RO(mstpath);
>>  
>>  static struct attribute *connector_dev_attrs[] = {
>>  	&dev_attr_status.attr,
>>  	&dev_attr_enabled.attr,
>>  	&dev_attr_dpms.attr,
>>  	&dev_attr_modes.attr,
>> +	&dev_attr_mstpath.attr,
>>  	NULL
>>  };
>>  
>> -- 
>> 2.22.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel