Populating netdev::dev_id for udev discrimination

Populating netdev::dev_id for udev discrimination

[Christopher R. Baker]

Hi All,

I'm new to the list, and have poked around on gmane enough to convince
myself that this hasn't been covered before, but if I've missed
something in this vein, apologies in advance.

My objective is to be able to totally discriminate CAN ports on
multi-port cards via udev so as to rename them to semantically
interesting/unique names for my system (e.g., "ecuCAN" and "auxCAN"
instead of "can0" and "can1").

udevadm info gives me the KERNELS=... incantation to match the pci bus
address, but there are no other differences in the udev listing for my
various CAN ports (at least for my peak_pci device)

Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
the intention of discriminating between physical ports that "share the
same link layer" (from netdev.h) or "share the same MAC address" (from
various other mailing lists).

The following patch assigns the dev_id field to match the channel number
on all multi-channel cards I could identify in a fresh git pull under
drivers/net/can.  I can only test my two-port Peak PCI card, but it
works as expected: ATTRS{dev_id} now expresses the port number and my
udev rules now unambiguously pick out and rename my individual CAN
ports.

The drivers for other cards I have touched at least compile, but should
certainly be tested before deploying this patch.

-Chris

diff --git a/drivers/net/can/sja1000/ems_pci.c b/drivers/net/can/sja1000/ems_pci.c
index d790b87..fd13dbf 100644
--- a/drivers/net/can/sja1000/ems_pci.c
+++ b/drivers/net/can/sja1000/ems_pci.c
@@ -323,6 +323,7 @@ static int ems_pci_add_card(struct pci_dev *pdev,
 			priv->cdr = EMS_PCI_CDR;
 
 			SET_NETDEV_DEV(dev, &pdev->dev);
+			dev->dev_id = i;
 
 			if (card->version == 1)
 				/* reset int flag of pita */
diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c
index 9e535f2..381de99 100644
--- a/drivers/net/can/sja1000/ems_pcmcia.c
+++ b/drivers/net/can/sja1000/ems_pcmcia.c
@@ -211,6 +211,7 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base)
 		priv = netdev_priv(dev);
 		priv->priv = card;
 		SET_NETDEV_DEV(dev, &pdev->dev);
+		dev->dev_id = i;
 
 		priv->irq_flags = IRQF_SHARED;
 		dev->irq = pdev->irq;
diff --git a/drivers/net/can/sja1000/kvaser_pci.c b/drivers/net/can/sja1000/kvaser_pci.c
index c96eb14..23b8e13 100644
--- a/drivers/net/can/sja1000/kvaser_pci.c
+++ b/drivers/net/can/sja1000/kvaser_pci.c
@@ -270,6 +270,7 @@ static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
 		 priv->reg_base, board->conf_addr, dev->irq);
 
 	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev->dev_id = channel;
 
 	/* Register SJA1000 device */
 	err = register_sja1000dev(dev);
diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c
index 065ca49..c540e3d 100644
--- a/drivers/net/can/sja1000/peak_pci.c
+++ b/drivers/net/can/sja1000/peak_pci.c
@@ -642,6 +642,7 @@ static int peak_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		icr |= chan->icr_mask;
 
 		SET_NETDEV_DEV(dev, &pdev->dev);
+		dev->dev_id = i;
 
 		/* Create chain of SJA1000 devices */
 		chan->prev_dev = pci_get_drvdata(pdev);
diff --git a/drivers/net/can/sja1000/peak_pcmcia.c b/drivers/net/can/sja1000/peak_pcmcia.c
index f7ad754..dd56133 100644
--- a/drivers/net/can/sja1000/peak_pcmcia.c
+++ b/drivers/net/can/sja1000/peak_pcmcia.c
@@ -550,6 +550,7 @@ static int pcan_add_channels(struct pcan_pccard *card)
 		priv = netdev_priv(netdev);
 		priv->priv = card;
 		SET_NETDEV_DEV(netdev, &pdev->dev);
