can: Add driver for esd CAN-USB/2 device

PATCH Superseded Matthias Fuchs 5 participants 9 messages May 26, 2010 – May 31, 2010 4 messages not in the archive

Subsystems (3)

Networking Network drivers CAN network drivers

lore

Share & tools

lore

Matthias Fuchs

author

2010-05-26 09:14 UTC #0 lore raw

PATCH v3 1/1 can: Add driver for esd CAN-USB/2 device

This patch adds a driver for esd's USB high speed
CAN interface. The driver supports devices with
multiple CAN interfaces.

Signed-off-by Matthias Fuchs <[email protected]>

---
version 3:
- remove bus-error reporting feature because
it cannot be controlled by user on demand
with current device's firmware
- rebased against current net-next-2.6 tree

version 2:
- use bus-error reporting and counters
- minor cleanup
- rebased against current net-next-2.6 tree
- initial post to linux-usb list for review

 drivers/net/can/usb/Kconfig    |    6 +
 drivers/net/can/usb/Makefile   |    1 +
 drivers/net/can/usb/esd_usb2.c | 1125 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1132 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/usb/esd_usb2.c

drivers/net/can/usb/Kconfig +6 -0

index 97ff6fe..0452549 100644
@@ -7,4 +7,10 @@ config CAN_EMS_USB
 	  This driver is for the one channel CPC-USB/ARM7 CAN/USB interface
 	  from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
 
