| Message ID | 151559583569.13545.12649741692530472663.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi David, Em Wed, 10 Jan 2018 14:50:35 +0000 David Howells <dhowells@redhat.com> 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. > > The struct cx23885_tsport has a port number available, which is printed > during boot: > > [ 10.951517] DVBSky T982 port 1 MAC address: 00:17:42:54:09:87 > ... > [ 10.984875] DVBSky T982 port 2 MAC address: 00:17:42:54:09:88 > > To make it possible to distinguish these in udev, do the following steps: > > (1) Save the port number into struct dvb_adapter. > > (2) Provide sysfs attributes to export port number and also MAC address, > adapter number and type. There are other fields that could perhaps be > exported also. > > 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_port dvb_type > power subsystem uevent > [root@deneb ~]# cat /sys/class/dvb/dvb0.frontend0/dvb_* > 0 > 00:17:42:54:09:87 > 0 > frontend > > They can be used in udev rules: > > SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:17:42:54:09:87", 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:17:42:54:09:88", 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. Sorry for not looking on it earlier... Has been very busy those days, and the dvb sub-maintainer has not been responsive lately, due to some personal issues. > > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > drivers/media/dvb-core/dvbdev.c | 46 +++++++++++++++++++++++++++++++ > drivers/media/dvb-core/dvbdev.h | 2 + > drivers/media/pci/cx23885/cx23885-dvb.c | 2 + > 3 files changed, 50 insertions(+) > > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c > index 060c60ddfcc3..b3aa5ae3d57f 100644 > --- a/drivers/media/dvb-core/dvbdev.c > +++ b/drivers/media/dvb-core/dvbdev.c > @@ -941,6 +941,51 @@ int dvb_usercopy(struct file *file, > return err; > } > > +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_port_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->port_num); > +} > +static DEVICE_ATTR_RO(dvb_port); > + > +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_port.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); > @@ -981,6 +1026,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; The patch itself looks good, but I'm not seeing any documentation. You should likely add something to Documentation/ABI and to the DVB uAPI (Documentation/media/uapi/dvb). > diff --git a/drivers/media/dvb-core/dvbdev.h b/drivers/media/dvb-core/dvbdev.h > index bbc1c20c0529..1d5a170e279a 100644 > --- a/drivers/media/dvb-core/dvbdev.h > +++ b/drivers/media/dvb-core/dvbdev.h > @@ -83,6 +83,7 @@ struct dvb_frontend; > * @device_list: List with the DVB devices > * @name: Name of the adapter > * @proposed_mac: proposed MAC address for the adapter > + * @port_num: Port number for multi-adapter devices > * @priv: private data > * @device: pointer to struct device > * @module: pointer to struct module > @@ -103,6 +104,7 @@ struct dvb_adapter { > struct list_head device_list; > const char *name; > u8 proposed_mac [6]; > + u8 port_num; > void* priv; > > struct device *device; > diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c b/drivers/media/pci/cx23885/cx23885-dvb.c > index e795ddeb7fe2..19c72c66d7e0 100644 > --- a/drivers/media/pci/cx23885/cx23885-dvb.c > +++ b/drivers/media/pci/cx23885/cx23885-dvb.c > @@ -1217,6 +1217,8 @@ static int dvb_register(struct cx23885_tsport *port) > /* Sets the gate control callback to be used by i2c command calls */ > port->gate_ctrl = cx23885_dvb_gate_ctrl; > > + port->frontends.adapter.port_num = port->nr; > + Doing it for each multi-adapter device is something that bothers me. The better would be if we could move this to the DVB Kernel, in order to not need to check/fix every driver. If, otherwise, this is not possible, then we need a patch fixing port_num for all drivers that support multiple adapters. Also, the risk of forgetting it seems high. So, perhaps we should add a new parameter to some function (like at dvb_register_device or at dvb_register_frontend), in order to make the port number a mandatory attribute. > /* init frontend */ > switch (dev->board) { > case CX23885_BOARD_HAUPPAUGE_HVR1250: > Thanks, Mauro
Mauro Carvalho Chehab <mchehab@kernel.org> wrote: > > + dvb_class->dev_groups = dvb_class_groups, > > dvb_class->dev_uevent = dvb_uevent; > > dvb_class->devnode = dvb_devnode; > > return 0; > > The patch itself looks good, but I'm not seeing any documentation. I should probably add something to Documentation/media/dvb-drivers/udev.rst > You should likely add something to Documentation/ABI Any suggestions as to where to add stuff in there? The README there leaves a lot to be desired as to how to name elements - for instance, DVB devices can be seen through /sys/class/ and /sys/devices/. I could put it in sys-class-dvb or sys-devices-dvb - or, arguably, both. > and to the DVB uAPI (Documentation/media/uapi/dvb). Likewise, any suggestion as to where in here? As far as I can tell, the docs here don't currently mention sysfs at all. I'm guessing I'll need to create a file specifically to talk about how to use this stuff with udev. > > + port->frontends.adapter.port_num = port->nr; > > + > > Doing it for each multi-adapter device is something that bothers > me. The better would be if we could move this to the DVB Kernel, > in order to not need to check/fix every driver. I'm not sure how achievable that is: *port in this case is a private cx23885-specific structure object. > If, otherwise, this is not possible, then we need a patch fixing port_num > for all drivers that support multiple adapters. > > Also, the risk of forgetting it seems high. So, perhaps we should > add a new parameter to some function (like at dvb_register_device > or at dvb_register_frontend), in order to make the port number > a mandatory attribute. Hmmm... The cx23885 driver doesn't call either of these functions as far as I can tell - at least, not directly. Maybe by vb2_dvb_register_bus()? Note that these attribute files appear for the demux, dvr and net directories as well as for the frontend. Hmmm... further, the port number is no longer getting through and all adapters are showing port 0. The MAC address works, though. Maybe I should drop the port number. David
Em Thu, 08 Mar 2018 23:35:45 +0000 David Howells <dhowells@redhat.com> escreveu: > Mauro Carvalho Chehab <mchehab@kernel.org> wrote: > > > > + dvb_class->dev_groups = dvb_class_groups, > > > dvb_class->dev_uevent = dvb_uevent; > > > dvb_class->devnode = dvb_devnode; > > > return 0; > > > > The patch itself looks good, but I'm not seeing any documentation. > > I should probably add something to Documentation/media/dvb-drivers/udev.rst Makes sense. > > You should likely add something to Documentation/ABI > > Any suggestions as to where to add stuff in there? The README there leaves > a lot to be desired as to how to name elements - for instance, DVB devices can > be seen through /sys/class/ and /sys/devices/. > > I could put it in sys-class-dvb or sys-devices-dvb - or, arguably, both. Good point. I don't have a strong opinion, but, as we're using /sys/class for remote controller's elements[1], I would place DVB stuff there also. [1] Documentation/ABI/testing/sysfs-class-rc > > and to the DVB uAPI (Documentation/media/uapi/dvb). > > Likewise, any suggestion as to where in here? As far as I can tell, the docs > here don't currently mention sysfs at all. I'm guessing I'll need to create a > file specifically to talk about how to use this stuff with udev. Yes. The docs don't mention simply because, right now, there's nothing special on sysfs. > > > + port->frontends.adapter.port_num = port->nr; > > > + > > > > Doing it for each multi-adapter device is something that bothers > > me. The better would be if we could move this to the DVB Kernel, > > in order to not need to check/fix every driver. > > I'm not sure how achievable that is: *port in this case is a private > cx23885-specific structure object. Yes, but other drivers that support multiple frontends store it somewhere. Yet, maybe we could keep this out of the first version. > > If, otherwise, this is not possible, then we need a patch fixing port_num > > for all drivers that support multiple adapters. > > > > Also, the risk of forgetting it seems high. So, perhaps we should > > add a new parameter to some function (like at dvb_register_device > > or at dvb_register_frontend), in order to make the port number > > a mandatory attribute. > > Hmmm... The cx23885 driver doesn't call either of these functions as far as I > can tell - at least, not directly. Maybe by vb2_dvb_register_bus()? Yeah, some hybrid drivers typically use a helper module to handle dvb registration, either at VB or VB2. As this has to be generic enough, if we add, we'll likely need to place the port number at dvb_register_device()/dvb_register_frontend() and then change vb2_dvb_register_bus() accordingly. Hmm... that's said, I guess we can get rid of the VB 1 version, as I'm not seeing any driver calling videobuf_dvb_register_bus() anymore. I'll double check and write a patch to get rid of it, if possible. > > Note that these attribute files appear for the demux, dvr and net directories > as well as for the frontend. > > Hmmm... further, the port number is no longer getting through and all adapters > are showing port 0. Weird. > The MAC address works, though. Maybe I should drop the > port number. Yeah, let's drop it for the first version. It can be added later if needed. Thanks, Mauro
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 060c60ddfcc3..b3aa5ae3d57f 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -941,6 +941,51 @@ int dvb_usercopy(struct file *file, return err; } +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_port_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->port_num); +} +static DEVICE_ATTR_RO(dvb_port); + +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_port.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); @@ -981,6 +1026,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/drivers/media/dvb-core/dvbdev.h b/drivers/media/dvb-core/dvbdev.h index bbc1c20c0529..1d5a170e279a 100644 --- a/drivers/media/dvb-core/dvbdev.h +++ b/drivers/media/dvb-core/dvbdev.h @@ -83,6 +83,7 @@ struct dvb_frontend; * @device_list: List with the DVB devices * @name: Name of the adapter * @proposed_mac: proposed MAC address for the adapter + * @port_num: Port number for multi-adapter devices * @priv: private data * @device: pointer to struct device * @module: pointer to struct module @@ -103,6 +104,7 @@ struct dvb_adapter { struct list_head device_list; const char *name; u8 proposed_mac [6]; + u8 port_num; void* priv; struct device *device; diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c b/drivers/media/pci/cx23885/cx23885-dvb.c index e795ddeb7fe2..19c72c66d7e0 100644 --- a/drivers/media/pci/cx23885/cx23885-dvb.c +++ b/drivers/media/pci/cx23885/cx23885-dvb.c @@ -1217,6 +1217,8 @@ static int dvb_register(struct cx23885_tsport *port) /* Sets the gate control callback to be used by i2c command calls */ port->gate_ctrl = cx23885_dvb_gate_ctrl; + port->frontends.adapter.port_num = port->nr; + /* init frontend */ switch (dev->board) { case CX23885_BOARD_HAUPPAUGE_HVR1250:
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. The struct cx23885_tsport has a port number available, which is printed during boot: [ 10.951517] DVBSky T982 port 1 MAC address: 00:17:42:54:09:87 ... [ 10.984875] DVBSky T982 port 2 MAC address: 00:17:42:54:09:88 To make it possible to distinguish these in udev, do the following steps: (1) Save the port number into struct dvb_adapter. (2) Provide sysfs attributes to export port number and also MAC address, adapter number and type. There are other fields that could perhaps be exported also. 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_port dvb_type power subsystem uevent [root@deneb ~]# cat /sys/class/dvb/dvb0.frontend0/dvb_* 0 00:17:42:54:09:87 0 frontend They can be used in udev rules: SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1", ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982", ATTR{dvb_mac}=="00:17:42:54:09:87", 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:17:42:54:09:88", 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@redhat.com> --- drivers/media/dvb-core/dvbdev.c | 46 +++++++++++++++++++++++++++++++ drivers/media/dvb-core/dvbdev.h | 2 + drivers/media/pci/cx23885/cx23885-dvb.c | 2 + 3 files changed, 50 insertions(+)