+		netdev->dev_id = i;
 
 		priv->irq_flags = IRQF_SHARED;
 		netdev->irq = pdev->irq;
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index fbb61a0..ec39b7c 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -587,6 +587,7 @@ static int plx_pci_add_card(struct pci_dev *pdev,
 			priv->cdr = ci->cdr;
 
 			SET_NETDEV_DEV(dev, &pdev->dev);
+			dev->dev_id = i;
 
 			/* Register SJA1000 device */
 			err = register_sja1000dev(dev);
diff --git a/drivers/net/can/softing/softing_main.c b/drivers/net/can/softing/softing_main.c
index 9ea0dcd..c79dad1 100644
--- a/drivers/net/can/softing/softing_main.c
+++ b/drivers/net/can/softing/softing_main.c
@@ -832,6 +832,7 @@ static int softing_pdev_probe(struct platform_device *pdev)
 			ret = -ENOMEM;
 			goto netdev_failed;
 		}
+		netdev->dev_id = j;
 		priv = netdev_priv(card->net[j]);
 		priv->index = j;
 		ret = softing_netdev_register(netdev);
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 7fbe859..65ea1058 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -1024,6 +1024,7 @@ static int esd_usb2_probe_one_net(struct usb_interface *intf, int index)
 	netdev->netdev_ops = &esd_usb2_netdev_ops;
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
+	netdev->dev_id = index;
 
 	err = register_candev(netdev);
 	if (err) {
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index e77d110..880c0257 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1529,6 +1529,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	netdev->netdev_ops = &kvaser_usb_netdev_ops;
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
+	netdev->dev_id = channel;
 
 	dev->nets[channel] = priv;
 
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 0b7a4c3..89f560b 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -769,6 +769,7 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 	usb_set_intfdata(intf, dev);
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
+	netdev->dev_id = ctrl_idx;
 
 	err = register_candev(netdev);
 	if (err) {

Re: Populating netdev::dev_id for udev discrimination

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

On 03/08/2014 05:00 PM, Christopher R. Baker wrote:
> I'm new to the list, and have poked around on gmane enough to convince
> myself that this hasn't been covered before, but if I've missed
> something in this vein, apologies in advance.

You are the first one to address this issue. \o/

> My objective is to be able to totally discriminate CAN ports on
> multi-port cards via udev so as to rename them to semantically
> interesting/unique names for my system (e.g., "ecuCAN" and "auxCAN"
> instead of "can0" and "can1").
>
> udevadm info gives me the KERNELS=... incantation to match the pci bus
> address, but there are no other differences in the udev listing for my
> various CAN ports (at least for my peak_pci device)
> 
> Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
> the intention of discriminating between physical ports that "share the
> same link layer" (from netdev.h) or "share the same MAC address" (from
> various other mailing lists).
> 
> The following patch assigns the dev_id field to match the channel number
> on all multi-channel cards I could identify in a fresh git pull under
> drivers/net/can.  I can only test my two-port Peak PCI card, but it
> works as expected: ATTRS{dev_id} now expresses the port number and my
> udev rules now unambiguously pick out and rename my individual CAN
> ports.
> 
> The drivers for other cards I have touched at least compile, but should
> certainly be tested before deploying this patch.

Thanks for the patch, looks good. Can I add your Signed-off-by[1]?

Marc

[1]
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L307
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: Populating netdev::dev_id for udev discrimination

[Christopher R. Baker]

Absolutely, and thanks for the link (without undue noob-roasting ;) )

Signed-off-by: Christopher R. Baker <cbaker@rec.ri.cmu.edu>

-Chris

