LinuxLists.cc - [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev

[permalink] [raw]

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

Em Mon, 24 Sep 2018 11:10:31 +0100
David Howells <[email protected]> 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 <[email protected]>

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 <[email protected]>
> +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 <[email protected]>
> +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 <[email protected]>
> +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

2018-10-30 22:35:06

[permalink] [raw]

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

On Tue, Oct 30, 2018 at 11:03:19AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Sep 2018 11:10:31 +0100
> David Howells <[email protected]> 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:

Not all dvb devices have a mac address.

> >
> > [ 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 <[email protected]>
>
> Looks OK to me.
>
> Michael/Sean/Brad,
>
> Any comments? If not, I'll probably submit it this week upstream.

With this patch, with a usb Hauppauge Nova-T Stick I get:

$ tail /sys/class/dvb/*/dvb_*
==> /sys/class/dvb/dvb0.demux0/dvb_adapter <==
0

==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
00:00:00:00:00:00

==> /sys/class/dvb/dvb0.demux0/dvb_type <==
demux

==> /sys/class/dvb/dvb0.dvr0/dvb_adapter <==
0

==> /sys/class/dvb/dvb0.dvr0/dvb_mac <==
00:00:00:00:00:00

==> /sys/class/dvb/dvb0.dvr0/dvb_type <==
dvr

==> /sys/class/dvb/dvb0.frontend0/dvb_adapter <==
0

==> /sys/class/dvb/dvb0.frontend0/dvb_mac <==
00:00:00:00:00:00

==> /sys/class/dvb/dvb0.frontend0/dvb_type <==
frontend

==> /sys/class/dvb/dvb0.net0/dvb_adapter <==
0

==> /sys/class/dvb/dvb0.net0/dvb_mac <==
00:00:00:00:00:00

==> /sys/class/dvb/dvb0.net0/dvb_type <==
net

This would mean a stable name is based on a mac of 0, and there are many
more devices don't have a mac so they would all match this stable name.

Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
I think.

The dvb type and dvb adapter no is already present in the device name,
I'm not sure why this needs duplicating.

Sean

> > ---
> >
> > 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 <[email protected]>
> > +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 <[email protected]>
> > +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 <[email protected]>
> > +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

2018-10-31 00:47:13

[permalink] [raw]

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

Em Tue, 30 Oct 2018 22:32:50 +0000
Sean Young <[email protected]> escreveu:

Thanks for reviewing it!

> On Tue, Oct 30, 2018 at 11:03:19AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 24 Sep 2018 11:10:31 +0100
> > David Howells <[email protected]> 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:
>
> Not all dvb devices have a mac address.

True. Usually, devices without eeprom don't have, specially the too old ones.

On others, the MAC address only appear after the firmware is loaded.

> > >
> > > [ 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 <[email protected]>
> >
> > Looks OK to me.
> >
> > Michael/Sean/Brad,
> >
> > Any comments? If not, I'll probably submit it this week upstream.
>
> With this patch, with a usb Hauppauge Nova-T Stick I get:

Weird. Normally, Hauppauge devices have MAC address, as they all have
eeproms. On several models, the MAC is even printed at the label on
its back.

Perhaps the logic didn't wait for the firmware to load?

>
> $ tail /sys/class/dvb/*/dvb_*
> ==> /sys/class/dvb/dvb0.demux0/dvb_adapter <==
> 0
>
> ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> 00:00:00:00:00:00
>
> ==> /sys/class/dvb/dvb0.demux0/dvb_type <==
> demux
>
> ==> /sys/class/dvb/dvb0.dvr0/dvb_adapter <==
> 0
>
> ==> /sys/class/dvb/dvb0.dvr0/dvb_mac <==
> 00:00:00:00:00:00
>
> ==> /sys/class/dvb/dvb0.dvr0/dvb_type <==
> dvr
>
> ==> /sys/class/dvb/dvb0.frontend0/dvb_adapter <==
> 0
>
> ==> /sys/class/dvb/dvb0.frontend0/dvb_mac <==
> 00:00:00:00:00:00
>
> ==> /sys/class/dvb/dvb0.frontend0/dvb_type <==
> frontend
>
> ==> /sys/class/dvb/dvb0.net0/dvb_adapter <==
> 0
>
> ==> /sys/class/dvb/dvb0.net0/dvb_mac <==
> 00:00:00:00:00:00
>
> ==> /sys/class/dvb/dvb0.net0/dvb_type <==
> net
>
>
> This would mean a stable name is based on a mac of 0, and there are many
> more devices don't have a mac so they would all match this stable name.

It can only provide information when the device has it.

> Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> I think.

Hmm... do you mean that, if the mac is reported as 00:00:00:00:00,
then the sysfs node should not be exposed? Makes sense.

> The dvb type and dvb adapter no is already present in the device name,
> I'm not sure why this needs duplicating.

IMO, it helps to write udev rules if those information are exposed.

>
>
> Sean
>
> > > ---
> > >
> > > 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 <[email protected]>
> > > +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 <[email protected]>
> > > +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 <[email protected]>
> > > +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

Thanks,
Mauro

2018-10-31 08:44:43

[permalink] [raw]

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

On Tue, Oct 30, 2018 at 09:35:31PM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 30 Oct 2018 22:32:50 +0000
> Sean Young <[email protected]> escreveu:
>
> Thanks for reviewing it!
>
> > On Tue, Oct 30, 2018 at 11:03:19AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 24 Sep 2018 11:10:31 +0100
> > > David Howells <[email protected]> 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:
> >
> > Not all dvb devices have a mac address.
>
> True. Usually, devices without eeprom don't have, specially the too old ones.
>
> On others, the MAC address only appear after the firmware is loaded.
>
> > > >
> > > > [ 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 <[email protected]>
> > >
> > > Looks OK to me.
> > >
> > > Michael/Sean/Brad,
> > >
> > > Any comments? If not, I'll probably submit it this week upstream.
> >
> > With this patch, with a usb Hauppauge Nova-T Stick I get:
>
> Weird. Normally, Hauppauge devices have MAC address, as they all have
> eeproms. On several models, the MAC is even printed at the label on
> its back.
>
> Perhaps the logic didn't wait for the firmware to load?

This is an ancient dib0700 device; the firmware did load but there is no
mac.

> > $ tail /sys/class/dvb/*/dvb_*
> > ==> /sys/class/dvb/dvb0.demux0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.demux0/dvb_type <==
> > demux
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_type <==
> > dvr
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_type <==
> > frontend
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_type <==
> > net
> >
> >
> > This would mean a stable name is based on a mac of 0, and there are many
> > more devices don't have a mac so they would all match this stable name.
>
> It can only provide information when the device has it.
>
> > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > I think.
>
> Hmm... do you mean that, if the mac is reported as 00:00:00:00:00,
> then the sysfs node should not be exposed? Makes sense.

Yes, I do.

> > The dvb type and dvb adapter no is already present in the device name,
> > I'm not sure why this needs duplicating.
>
> IMO, it helps to write udev rules if those information are exposed.

That is true. There is support for regexs in udev, e.g.:

KERNEL=="dvd[0-9]*.demux.[0-9]*"

Having said that dvb_type does look a little nicer:

ATTR{dvb_type}=="demux"

Sean

2018-10-31 10:37:20

[permalink] [raw]

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

Sean Young <[email protected]> wrote:

> > > Devices have a MAC address available, which is printed during boot:
>
> Not all dvb devices have a mac address.

How do I tell? If it's all zeros it's not there?

> Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> I think.

I'm not sure that's possible within the core infrastructure. It's a class
attribute set when the class is created; I'm not sure it can be overridden on
a per-device basis.

Possibly the file could return "" or "none" in this case?

> The dvb type and dvb adapter no is already present in the device name,
> I'm not sure why this needs duplicating.

They can be used with ATTR{} in udev rules. I'm not clear that the name can.

> With this patch, with a usb Hauppauge Nova-T Stick I get:
> ...
> ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> 00:00:00:00:00:00

I can't say why that happens. I don't have access to this hardware. Should
it have a MAC address there? Is the MAC address getting stored in
dvbdev->adapter->proposed_mac? Maybe it's not getting read - on the card I
use it's read by the cx23885 driver... I think... The nova-t-usb2.c file
doesn't mention proposed_mac.

David

2018-10-31 10:49:51

[permalink] [raw]

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

On Wed, Oct 31, 2018 at 10:36:22AM +0000, David Howells wrote:
> Sean Young <[email protected]> wrote:
>
> > > > Devices have a MAC address available, which is printed during boot:
> >
> > Not all dvb devices have a mac address.
>
> How do I tell? If it's all zeros it's not there?

The mac gets populated through read_mac_address member of
dvb_usb_device_properties. If that's not called (or does not succeed), then
there is no mac address. I think you can safely assume that if it's all 0's
then it was not read.

> > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > I think.
>
> I'm not sure that's possible within the core infrastructure. It's a class
> attribute set when the class is created; I'm not sure it can be overridden on
> a per-device basis.
>
> Possibly the file could return "" or "none" in this case?

That's very ugly. Have a look at, for example, rc-core wakeup filters:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844

> > The dvb type and dvb adapter no is already present in the device name,
> > I'm not sure why this needs duplicating.
>
> They can be used with ATTR{} in udev rules. I'm not clear that the name can.

See my other email.

KERNEL=="dvb[0-9]+\.demux\.[0-9]+"

> > With this patch, with a usb Hauppauge Nova-T Stick I get:
> > ...
> > ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> > 00:00:00:00:00:00
>
> I can't say why that happens. I don't have access to this hardware. Should
> it have a MAC address there? Is the MAC address getting stored in
> dvbdev->adapter->proposed_mac? Maybe it's not getting read - on the card I
> use it's read by the cx23885 driver... I think... The nova-t-usb2.c file
> doesn't mention proposed_mac.

This is a dib0700-type device (much older).

Sean

2018-10-31 10:59:52

[permalink] [raw]

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

Sean Young <[email protected]> wrote:

> With this patch, with a usb Hauppauge Nova-T Stick I get:
>
> $ tail /sys/class/dvb/*/dvb_*
> ...
> ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> 00:00:00:00:00:00

I presume you're complaining, then, that the file exists in this instance
rather than it doesn't have the real MAC address in it?

> Having said that dvb_type does look a little nicer:
>
> ATTR{dvb_type}=="demux"

So I should keep that or drop that?

David

2018-10-31 11:01:56

[permalink] [raw]

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

Sean Young <[email protected]> wrote:

> > How do I tell? If it's all zeros it's not there?
>
> The mac gets populated through read_mac_address member of
> dvb_usb_device_properties.

That doesn't seem to be true for all drivers. The cx23885 driver does things
differently, for instance.

David

2018-10-31 11:20:07

[permalink] [raw]

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

Sean Young <[email protected]> wrote:

> > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > I think.
> >
> > I'm not sure that's possible within the core infrastructure. It's a class
> > attribute set when the class is created; I'm not sure it can be overridden on
> > a per-device basis.
> >
> > Possibly the file could return "" or "none" in this case?
>
> That's very ugly. Have a look at, for example, rc-core wakeup filters:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844

By analogy, then, I think the thing to do is to put something like struct
rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
and then the dvb_mac attribute in there during dvb_register_device() based on
whether or not the MAC address is not all zeros at that point.

David

2018-10-31 16:13:35

[permalink] [raw]

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

Em Wed, 31 Oct 2018 15:51:40 +0000
David Howells <[email protected]> escreveu:

> David Howells <[email protected]> wrote:
>
> > > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > > > I think.
> > > >
> > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > class attribute set when the class is created; I'm not sure it can be
> > > > overridden on a per-device basis.
> > > >
> > > > Possibly the file could return "" or "none" in this case?
> > >
> > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> >
> > By analogy, then, I think the thing to do is to put something like struct
> > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> > and then the dvb_mac attribute in there during dvb_register_device() based on
> > whether or not the MAC address is not all zeros at that point.
>
> Hmmm... This is trickier than it seems. At the point the device struct is
> registered, the MAC address hasn't yet been read:
>
> [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky S952 [card=50,autodetected]
> [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382 bytes)
> [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully identified
> [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.096936] cx23885 0000:06:00.0: DVB: registering adapter 2 frontend 0 (Montage Technology M88DS3103)...
> [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully identified
> [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.228190] cx23885 0000:06:00.0: DVB: registering adapter 3 frontend 0 (Montage Technology M88DS3103)...
> [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> [ 15.256004] cx23885: cx23885[1]/0: found at 0000:06:00.0, rev: 4, irq: 19, latency: 0, mmio: 0xf7a00000
>
> The device structs are registered at 15.096936 and 15.228190 and this is the
> point by which I think I have to set the device::groups pointer.
>
> However, the device isn't fully initialised at this point and the MAC address
> hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> second elapses between the device registration and the MAC being printed for
> each adapter.
>
> Any suggestions?

Yeah, I was afraid about that. At V4L2 core, what we do is that we first
do everything, including firmware load, and only after having everything
setup, we register the char device.

I suspect it should be possible to do that too at dvb-usb and dvb-usb-v2,
but some work would be needed.

>
> David
> ---
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index b7171bf094fb..edbfa5549994 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
> return 0;
> }
>
> +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 struct attribute *dvb_device_attrs[] = {
> + &dev_attr_dvb_mac.attr,
> + NULL
> +};
> +static const struct attribute_group dvb_device_attr_grp = {
> + .attrs = dvb_device_attrs,
> +};
> +
> int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> const struct dvb_device *template, void *priv,
> enum dvb_device_type type, int demux_sink_pads)
> @@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
>
> mutex_unlock(&dvbdev_register_lock);
>
> + if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
> + adap->proposed_mac[2] || adap->proposed_mac[3] ||
> + adap->proposed_mac[4] || adap->proposed_mac[5]) {
> + dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
> + dvbdev->sysfs_groups[1] = NULL;
> + adap->device->groups = dvbdev->sysfs_groups;
> + }
> +
> clsdev = device_create(dvb_class, adap->device,
> MKDEV(DVB_MAJOR, minor),
> dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id);
> @@ -1010,6 +1035,31 @@ 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_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_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);
> @@ -1050,6 +1100,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;
> diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
> index 881ca461b7bb..d6becdd2d56e 100644
> --- a/include/media/dvbdev.h
> +++ b/include/media/dvbdev.h
> @@ -127,6 +127,7 @@ struct dvb_adapter {
> *
> * @list_head: List head with all DVB devices
> * @fops: pointer to struct file_operations
> + * @sysfs_groups: Additional sysfs attributes
> * @adapter: pointer to the adapter that holds this device node
> * @type: type of the device, as defined by &enum dvb_device_type.
> * @minor: devnode minor number. Major number is always DVB_MAJOR.
> @@ -157,6 +158,7 @@ struct dvb_adapter {
> struct dvb_device {
> struct list_head list_head;
> const struct file_operations *fops;
> + const struct attribute_group *sysfs_groups[2];
> struct dvb_adapter *adapter;
> enum dvb_device_type type;
> int minor;

Thanks,
Mauro

2018-10-31 16:14:41

[permalink] [raw]

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

On Wed, Oct 31, 2018 at 03:51:40PM +0000, David Howells wrote:
> David Howells <[email protected]> wrote:
>
> > > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > > > I think.
> > > >
> > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > class attribute set when the class is created; I'm not sure it can be
> > > > overridden on a per-device basis.
> > > >
> > > > Possibly the file could return "" or "none" in this case?
> > >
> > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> >
> > By analogy, then, I think the thing to do is to put something like struct
> > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> > and then the dvb_mac attribute in there during dvb_register_device() based on
> > whether or not the MAC address is not all zeros at that point.
>
> Hmmm... This is trickier than it seems. At the point the device struct is
> registered, the MAC address hasn't yet been read:
>
> [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky S952 [card=50,autodetected]
> [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382 bytes)
> [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully identified
> [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.096936] cx23885 0000:06:00.0: DVB: registering adapter 2 frontend 0 (Montage Technology M88DS3103)...
> [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully identified
> [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.228190] cx23885 0000:06:00.0: DVB: registering adapter 3 frontend 0 (Montage Technology M88DS3103)...
> [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> [ 15.256004] cx23885: cx23885[1]/0: found at 0000:06:00.0, rev: 4, irq: 19, latency: 0, mmio: 0xf7a00000
>
> The device structs are registered at 15.096936 and 15.228190 and this is the
> point by which I think I have to set the device::groups pointer.
>
> However, the device isn't fully initialised at this point and the MAC address
> hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> second elapses between the device registration and the MAC being printed for
> each adapter.

device_create() will register the device in sysfs and send uevent. So, your
original udev rule/code will not work, since it always would read
a mac address of 0, as proposed_mac is not populated when the device is
announced. That is, unless udev is scheduled after the mac is read.

I think the device_add/device_create() which triggers the uevent should be
delayed until everything is available.

Sean

>
> Any suggestions?
>
> David
> ---
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index b7171bf094fb..edbfa5549994 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
> return 0;
> }
>
> +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 struct attribute *dvb_device_attrs[] = {
> + &dev_attr_dvb_mac.attr,
> + NULL
> +};
> +static const struct attribute_group dvb_device_attr_grp = {
> + .attrs = dvb_device_attrs,
> +};
> +
> int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> const struct dvb_device *template, void *priv,
> enum dvb_device_type type, int demux_sink_pads)
> @@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
>
> mutex_unlock(&dvbdev_register_lock);
>
> + if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
> + adap->proposed_mac[2] || adap->proposed_mac[3] ||
> + adap->proposed_mac[4] || adap->proposed_mac[5]) {

is_zero_ether_addr()

> + dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
> + dvbdev->sysfs_groups[1] = NULL;
> + adap->device->groups = dvbdev->sysfs_groups;
> + }
> +
> clsdev = device_create(dvb_class, adap->device,
> MKDEV(DVB_MAJOR, minor),
> dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id);
> @@ -1010,6 +1035,31 @@ 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_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_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);
> @@ -1050,6 +1100,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;
> diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
> index 881ca461b7bb..d6becdd2d56e 100644
> --- a/include/media/dvbdev.h
> +++ b/include/media/dvbdev.h
> @@ -127,6 +127,7 @@ struct dvb_adapter {
> *
> * @list_head: List head with all DVB devices
> * @fops: pointer to struct file_operations
> + * @sysfs_groups: Additional sysfs attributes
> * @adapter: pointer to the adapter that holds this device node
> * @type: type of the device, as defined by &enum dvb_device_type.
> * @minor: devnode minor number. Major number is always DVB_MAJOR.
> @@ -157,6 +158,7 @@ struct dvb_adapter {
> struct dvb_device {
> struct list_head list_head;
> const struct file_operations *fops;
> + const struct attribute_group *sysfs_groups[2];
> struct dvb_adapter *adapter;
> enum dvb_device_type type;
> int minor;

2018-10-31 16:30:22

[permalink] [raw]

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

Em Wed, 31 Oct 2018 16:13:00 +0000
Sean Young <[email protected]> escreveu:

> On Wed, Oct 31, 2018 at 03:51:40PM +0000, David Howells wrote:
> > David Howells <[email protected]> wrote:
> >
> > > > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > > > > I think.
> > > > >
> > > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > > class attribute set when the class is created; I'm not sure it can be
> > > > > overridden on a per-device basis.
> > > > >
> > > > > Possibly the file could return "" or "none" in this case?
> > > >
> > > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> > >
> > > By analogy, then, I think the thing to do is to put something like struct
> > > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> > > and then the dvb_mac attribute in there during dvb_register_device() based on
> > > whether or not the MAC address is not all zeros at that point.
> >
> > Hmmm... This is trickier than it seems. At the point the device struct is
> > registered, the MAC address hasn't yet been read:
> >
> > [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky S952 [card=50,autodetected]
> > [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> > [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382 bytes)
> > [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> > [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> > [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> > [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully identified
> > [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> > [ 15.096936] cx23885 0000:06:00.0: DVB: registering adapter 2 frontend 0 (Montage Technology M88DS3103)...
> > [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> > [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> > [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> > [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> > [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully identified
> > [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> > [ 15.228190] cx23885 0000:06:00.0: DVB: registering adapter 3 frontend 0 (Montage Technology M88DS3103)...
> > [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> > [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> > [ 15.256004] cx23885: cx23885[1]/0: found at 0000:06:00.0, rev: 4, irq: 19, latency: 0, mmio: 0xf7a00000
> >
> > The device structs are registered at 15.096936 and 15.228190 and this is the
> > point by which I think I have to set the device::groups pointer.
> >
> > However, the device isn't fully initialised at this point and the MAC address
> > hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> > set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> > second elapses between the device registration and the MAC being printed for
> > each adapter.
>
> device_create() will register the device in sysfs and send uevent. So, your
> original udev rule/code will not work, since it always would read
> a mac address of 0, as proposed_mac is not populated when the device is
> announced. That is, unless udev is scheduled after the mac is read.
>
> I think the device_add/device_create() which triggers the uevent should be
> delayed until everything is available.

Yes. For udev rules to work, the very last thing to do is to create
devices.

In practice, dvb-usb/dvb-usb-v2 should delay calling
dvb_register_device() until firmware is in warm state.

>
> Sean
>
> >
> > Any suggestions?
> >
> > David
> > ---
> > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> > index b7171bf094fb..edbfa5549994 100644
> > --- a/drivers/media/dvb-core/dvbdev.c
> > +++ b/drivers/media/dvb-core/dvbdev.c
> > @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
> > return 0;
> > }
> >
> > +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 struct attribute *dvb_device_attrs[] = {
> > + &dev_attr_dvb_mac.attr,
> > + NULL
> > +};
> > +static const struct attribute_group dvb_device_attr_grp = {
> > + .attrs = dvb_device_attrs,
> > +};
> > +
> > int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> > const struct dvb_device *template, void *priv,
> > enum dvb_device_type type, int demux_sink_pads)
> > @@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> >
> > mutex_unlock(&dvbdev_register_lock);
> >
> > + if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
> > + adap->proposed_mac[2] || adap->proposed_mac[3] ||
> > + adap->proposed_mac[4] || adap->proposed_mac[5]) {
>
> is_zero_ether_addr()
>
> > + dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
> > + dvbdev->sysfs_groups[1] = NULL;
> > + adap->device->groups = dvbdev->sysfs_groups;
> > + }
> > +
> > clsdev = device_create(dvb_class, adap->device,
> > MKDEV(DVB_MAJOR, minor),
> > dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id);
> > @@ -1010,6 +1035,31 @@ 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_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_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);
> > @@ -1050,6 +1100,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;
> > diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
> > index 881ca461b7bb..d6becdd2d56e 100644
> > --- a/include/media/dvbdev.h
> > +++ b/include/media/dvbdev.h
> > @@ -127,6 +127,7 @@ struct dvb_adapter {
> > *
> > * @list_head: List head with all DVB devices
> > * @fops: pointer to struct file_operations
> > + * @sysfs_groups: Additional sysfs attributes
> > * @adapter: pointer to the adapter that holds this device node
> > * @type: type of the device, as defined by &enum dvb_device_type.
> > * @minor: devnode minor number. Major number is always DVB_MAJOR.
> > @@ -157,6 +158,7 @@ struct dvb_adapter {
> > struct dvb_device {
> > struct list_head list_head;
> > const struct file_operations *fops;
> > + const struct attribute_group *sysfs_groups[2];
> > struct dvb_adapter *adapter;
> > enum dvb_device_type type;
> > int minor;

Thanks,
Mauro

2018-10-31 16:56:43

[permalink] [raw]

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

David Howells <[email protected]> wrote:

> > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > > I think.
> > >
> > > I'm not sure that's possible within the core infrastructure. It's a
> > > class attribute set when the class is created; I'm not sure it can be
> > > overridden on a per-device basis.
> > >
> > > Possibly the file could return "" or "none" in this case?
> >
> > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
>
> By analogy, then, I think the thing to do is to put something like struct
> rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> and then the dvb_mac attribute in there during dvb_register_device() based on
> whether or not the MAC address is not all zeros at that point.

Hmmm... This is trickier than it seems. At the point the device struct is
registered, the MAC address hasn't yet been read:

[ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky S952 [card=50,autodetected]
[ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
[ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382 bytes)
[ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
[ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
[ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
[ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully identified
[ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
[ 15.096936] cx23885 0000:06:00.0: DVB: registering adapter 2 frontend 0 (Montage Technology M88DS3103)...
[ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
[ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
[ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
[ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
[ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully identified
[ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
[ 15.228190] cx23885 0000:06:00.0: DVB: registering adapter 3 frontend 0 (Montage Technology M88DS3103)...
[ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
[ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
[ 15.256004] cx23885: cx23885[1]/0: found at 0000:06:00.0, rev: 4, irq: 19, latency: 0, mmio: 0xf7a00000

The device structs are registered at 15.096936 and 15.228190 and this is the
point by which I think I have to set the device::groups pointer.

However, the device isn't fully initialised at this point and the MAC address
hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
set right after lines 15.124665 and 15.255996. Interestingly, a third of a
second elapses between the device registration and the MAC being printed for
each adapter.

Any suggestions?

David
---
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index b7171bf094fb..edbfa5549994 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
return 0;
}

+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 struct attribute *dvb_device_attrs[] = {
+ &dev_attr_dvb_mac.attr,
+ NULL
+};
+static const struct attribute_group dvb_device_attr_grp = {
+ .attrs = dvb_device_attrs,
+};
+
int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
const struct dvb_device *template, void *priv,
enum dvb_device_type type, int demux_sink_pads)
@@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,

mutex_unlock(&dvbdev_register_lock);

+ if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
+ adap->proposed_mac[2] || adap->proposed_mac[3] ||
+ adap->proposed_mac[4] || adap->proposed_mac[5]) {
+ dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
+ dvbdev->sysfs_groups[1] = NULL;
+ adap->device->groups = dvbdev->sysfs_groups;
+ }
+
clsdev = device_create(dvb_class, adap->device,
MKDEV(DVB_MAJOR, minor),
dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id);
@@ -1010,6 +1035,31 @@ 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_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_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);
@@ -1050,6 +1100,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;
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index 881ca461b7bb..d6becdd2d56e 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -127,6 +127,7 @@ struct dvb_adapter {
*
* @list_head: List head with all DVB devices
* @fops: pointer to struct file_operations
+ * @sysfs_groups: Additional sysfs attributes
* @adapter: pointer to the adapter that holds this device node
* @type: type of the device, as defined by &enum dvb_device_type.
* @minor: devnode minor number. Major number is always DVB_MAJOR.
@@ -157,6 +158,7 @@ struct dvb_adapter {
struct dvb_device {
struct list_head list_head;
const struct file_operations *fops;
+ const struct attribute_group *sysfs_groups[2];
struct dvb_adapter *adapter;
enum dvb_device_type type;
int minor;

2018-10-31 19:11:09