Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev

[Mauro Carvalho Chehab]

Em Mon, 24 Sep 2018 11:10:31 +0100
David Howells <dhowells@xxxxxxxxxx> escreveu:

> Some devices, such as the DVBSky S952 and T982 cards, are dual port cards
> that provide two cx23885 devices on the same PCI device, which means the
> attributes available for writing udev rules are exactly the same, apart
> from the adapter number.  Unfortunately, the adapter numbers are dependent
> on the order in which things are initialised, so this can change over
> different releases of the kernel.
> 
> Devices have a MAC address available, which is printed during boot:
> 
> 	[   10.951517] DVBSky T982 port 1 MAC address: 00:11:22:33:44:55
> 	...
> 	[   10.984875] DVBSky T982 port 2 MAC address: 00:11:22:33:44:56
> 
> To make it possible to distinguish these in udev, provide sysfs attributes
> to make the MAC address, adapter number and type available.  There are
> other fields that could perhaps be exported also.  In particular, it would
> be nice to provide the port number, but somehow that doesn't manage to
> propagate through the labyrinthine initialisation process.
> 
> The new sysfs attributes can be seen from userspace as:
> 
> 	[root@deneb ~]# ls /sys/class/dvb/dvb0.frontend0/
> 	dev  device  dvb_adapter  dvb_mac  dvb_type
> 	power  subsystem  uevent
> 	[root@deneb ~]# cat /sys/class/dvb/dvb0.frontend0/dvb_*
> 	0
> 	00:11:22:33:44:55
> 	frontend
> 
> They can be used in udev rules:
> 
> 	SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:11:22:33:44:55", PROGRAM="/bin/sh -c 'K=%k; K=$${K#dvb}; printf dvb/adapter9820/%%s $${K#*.}'", SYMLINK+="%c"
> 	SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:11.22.33.44.56", PROGRAM="/bin/sh -c 'K=%k; K=$${K#dvb}; printf dvb/adapter9821/%%s $${K#*.}'", SYMLINK+="%c"
> 
> where the match is made with ATTR{dvb_mac} or similar.  The rules above
> make symlinks from /dev/dvb/adapter982/* to /dev/dvb/adapterXX/*.
> 
> Note that binding the dvb-net device to a network interface and changing it
> there does not reflect back into the the dvb_adapter struct and doesn't
> change the MAC address here.  This means that a system with two identical
> cards in it may need to distinguish them by some other means than MAC
> address.
> 
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>

Looks OK to me.

Michael/Sean/Brad,

Any comments? If not, I'll probably submit it this week upstream.


> ---
> 
>  Documentation/ABI/testing/sysfs-class-dvb     |   29 +++++++++++
>  Documentation/media/dvb-drivers/udev.rst      |   29 +++++++++++
>  Documentation/media/uapi/dvb/intro.rst        |    7 +++
>  Documentation/media/uapi/dvb/stable_names.rst |   66 +++++++++++++++++++++++++
>  drivers/media/dvb-core/dvbdev.c               |   36 ++++++++++++++
>  5 files changed, 167 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-dvb
>  create mode 100644 Documentation/media/uapi/dvb/stable_names.rst
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-dvb b/Documentation/ABI/testing/sysfs-class-dvb
> new file mode 100644
> index 000000000000..09e3be329c92
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-dvb
> @@ -0,0 +1,29 @@
> +What:		/sys/class/dvb/.../dvb_adapter
> +Date:		September 2018
> +KernelVersion:	4.20
> +Contact:	David Howells <dhowells@xxxxxxxxxx>
> +Description:
> +		This displays the assigned adapter number of a DVB device.
> +
> +What:		/sys/class/dvb/.../dvb_mac
> +Date:		September 2018
> +KernelVersion:	4.20
> +Contact:	David Howells <dhowells@xxxxxxxxxx>
> +Description:
> +		This displays the mac address of a DVB device.  This can be
> +		used by udev to name stable device files for DVB devices and
> +		avoid problems with changes in the order of device
> +		initialisation changing the assigned device numbers.  See:
> +
> +			Documentation/media/dvb-drivers/udev.rst
> +			Documentation/media/uapi/dvb/stable_names.rst
> +
> +		for information on how to actually do this.
> +
> +What:		/sys/class/dvb/.../dvb_type
> +Date:		September 2018
> +KernelVersion:	4.20
> +Contact:	David Howells <dhowells@xxxxxxxxxx>
> +Description:
> +		This displays the object type of a DVB device interface, such
> +		as "frontend" or "demux".
> diff --git a/Documentation/media/dvb-drivers/udev.rst b/Documentation/media/dvb-drivers/udev.rst
> index 7d7d5d82108a..df754312f1f4 100644
> --- a/Documentation/media/dvb-drivers/udev.rst
> +++ b/Documentation/media/dvb-drivers/udev.rst
> @@ -59,3 +59,32 @@ have a look at "man udev".
>  For every device that registers to the sysfs subsystem with a "dvb" prefix,
>  the helper script /etc/udev/scripts/dvb.sh is invoked, which will then
>  create the proper device node in your /dev/ directory.
> +
> +2. A DVB device's adapter number, type and MAC addresses are exposed through
> +the sysfs interface as files dvb_adapter, dvb_type and dvb_mac in the various
> +dvb object directories, e.g. /sys/class/dvb/dvb0.demux0/dvb_mac.
> +
> +These can be used to influence the binding of devices to names in /dev to avoid
> +problems when the order in which names are assigned changes.  This is of
> +particular interest when you have, say, a PCI card with multiple identical
> +devices on board under the same PCI function slot.  The only way to distinguish
> +them is either by the DVB port number or the DVB MAC address.
> +
> +To make use of this with udev, a rule needs to be emplaced in a file under
> +/etc/udev/rules.d/ that has an appropriate ATTR{} clause in it.  Something like
> +the following, for example::
> +
> +	SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:11:22:33:44:55", PROGRAM="/bin/sh -c 'K=%k; K=$${K#dvb}; printf dvb/adapter9820/%%s $${K#*.}'", SYMLINK+="%c"
> +
> +Note the 'ATTR{dvb_mac}' clause that indicates the MAC address to look for.
> +This should be different for every device, even if the devices are otherwise
> +identical.  The other ATTR{} clauses in this example refer to PCI parameters.
> +
> +This example generates a directory called /dev/dvb/adapter9820/ and places
> +symlinks in it to the device files under the appropriate /dev/dvb/adapterX/
> +directory - whatever X happens to be today.
> +
> +The generated name is then stable and can be relied on by programs that need to
> +pick it up without user interaction.
> +
> +Note that this facility does not exist in v4.19 kernels and earlier.
> diff --git a/Documentation/media/uapi/dvb/intro.rst b/Documentation/media/uapi/dvb/intro.rst
> index 79b4d0e4e920..074fb3b3ee21 100644
> --- a/Documentation/media/uapi/dvb/intro.rst
> +++ b/Documentation/media/uapi/dvb/intro.rst
> @@ -153,6 +153,13 @@ where ``N`` enumerates the Digital TV cards in a system starting fromÂ0, and
>  fromÂ0, too. We will omit the â``/dev/dvb/adapterN/``\ â in the further
>  discussion of these devices.
>  
> +Note that the automatic numbering of adapters isn't stable and may vary
> +depending on changes to the order in which devices are initialised, both in
> +the order in which individual devices get initialised and also the order in
> +which subdevices get initialised (e.g. a PCI card with multiple identical DVB
> +devices attached to the same PCI function).  :ref:`stable_names` shows use
> +udev rules to create stable names.
> +
>  More details about the data structures and function calls of all the
>  devices are described in the following chapters.
>  
> diff --git a/Documentation/media/uapi/dvb/stable_names.rst b/Documentation/media/uapi/dvb/stable_names.rst
> new file mode 100644
> index 000000000000..1b5dc5171ee3
> --- /dev/null
> +++ b/Documentation/media/uapi/dvb/stable_names.rst
> @@ -0,0 +1,66 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _stable_names:
> +
> +*********************************
> +Creating stable device file names
> +*********************************
> +
> +From time to time the order in which the Linux kernel initialises devices and
> +initialises subdevices within those devices has changed.  This can cause the
> +assignment of user-visible device numbers to devices to fluctuate - leading to
> +the failure of services to operate correctly in non-obvious ways when multiple,
> +otherwise identical devices are available in a system.
> +
> +To counteract this, udev rules can be defined that map devices onto stable
> +names.  This must, however, be done in relation to attributes of a device that
> +don't vary, such as the MAC address.
> +
> +Take, for example, a PCI DVB card that has two identical DVB devices attached
> +to the same PCI function.  The devices cannot be distinguished on PCI
> +parameters and the DVB port number - which could otherwise distinguish these
> +subdevices - is not easily accessible by userspace.
> +
> +The MAC address, however, *is* made available, and this is supposed to be
> +unique to each individual DVB device, and won't vary even if the device is
> +moved to another slot.  This is exported to userspace through sysfs.  It can
> +be found by looking in the dvb_mac file that can be found in a device
> +interface's directory, for example:
> +
> +	/sys/class/dvb/dvb0.demux0/dvb_mac
> +
> +Two other files can be found there that export the adapter number and the
> +interface type:
> +
> +	/sys/class/dvb/dvb0.demux0/dvb_adapter
> +	/sys/class/dvb/dvb0.demux0/dvb_type
> +
> +Note that the two numbers in the path are assigned based on the order in which
> +the devices are registered with the core code, and not necessarily on the
> +physical arrangement of the device - and thus should not be considered stable.
> +
> +
> +The creation of stable names can be done by writing rules for udev to match on
> +the MAC addresses of the devices.  Rules needs to be placed in a file in the
> +/etc/udev/rules.d/ directory for udev to pick up.  They need appropriate
> +ATTR{} clauses to specify the attribute matches to make.  Any of the above
> +mentioned files can be used.  For example::
> +
> +	SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:11:22:33:44:55", PROGRAM="/bin/sh -c 'K=%k; K=$${K#dvb}; printf dvb/adapter9820/%%s $${K#*.}'", SYMLINK+="%c"
> +	SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:11.22.33.44.56", PROGRAM="/bin/sh -c 'K=%k; K=$${K#dvb}; printf dvb/adapter9821/%%s $${K#*.}'", SYMLINK+="%c"
> +
> +In each of these example rules, the first three ATTR{} clauses specify the PCI
> +card to match - in this case the same DVBsky T982 dual T2 receiver card.  The
> +ATTR{dvb_mac} attribute in each specifies the card MAC address of that
> +receiver unit (the name of the attribute refers to the name of sysfs file to
> +read).
> +
> +This example generates a pair of directories called /dev/dvb/adapter9820/ and
> +/dev/dvb/adapter9821/ and places in each symlinks to the device files under
> +the appropriate /dev/dvb/adapterX/ and /dev/dvb/adapterY/ directories -
> +whatever X and Y happens to be today.
> +
> +The generated names are then stable and can be relied on by programs that need
> +to pick it up without user interaction.
> +
> +Note that this facility does not exist in v4.19 kernels and earlier.
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index 64d6793674b9..41be3ba66341 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -995,6 +995,41 @@ void dvb_module_release(struct i2c_client *client)
>  EXPORT_SYMBOL_GPL(dvb_module_release);
>  #endif
>  
> +static ssize_t dvb_adapter_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct dvb_device *dvbdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", dvbdev->adapter->num);
> +}
> +static DEVICE_ATTR_RO(dvb_adapter);
> +
> +static ssize_t dvb_mac_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct dvb_device *dvbdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%pM\n", dvbdev->adapter->proposed_mac);
> +}
> +static DEVICE_ATTR_RO(dvb_mac);
> +
> +static ssize_t dvb_type_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct dvb_device *dvbdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", dnames[dvbdev->type]);
> +}
> +static DEVICE_ATTR_RO(dvb_type);
> +
> +static struct attribute *dvb_class_attrs[] = {
> +	&dev_attr_dvb_adapter.attr,
> +	&dev_attr_dvb_mac.attr,
> +	&dev_attr_dvb_type.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(dvb_class);
> +
>  static int dvb_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct dvb_device *dvbdev = dev_get_drvdata(dev);
> @@ -1035,6 +1070,7 @@ static int __init init_dvbdev(void)
>  		retval = PTR_ERR(dvb_class);
>  		goto error;
>  	}
> +	dvb_class->dev_groups = dvb_class_groups,
>  	dvb_class->dev_uevent = dvb_uevent;
>  	dvb_class->devnode = dvb_devnode;
>  	return 0;
> 



Thanks,
Mauro