On Sun, 2014-03-09 at 20:05 +0100, Marc Kleine-Budde wrote:
> On 03/08/2014 05:00 PM, Christopher R. Baker wrote:
> > I'm new to the list, and have poked around on gmane enough to convince
> > myself that this hasn't been covered before, but if I've missed
> > something in this vein, apologies in advance.
> 
> You are the first one to address this issue. \o/
> 
> > My objective is to be able to totally discriminate CAN ports on
> > multi-port cards via udev so as to rename them to semantically
> > interesting/unique names for my system (e.g., "ecuCAN" and "auxCAN"
> > instead of "can0" and "can1").
> >
> > udevadm info gives me the KERNELS=... incantation to match the pci bus
> > address, but there are no other differences in the udev listing for my
> > various CAN ports (at least for my peak_pci device)
> > 
> > Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
> > the intention of discriminating between physical ports that "share the
> > same link layer" (from netdev.h) or "share the same MAC address" (from
> > various other mailing lists).
> > 
> > The following patch assigns the dev_id field to match the channel number
> > on all multi-channel cards I could identify in a fresh git pull under
> > drivers/net/can.  I can only test my two-port Peak PCI card, but it
> > works as expected: ATTRS{dev_id} now expresses the port number and my
> > udev rules now unambiguously pick out and rename my individual CAN
> > ports.
> > 
> > The drivers for other cards I have touched at least compile, but should
> > certainly be tested before deploying this patch.
> 
> Thanks for the patch, looks good. Can I add your Signed-off-by[1]?
> 
> Marc
> 
> [1]
> http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L307

Re: Populating netdev::dev_id for udev discrimination

[Oliver Hartkopp]

Hello Christopher,

On 09.03.2014 20:05, Marc Kleine-Budde wrote:
> On 03/08/2014 05:00 PM, Christopher R. Baker wrote:

> You are the first one to address this issue. \o/

I remember we discussed this issue years ago - but it was not really brought
to an end. So I really appreciate Christophers attempt :-)

>> udevadm info gives me the KERNELS=... incantation to match the pci bus
>> address, but there are no other differences in the udev listing for my
>> various CAN ports (at least for my peak_pci device)
>>
>> Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
>> the intention of discriminating between physical ports

I assume dev->if_port has to be assigned to different CAN transceiver types
for a specific CAN controller then ... 

We should keep that in mind if someone comes up with that question.

>> that "share the
>> same link layer" (from netdev.h) or "share the same MAC address" (from
>> various other mailing lists).
>>
>> The following patch assigns the dev_id field to match the channel number
>> on all multi-channel cards

Ok. That looks reasonable (and therefore the patch fits excellent).

As you stated above you can identify the card via the PCI bus address too.

Do you have an idea how to proceed with USB or other devices that bring their
own identification (like a serial number / device id) ??

E.g. the Softing Card provides some serial number through sysfs:

http://lxr.free-electrons.com/source/drivers/net/can/softing/softing_main.c#L719

and e.g. the PEAK USB have a so called "device number" that is currently just
printed in kernel log:

http://lxr.free-electrons.com/source/drivers/net/can/usb/peak_usb/pcan_usb_core.c#L799

Especially to assign USB devices that can be attached in a wild manner each
time a good idea would be needed for udev to identify the adapter.

Do you also have an idea for that? E.g. use the HW MAC address field (we do
not need for CAN) for this kind of "serial number / device id" ??

Regards,
Oliver

Re: Populating netdev::dev_id for udev discrimination

[Kurt Van Dijck]

Hey Christopher & Oliver,

On Mon, Mar 10, 2014 at 08:43:55AM +0100, Oliver Hartkopp wrote:
> Hello Christopher,
> 
> On 09.03.2014 20:05, Marc Kleine-Budde wrote:
> > On 03/08/2014 05:00 PM, Christopher R. Baker wrote:
> 
> > You are the first one to address this issue. \o/
> 
> I remember we discussed this issue years ago - but it was not really brought
> to an end. So I really appreciate Christophers attempt :-)
> 
> >> udevadm info gives me the KERNELS=... incantation to match the pci bus
> >> address, but there are no other differences in the udev listing for my
> >> various CAN ports (at least for my peak_pci device)
> >>
> >> Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
> >> the intention of discriminating between physical ports

I find this attempt attractive, because it moves multichannel device identification
to generic netdev infrastructure.

The patch obsoletes the 'channel' sysfs property of the softing device driver.
That property did exactly what can now be done with 'dev_id'.
I'll prepare a patch when this gets accepted.
That patch would not only remove 1 property.
The 'channel' property required the attr_group to be present at device creation.
Whithout 'channel', I could use the regular sysfs_create_group which makes
the code even simpler.

[...]
> 
> Ok. That looks reasonable (and therefore the patch fits excellent).
> 
> As you stated above you can identify the card via the PCI bus address too.