+config CAN_ESD_USB2
+        tristate "ESD USB/2 CAN/USB interface"
+        ---help---
+          This driver supports the CAN-USB/2 interface
+          from esd electronic system design gmbh (http://www.esd.eu).
+
 endmenu

drivers/net/can/usb/Makefile +1 -0

index 0afd51d..fce3cf1 100644
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
+obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

drivers/net/can/usb/esd_usb2.c new file +1125 -0

1,125 lines not on this page · raw message

Signature · 1 line

1.5.6.3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfgang Grandegger

2010-05-26 10:15 UTC #2 lore raw

Hi Matthias,

Quoted 12 lines · Matthias Fuchs

This patch adds a driver for esd's USB high speed
CAN interface. The driver supports devices with
multiple CAN interfaces.
Signed-off-by Matthias Fuchs <[email protected]>
---
version 3:
- remove bus-error reporting feature because
it cannot be controlled by user on demand
with current device's firmware
- rebased against current net-next-2.6 tree

Quoted 5 lines

version 2:
- use bus-error reporting and counters
- minor cleanup
- rebased against current net-next-2.6 tree
- initial post to linux-usb list for review

I get the following compiler warnings when compiling the kernel:

CC drivers/net/can/usb/esd_usb2.o
drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_write_bulk_callback':
drivers/net/can/usb/esd_usb2.c:466: error: implicit declaration of function 'usb_buffer_free' drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_setup_rx_urbs':
drivers/net/can/usb/esd_usb2.c:562: error: implicit declaration of function 'usb_buffer_alloc' drivers/net/can/usb/esd_usb2.c:563: warning: assignment makes pointer from integer without a cast drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_start_xmit':
drivers/net/can/usb/esd_usb2.c:732: warning: assignment makes pointer from integer without a cast make[4]: *** [drivers/net/can/usb/esd_usb2.o] Error 1

This is due to commit e26bcf37234c67624f62d9fc95f922b8dbda1363. You need a similar fix like for ems_usb.c. Are you using a recent version of the net-next-2.6 tree?

Wolfgang.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matthias Fuchs

author

2010-05-26 12:38 UTC #4 lore raw

Hi Wolfgang,

oops. I did my tests againt latest net-next-2.6. But I forgot to commit these missing changes. I attached a fixed version of patch v3. v4 will propably come when I finally checked Viral's comments.

Matthias

Quoted 33 lines · Wolfgang Grandegger

Hi Matthias,
Quoted 12 lines · Matthias Fuchs
This patch adds a driver for esd's USB high speed
CAN interface. The driver supports devices with
multiple CAN interfaces.
Signed-off-by Matthias Fuchs <[email protected]>
---
version 3:
- remove bus-error reporting feature because
it cannot be controlled by user on demand
with current device's firmware
- rebased against current net-next-2.6 tree
Quoted 5 lines
version 2:
- use bus-error reporting and counters
- minor cleanup
- rebased against current net-next-2.6 tree
- initial post to linux-usb list for review
I get the following compiler warnings when compiling the kernel:
CC drivers/net/can/usb/esd_usb2.o
drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_write_bulk_callback':
drivers/net/can/usb/esd_usb2.c:466: error: implicit declaration of function 'usb_buffer_free' drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_setup_rx_urbs':
drivers/net/can/usb/esd_usb2.c:562: error: implicit declaration of function 'usb_buffer_alloc' drivers/net/can/usb/esd_usb2.c:563: warning: assignment makes pointer from integer without a cast drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_start_xmit':
drivers/net/can/usb/esd_usb2.c:732: warning: assignment makes pointer from integer without a cast make[4]: *** [drivers/net/can/usb/esd_usb2.o] Error 1

Quoted 7 lines

This is due to commit e26bcf37234c67624f62d9fc95f922b8dbda1363. You need a similar fix like for ems_usb.c. Are you using a recent version of the net-next-2.6 tree?
Wolfgang.

---
 drivers/net/can/usb/Kconfig    |    6 +
 drivers/net/can/usb/Makefile   |    1 +
 drivers/net/can/usb/esd_usb2.c | 1126 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/usb/esd_usb2.c

drivers/net/can/usb/Kconfig +6 -0

index 97ff6fe..0452549 100644
@@ -7,4 +7,10 @@ config CAN_EMS_USB
 	  This driver is for the one channel CPC-USB/ARM7 CAN/USB interface
 	  from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
 
+config CAN_ESD_USB2
+        tristate "ESD USB/2 CAN/USB interface"
+        ---help---
+          This driver supports the CAN-USB/2 interface
+          from esd electronic system design gmbh (http://www.esd.eu).
+
 endmenu

drivers/net/can/usb/Makefile +1 -0

index 0afd51d..fce3cf1 100644
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
+obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

drivers/net/can/usb/esd_usb2.c new file +1126 -0

1,126 lines not on this page · raw message

Signature · 6 lines

1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viral Mehta

2010-05-26 11:31 UTC #5 lore raw

In reply to a message not in the archive

Hi,

Quoted 14 lines

________________________________________
From: [email protected] [[email protected]] On Behalf Of Matthias Fuchs [[email protected]]
Sent: Wednesday, May 26, 2010 2:44 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
This patch adds a driver for esd's USB high speed
CAN interface. The driver supports devices with
multiple CAN interfaces.
Signed-off-by Matthias Fuchs <[email protected]>
---

drivers/net/can/usb/esd_usb2.c line 459

Quoted 9 lines

drivers/net/can/usb/esd_usb2.c new file +9 -0

+static void esd_usb2_write_bulk_callback(struct urb *urb)
+{
+       struct esd_tx_urb_context *context = urb->context;
+       struct esd_usb2_net_priv *priv;
+       struct esd_usb2 *dev;
+       struct net_device *netdev;
+       size_t size = sizeof(struct esd_usb2_msg);
+
+       BUG_ON(!context);

It is preferred to used WARN_ON and avoid using BUG_ON and thus dont kill the whole system.... [...]

Quoted 9 lines

drivers/net/can/usb/esd_usb2.c new file +9 -0

+
+       priv = context->priv;
+       netdev = priv->netdev;
+       dev = priv->usb2;
+       err = usb_submit_urb(urb, GFP_ATOMIC);
+       if (err) {
+               can_free_echo_skb(netdev, context->echo_index);
+
+               atomic_dec(&priv->active_tx_jobs);

drivers/net/can/usb/esd_usb2.c line 808

Quoted 10 lines

drivers/net/can/usb/esd_usb2.c new file +10 -0

+               usb_unanchor_urb(urb);
+
+               stats->tx_dropped++;
+
+               if (err == -ENODEV)
+                       netif_device_detach(netdev);
+               else
+                       dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
+
+               goto releasebuf;

You probably want to set "ret" here or do you really want to return NETDEV_TX_OK

Quoted 1 line

+ }

[...]

Quoted 4 lines

drivers/net/can/usb/esd_usb2.c new file +4 -0

+static int esd_usb2_close(struct net_device *netdev)
+{
+       struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+       struct esd_usb2_msg msg;

drivers/net/can/usb/esd_usb2.c line 848

Quoted 10 lines

drivers/net/can/usb/esd_usb2.c new file +10 -0

+       int i;
+
+       /* Disable all IDs (see esd_usb2_start()) */
+       msg.msg.hdr.cmd = CMD_IDADD;
+       msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+       msg.msg.filter.net = priv->index;
+       msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+       for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
+               msg.msg.filter.mask[i] = 0;
+       esd_usb2_send_msg(priv->usb2, &msg);

Probably, you may not care but it can return error value....

drivers/net/can/usb/esd_usb2.c line 856

Quoted 8 lines

drivers/net/can/usb/esd_usb2.c new file +8 -0

+
+       /* set CAN controller to reset mode */
+       msg.msg.hdr.len = 2;
+       msg.msg.hdr.cmd = CMD_SETBAUD;
+       msg.msg.setbaud.net = priv->index;
+       msg.msg.setbaud.rsvd = 0;
+       msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
+       esd_usb2_send_msg(priv->usb2, &msg);

Probably, you may not care but it can return error value....

Quoted 7 lines

drivers/net/can/usb/esd_usb2.c new file +7 -0

+
+       priv->can.state = CAN_STATE_STOPPED;
+
+       netif_stop_queue(netdev);
+
+       close_candev(netdev);
+

[...]

drivers/net/can/usb/esd_usb2.c line 1015

Quoted 10 lines

drivers/net/can/usb/esd_usb2.c new file +10 -0

+static int esd_usb2_probe(struct usb_interface *intf,
+                        const struct usb_device_id *id)
+{
+       struct esd_usb2 *dev;
+       struct esd_usb2_msg msg;
+       int i, err = -ENOMEM;
+
+       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+       if (!dev)
+               return -ENOMEM;

You probably would like to rewrite "return" statements.... for example, above you define err=-ENOMEM and then here you are saying "return -ENOMEM" later on at the end you are saying "return err";

Also, people prefer and practice to have only one return point. Read further.

Quoted 7 lines

drivers/net/can/usb/esd_usb2.c new file +7 -0

+
+       dev->udev = interface_to_usbdev(intf);
+
+       init_usb_anchor(&dev->rx_submitted);
+
+       usb_set_intfdata(intf, dev);
+

drivers/net/can/usb/esd_usb2.c line 1032

Quoted 10 lines

drivers/net/can/usb/esd_usb2.c new file +10 -0

+       /* query number of CAN interfaces (nets) */
+       msg.msg.hdr.cmd = CMD_VERSION;
+       msg.msg.hdr.len = 2;
+       msg.msg.version.rsvd = 0;
+       msg.msg.version.flags = 0;
+       msg.msg.version.drv_version = 0;
+
+       if (esd_usb2_send_msg(dev, &msg) < 0) {
+               dev_err(&intf->dev, "sending version message failed\n");
+               goto free_dev;

You might like to set "err" here, since you are not propogating error values properly..... Irrespective of what esd_usb2_send_msg and thus in turn usb_bulk_msg returns, you are ignoring those values and always returning -ENOMEM at the end......

drivers/net/can/usb/esd_usb2.c line 1037

Quoted 5 lines

drivers/net/can/usb/esd_usb2.c new file +5 -0

+       }
+
+       if (esd_usb2_wait_msg(dev, &msg) < 0) {
+               dev_err(&intf->dev, "no version message answer\n");
+               goto free_dev;

Same here.

drivers/net/can/usb/esd_usb2.c line 1066

Quoted 4 lines

drivers/net/can/usb/esd_usb2.c new file +4 -0

+free_dev:
+       kfree(dev);
+       return err;
+}

HTH,
Viral Mehta

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 70376CA23424B34D86F1C7DE6B9973430254343AD9-fVvhrSDAkVjuuPCCJ6VnObSn4PsL5ZDKvpI+GvaZM4lBDgjK7y7TUQ@public.gmane.org

Matthias Fuchs

author

2010-05-28 15:25 UTC #7 lore raw

Hi Viral,

thanks for review. Please see some comments below.

Quoted 11 lines · Viral Mehta

Hi,
drivers/net/can/usb/esd_usb2.c line 459
Quoted 8 lines
________________________________________
From: [email protected] [[email protected]] On Behalf Of Matthias Fuchs [[email protected]]
Sent: Wednesday, May 26, 2010 2:44 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
drivers/net/can/usb/esd_usb2.c new file +2 -0
+
+       BUG_ON(!context);
It is preferred to used WARN_ON and avoid using BUG_ON and thus dont kill the whole system....

Really? Even when next line will reference a NULL pointer in this case? Ok. Will be changed.

Quoted 14 lines

[...]

Quoted 13 lines

drivers/net/can/usb/esd_usb2.c new file +13 -0

+
+       priv = context->priv;
+       netdev = priv->netdev;
+       dev = priv->usb2;
+       err = usb_submit_urb(urb, GFP_ATOMIC);
+       if (err) {
+               can_free_echo_skb(netdev, context->echo_index);
+
+               atomic_dec(&priv->active_tx_jobs);
+               usb_unanchor_urb(urb);
+
+               stats->tx_dropped++;
+

Quoted 8 lines

drivers/net/can/usb/esd_usb2.c line 808

Quoted 6 lines

drivers/net/can/usb/esd_usb2.c new file +6 -0

+               if (err == -ENODEV)
+                       netif_device_detach(netdev);
+               else
+                       dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
+
+               goto releasebuf;

You probably want to set "ret" here or do you really want to return NETDEV_TX_OK

As far as I can see netword device drivers xmit_start() return NETDEV_TX_OK or _BUSY. There are no real alternatives. But please correct me when I am wrong.

Your other comments will be fixed by version 4 of my patch.

Matthias

Viral Mehta

2010-05-31 06:35 UTC #8 lore raw

Hi,

Quoted 25 lines

-----Original Message-----
From: Matthias Fuchs [mailto:[email protected]]
Sent: Friday, May 28, 2010 8:56 PM
To: Viral Mehta
Cc: [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
Hi Viral,
thanks for review. Please see some comments below.
Quoted 3 lines · Viral Mehta
Hi,
Quoted 2 lines
________________________________________
From: [email protected] [linux-usb-
[email protected]] On Behalf Of Matthias Fuchs [[email protected]]
Quoted 7 lines
Quoted 6 lines
Sent: Wednesday, May 26, 2010 2:44 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
drivers/net/can/usb/esd_usb2.c new file +2 -0
+
+       BUG_ON(!context);

Quoted 4 lines

Quoted 1 line
It is preferred to used WARN_ON and avoid using BUG_ON and thus dont
kill the whole system....
Really? Even when next line will reference a NULL pointer in this case? Ok. Will be changed.

How about,
if(!context)

        return;

Look at the other drivers for example.
I, personally, dont care since I will not be using your driver but I am sure that other people, too, wont like BUG_ON....

Quoted 20 lines

Quoted 11 lines

[...]

Quoted 9 lines

drivers/net/can/usb/esd_usb2.c new file +9 -0

+
+       priv = context->priv;
+       netdev = priv->netdev;
+       dev = priv->usb2;
+       err = usb_submit_urb(urb, GFP_ATOMIC);
+       if (err) {
+               can_free_echo_skb(netdev, context->echo_index);
+
+               atomic_dec(&priv->active_tx_jobs);

Quoted 8 lines

drivers/net/can/usb/esd_usb2.c

Quoted 8 lines

drivers/net/can/usb/esd_usb2.c new file +8 -0

+               usb_unanchor_urb(urb);
+
+               stats->tx_dropped++;
+
+               if (err == -ENODEV)
+                       netif_device_detach(netdev);
+               else
+                       dev_warn(netdev->dev.parent, "failed tx_urb

%d\n", err);

Quoted 8 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+               goto releasebuf;
You probably want to set "ret" here or do you really want to return
NETDEV_TX_OK
As far as I can see netword device drivers xmit_start() return NETDEV_TX_OK or _BUSY.
There are no real alternatives.

Yup. this is okie....

Quoted 7 lines

But please correct me when I am wrong.
Your other comments will be fixed by version 4 of my patch.
Matthias
______________________________________________________________________

______________________________________________________________________

Oliver Neukum

2010-05-26 13:47 UTC #9 lore raw

In reply to Matthias Fuchs

Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:

drivers/net/can/usb/esd_usb2.c line 815

Quoted 5 lines

drivers/net/can/usb/esd_usb2.c new file +5 -0

+       netdev->trans_start = jiffies;
+
+       /* Slow down tx path */
+       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+               netif_stop_queue(netdev);

Where is the queue started again?

Regards
Oliver

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matthias Fuchs

author

2010-05-28 14:19 UTC #11 lore raw

Quoted 11 lines · Oliver Neukum

Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:
drivers/net/can/usb/esd_usb2.c line 815
Quoted 5 lines
drivers/net/can/usb/esd_usb2.c new file +5 -0
+       netdev->trans_start = jiffies;
+
+       /* Slow down tx path */
+       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+               netif_stop_queue(netdev);
Where is the queue started again?
Regards
Oliver

This is done in the ack-patch for sent messages:

static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
struct esd_usb2_msg *msg)
{
...

	atomic_dec(&priv->active_tx_jobs);

	netif_wake_queue(netdev);
}

Matthias

Oliver Neukum

2010-05-28 15:25 UTC #12 lore raw

Am Freitag, 28. Mai 2010 16:19:33 schrieb Matthias Fuchs:

Quoted 15 lines · Oliver Neukum

Quoted 11 lines
Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:
drivers/net/can/usb/esd_usb2.c line 815
Quoted 5 lines
drivers/net/can/usb/esd_usb2.c new file +5 -0
+       netdev->trans_start = jiffies;
+
+       /* Slow down tx path */
+       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+               netif_stop_queue(netdev);
Where is the queue started again?
Regards
Oliver
This is done in the ack-patch for sent messages:

Quoted 8 lines

static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
struct esd_usb2_msg *msg)
{
...
	atomic_dec(&priv->active_tx_jobs);

	netif_wake_queue(netdev);
}

Oh, I see. I am afraid this is a race. You have no guarantee that esd_usb2_tx_done_msg() runs after you stop the queue. If you do it that way you must stop the queue before you submit the URB due to which you want to stop it (and do the error handling)

Regards
Oliver