I contest this. If 2 CAN ports appear in 1 single PCI device probe,
then they share all PCI attributes, via their 'device' link.
> 
> Do you have an idea how to proceed with USB or other devices that bring their
> own identification (like a serial number / device id) ??
> 
> E.g. the Softing Card provides some serial number through sysfs:
> 
> http://lxr.free-electrons.com/source/drivers/net/can/softing/softing_main.c#L719

I still think this is the correct way.
udev identifies the device based on anyting device related (serial number, driver, ...)
and identifies the channel of the device based on the netdev (now through dev_id).
The netdev links to the device, so there is be no problem with identification.

> 
> and e.g. the PEAK USB have a so called "device number" that is currently just
> printed in kernel log:
> 
> http://lxr.free-electrons.com/source/drivers/net/can/usb/peak_usb/pcan_usb_core.c#L799
> 
> Especially to assign USB devices that can be attached in a wild manner each
> time a good idea would be needed for udev to identify the adapter.
> 
> Do you also have an idea for that? E.g. use the HW MAC address field (we do
> not need for CAN) for this kind of "serial number / device id" ??

Putting somehing else into the MAC address field is not right.
What problem do you try to solve here?

Kind regards,
Kurt Van Dijck

Re: Populating netdev::dev_id for udev discrimination

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

On 03/10/2014 11:46 AM, Kurt Van Dijck wrote:
> Hey Christopher & Oliver,
> 
> On Mon, Mar 10, 2014 at 08:43:55AM +0100, Oliver Hartkopp wrote:
>> Hello Christopher,
>>
>> On 09.03.2014 20:05, Marc Kleine-Budde wrote:
>>> On 03/08/2014 05:00 PM, Christopher R. Baker wrote:
>>
>>> You are the first one to address this issue. \o/
>>
>> I remember we discussed this issue years ago - but it was not really brought
>> to an end. So I really appreciate Christophers attempt :-)
>>
>>>> udevadm info gives me the KERNELS=... incantation to match the pci bus
>>>> address, but there are no other differences in the udev listing for my
>>>> various CAN ports (at least for my peak_pci device)
>>>>
>>>> Digging into the semantics of ATTRS{...}, "dev_id" stood out as having
>>>> the intention of discriminating between physical ports
> 
> I find this attempt attractive, because it moves multichannel device identification
> to generic netdev infrastructure.
> 
> The patch obsoletes the 'channel' sysfs property of the softing device driver.
> That property did exactly what can now be done with 'dev_id'.
> I'll prepare a patch when this gets accepted.
> That patch would not only remove 1 property.
> The 'channel' property required the attr_group to be present at device creation.
> Whithout 'channel', I could use the regular sysfs_create_group which makes
> the code even simpler.

You can create a patch against the testing branch of linux-can-next[1]

Marc

[1]
https://gitorious.org/linux-can/linux-can-next/source/e7bf719dbbad19857040ef50ed8b2cfefd81d78c:

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: Populating netdev::dev_id for udev discrimination

[Oliver Hartkopp]

Hey Kurt!

On 10.03.2014 11:46, Kurt Van Dijck wrote:

>> Do you have an idea how to proceed with USB or other devices that bring their
>> own identification (like a serial number / device id) ??
>>
>> E.g. the Softing Card provides some serial number through sysfs:
>>
>> http://lxr.free-electrons.com/source/drivers/net/can/softing/softing_main.c#L719
> 
> I still think this is the correct way.

Yes. You are right.

> udev identifies the device based on anyting device related (serial number, driver, ...)
> and identifies the channel of the device based on the netdev (now through dev_id).
> The netdev links to the device, so there is be no problem with identification.

Looking into your code IMO a similar thing has to be done for the other CAN
interfaces providing HW/SW versions and unique IDs like e.g. a serial number.

E.g. the PEAK USB adapters or the USB8DEV adapter, etc.

It already looks pretty good for a general usage.

Do you know what your attributes "frequency" and "tx_pending" have been used
for in the softing driver? Are they still needed?

> Putting somehing else into the MAC address field is not right.

Ack. It was just a suggestion. But after looking into your sysfs code you
convinced me :-)

> What problem do you try to solve here?

Just to have a unique ID to identify CAN USB adapters to set them to the right
bitrate and netdevice name via udev when they are attached.

Best regards,
Oliver

Re: Populating netdev::dev_id for udev discrimination

[Oliver Hartkopp]

On 10.03.2014 12:01, Marc Kleine-Budde wrote:

> 
> You can create a patch against the testing branch of linux-can-next[1]
> 

FYI:

Tested with PEAK PCAN USB pro and EMS PCMCIA (both with two channels) and the
dev_id works like expected (first channel dev_id = 0x0, second channel 0x1).

Tnx
Oliver

Re: Populating netdev::dev_id for udev discrimination

[Kurt Van Dijck]

Marc,

[...]
> You can create a patch against the testing branch of linux-can-next[1]

later this week ...

Kurt

Re: Populating netdev::dev_id for udev discrimination

[Kurt Van Dijck]

On Mon, Mar 10, 2014 at 01:15:01PM +0100, Oliver Hartkopp wrote:
> Hey Kurt!
> 
> On 10.03.2014 11:46, Kurt Van Dijck wrote:
> 
> >> Do you have an idea how to proceed with USB or other devices that bring their
> >> own identification (like a serial number / device id) ??
> >>
> >> E.g. the Softing Card provides some serial number through sysfs:
> >>
> >> http://lxr.free-electrons.com/source/drivers/net/can/softing/softing_main.c#L719
> > 
> > I still think this is the correct way.
> 
> Yes. You are right.
> 
> > udev identifies the device based on anyting device related (serial number, driver, ...)
> > and identifies the channel of the device based on the netdev (now through dev_id).
> > The netdev links to the device, so there is be no problem with identification.
> 
> Looking into your code IMO a similar thing has to be done for the other CAN
> interfaces providing HW/SW versions and unique IDs like e.g. a serial number.

Yes, I think too.
Now that (later this week) the netdev attrs have been integrated in a regular way,
the sysfs code will be much more readable and easier to follow than today.
I had to use special handlings for the 'channel', now dev_id.

> 
> E.g. the PEAK USB adapters or the USB8DEV adapter, etc.
> 
> It already looks pretty good for a general usage.
> 
> Do you know what your attributes "frequency" and "tx_pending" have been used
> for in the softing driver? Are they still needed?

tx_pending can go. I'll address that too later this week.
Frequency is an indication of the embedded cpu's frequency.
It's exported, but with no immediate user.

Kurt

Re: Populating netdev::dev_id for udev discrimination

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On 03/10/2014 01:18 PM, Oliver Hartkopp wrote:
>> You can create a patch against the testing branch of linux-can-next[1]
> 
> FYI:
> 
> Tested with PEAK PCAN USB pro and EMS PCMCIA (both with two channels) and the
> dev_id works like expected (first channel dev_id = 0x0, second channel 0x1).

Can I add your Tested-by for the Peak and the EMS?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: Populating netdev::dev_id for udev discrimination

[Oliver Hartkopp]

On 11.03.2014 20:08, Marc Kleine-Budde wrote:
> On 03/10/2014 01:18 PM, Oliver Hartkopp wrote:
>>> You can create a patch against the testing branch of linux-can-next[1]
>>
>> FYI:
>>
>> Tested with PEAK PCAN USB pro and EMS PCMCIA (both with two channels) and the
>> dev_id works like expected (first channel dev_id = 0x0, second channel 0x1).
> 
> Can I add your Tested-by for the Peak and the EMS?
> 

Oh, yes.

At least for these two adapters I checked it myself.

Thanks,
Oliver

Re: Populating netdev::dev_id for udev discrimination

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On 03/11/2014 08:41 PM, Oliver Hartkopp wrote:
>>> FYI:
>>>
>>> Tested with PEAK PCAN USB pro and EMS PCMCIA (both with two channels) and the
>>> dev_id works like expected (first channel dev_id = 0x0, second channel 0x1).
>>
>> Can I add your Tested-by for the Peak and the EMS?
>>
> 
> Oh, yes.
> 
> At least for these two adapters I checked it myself.

I've added:
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> [PEAK PCAN-USB pro and EMS PCMCIA]

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]