| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Sep 2009 13:40:55 +0200
From: Sebastian Haas <haas@...-wuensche.com>
To: Wolfgang Grandegger <wg@...ndegger.com>
CC: netdev@...r.kernel.org, socketcan-core@...ts.berlios.de,
greg@...ah.com, oliver@...tkopp.net, linux-usb@...r.kernel.org,
Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH 2/2] ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB
interface
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Wolfgang Grandegger schrieb:
> Hi Sebastian,
>
> Sebastian Haas wrote:
>> This patch adds support for one channel CAN/USB interace CPC-USB/ARM7 from
>> EMS Dr. Thomas Wuensche (http://www.ems-wuensche.com).
>>
>> Signed-off-by: Sebastian Haas <haas@...-wuensche.com>
>
> The driver is almost OK from the Socket-CAN point of view. Just some
> *final* nitpicking:
>
> [snip]
>> +static void ems_usb_read_interrupt_callback(struct urb *urb)
>> +{
>> + struct ems_usb *dev = urb->context;
>> + struct net_device *netdev;
>> + int err;
>> +
>> + netdev = dev->netdev;
>
> Could be done in the declaration part above.
Fixed.
>> + if (!netif_device_present(netdev))
>> + return;
>> +
>> + switch (urb->status) {
>> + case 0:
>> + dev->free_slots = dev->intr_in_buffer[1];
>> + break;
>> +
>> + case -ECONNRESET: /* unlink */
>> + case -ENOENT:
>> + case -ESHUTDOWN:
>> + return;
>> +
>> + default:
>> + dev_info(netdev->dev.parent, "Rx interrupt aborted %d\n",
>> + urb->status);
>> + break;
>> + }
>> +
>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>> +
>> + if (err == -ENODEV)
>> + netif_device_detach(netdev);
>> + else if (err)
>> + dev_err(netdev->dev.parent,
>> + "failed resubmitting intr urb: %d\n", err);
>> +
>> + return;
>> +}
>> +
>> +static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>> +{
>> + struct can_frame *cf;
>> + struct sk_buff *skb;
>> + int i;
>> + struct net_device_stats *stats = &dev->netdev->stats;
>> +
>> + skb = dev_alloc_skb(sizeof(struct can_frame));
>
> Please use netdev_alloc_skb() ...
Fixed.
>> + if (skb == NULL)
>> + return;
>> +
>> + skb->dev = dev->netdev;
>
> ... making the line above obsolete.
Fixed.
>> + skb->protocol = htons(ETH_P_CAN);
>> +
>> + cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>> +
>> + cf->can_id = msg->msg.can_msg.id;
>> + cf->can_dlc = min_t(u8, msg->msg.can_msg.length, 8);
>> +
>> + if (msg->type == CPC_MSG_TYPE_EXT_CAN_FRAME
>> + || msg->type == CPC_MSG_TYPE_EXT_RTR_FRAME)
>> + cf->can_id |= CAN_EFF_FLAG;
>> +
>> + if (msg->type == CPC_MSG_TYPE_RTR_FRAME
>> + || msg->type == CPC_MSG_TYPE_EXT_RTR_FRAME) {
>> + cf->can_id |= CAN_RTR_FLAG;
>> + } else {
>> + for (i = 0; i < cf->can_dlc; i++)
>> + cf->data[i] = msg->msg.can_msg.msg[i];
>> + }
>> +
>> + netif_rx(skb);
>> +
>> + dev->netdev->last_rx = jiffies;
>> + stats->rx_packets++;
>> + stats->rx_bytes += cf->can_dlc;
>> +}
>> +
>> +static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>> +{
>> + struct can_frame *cf;
>> + struct sk_buff *skb;
>> + struct net_device_stats *stats = &dev->netdev->stats;
>> +
>> + skb = dev_alloc_skb(sizeof(struct can_frame));
>> + if (skb == NULL)
>> + return;
>> +
>> + skb->dev = dev->netdev;
>
> Ditto.
>
> [snip]
>> +static int ems_usb_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>> +{
>> + struct ems_usb *dev = netdev_priv(netdev);
>> + struct ems_tx_urb_context *context = NULL;
>> + struct net_device_stats *stats = &netdev->stats;
>> + struct can_frame *cf = (struct can_frame *)skb->data;
>> + struct ems_cpc_msg *msg;
>> + struct urb *urb;
>> + u8 *buf;
>> + int i, err;
>> + size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
>> + + sizeof(struct cpc_can_msg);
>> +
>> + /* create a URB, and a buffer for it, and copy the data to the URB */
>> + urb = usb_alloc_urb(0, GFP_ATOMIC);
>> + if (!urb) {
>> + dev_err(netdev->dev.parent, "No memory left for URBs\n");
>> + goto nomem;
>> + }
>> +
>> + buf = usb_buffer_alloc(dev->udev, size, GFP_ATOMIC, &urb->transfer_dma);
>> + if (!buf) {
>> + dev_err(netdev->dev.parent, "No memory left for USB buffer\n");
>> + usb_free_urb(urb);
>> + goto nomem;
>> + }
>> +
>> + msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
>> +
>> + msg->msg.can_msg.id = cf->can_id & 0x1FFFFFFFU;
>
> Please use CAN_ERR_MASK instead (even if the name is somehow
> misleading). See http://lxr.linux.no/#linux+v2.6.31/include/linux/can.h#L31.
Fixed.
> [snip]
>> +}
>> +
>> +
>
> Remove one empty line, please.
Fixed.
>> +static void init_params_sja1000(struct ems_cpc_msg *msg)
>> +{
>> + struct cpc_sja1000_params *sja1000 =
>> + &msg->msg.can_params.cc_params.sja1000;
>> +
>> + msg->type = CPC_CMD_TYPE_CAN_PARAMS;
>> + msg->length = sizeof(struct cpc_can_params);
>> + msg->msgid = 0;
>> +
>> + msg->msg.can_params.cc_type = CPC_CC_TYPE_SJA1000;
>> +
>> + /* Acceptance filter open */
>> + sja1000->acc_code0 = 0x00;
>> + sja1000->acc_code1 = 0x00;
>> + sja1000->acc_code2 = 0x00;
>> + sja1000->acc_code3 = 0x00;
>> +
>> + /* Acceptance filter open */
>> + sja1000->acc_mask0 = 0xFF;
>> + sja1000->acc_mask1 = 0xFF;
>> + sja1000->acc_mask2 = 0xFF;
>> + sja1000->acc_mask3 = 0xFF;
>> +
>> + sja1000->btr0 = 0;
>> + sja1000->btr1 = 0;
>> +
>> + sja1000->outp_contr = SJA1000_DEFAULT_OUTPUT_CONTROL;
>> + sja1000->mode = SJA1000_MOD_RM;
>> +}
>> +
>> +/*
>> + * probe function for new CPC-USB devices
>> + */
>> +static int ems_usb_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct net_device *netdev;
>> + struct ems_usb *dev;
>> + int i, err;
>> +
>> + netdev = alloc_candev(sizeof(struct ems_usb));
>> + if (!netdev) {
>> + dev_err(netdev->dev.parent, "Couldn't alloc candev\n");
>> + return -ENOMEM;
>> + }
>> +
>> + dev = netdev_priv(netdev);
>> +
>> + dev->udev = interface_to_usbdev(intf);
>> + dev->netdev = netdev;
>> +
>> + dev->can.state = CAN_STATE_STOPPED;
>> + dev->can.bittiming_const = &ems_usb_bittiming_const;
>> + dev->can.do_set_bittiming = ems_usb_set_bittiming;
>> + dev->can.do_set_mode = ems_usb_set_mode;
>> +
>> + netdev->flags |= IFF_ECHO; /* we support local echo */
>> +
>> + /*
>> + * The device actually uses a 16MHz clock to generate the CAN clock
>> + * but it expects SJA1000 bit settings based on 8MHz (is internally
>> + * converted).
>> + */
>
> Should go up to the macro definition.
Fixed.
>> + dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
>> +
>> + netdev->netdev_ops = &ems_usb_netdev_ops;
>> +
>> + netdev->flags |= IFF_ECHO; /* we support local echo */
>> +
>> + init_usb_anchor(&dev->rx_submitted);
>> +
>> + init_usb_anchor(&dev->tx_submitted);
>> + atomic_set(&dev->active_tx_urbs, 0);
>> +
>> + for (i = 0; i < MAX_TX_URBS; i++)
>> + dev->tx_contexts[i].echo_index = MAX_TX_URBS;
>> +
>> + dev->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!dev->intr_urb) {
>> + dev_err(netdev->dev.parent, "Couldn't alloc intr URB\n");
>> + free_candev(netdev);
>
> Please use goto's for cleanup to avoid code duplication.
>
>> + return -ENOMEM;
>> + }
>> +
>> + dev->intr_in_buffer = kzalloc(INTR_IN_BUFFER_SIZE, GFP_KERNEL);
>> + if (!dev->intr_in_buffer) {
>> + dev_err(netdev->dev.parent, "Couldn't alloc Intr buffer\n");
>> + free_candev(netdev);
>> + usb_free_urb(dev->intr_urb);
>> + return -ENOMEM;
>
> Ditto.
>
>> + }
>> +
>> + dev->tx_msg_buffer = kzalloc(CPC_HEADER_SIZE +
>> + sizeof(struct ems_cpc_msg), GFP_KERNEL);
>> + if (!dev->tx_msg_buffer) {
>> + dev_err(netdev->dev.parent, "Couldn't alloc Tx buffer\n");
>> + free_candev(netdev);
>> + usb_free_urb(dev->intr_urb);
>> + kfree(dev->intr_in_buffer);
>> + return -ENOMEM;
>
> Ditto.
>
>> + }
>> +
>> + usb_set_intfdata(intf, dev);
>> +
>> + SET_NETDEV_DEV(netdev, &intf->dev);
>> +
>> + init_params_sja1000(&dev->active_params);
>> +
>> + err = ems_usb_command_msg(dev, &dev->active_params);
>> + if (err) {
>> + dev_err(netdev->dev.parent,
>> + "couldn't initialize controller: %d\n", err);
>> + free_candev(netdev);
>> + usb_free_urb(dev->intr_urb);
>> + return err;
>
> Ditto. Also kfree(dev->intr_in_buffer) seems to be missing.
Fixed.
> Please add a version number to the next patch, e.g. [PATCH v3 ...].
Okay, have to figure out how to do with stgit ;-)
> It would also be nice, if some USB guru's could have a look as well.
Oliver Neukum already did a quick review. I've fixed serveral
GFP_ATOMIC/KERNEL issues and a problem with DMA coherency on his feedback.
Cheers, Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkqvfUAACgkQpqRB8PJG7Xxw/ACfT8FUQ5DlmrcDnxotWceIcyNn
fOIAoI/w8NHJg0qGxpF2PvU6Qd5BXaBg
=DV5x
-----END PGP SIGNATURE-----
--
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HRA Neuburg a.d. Donau, HR-Nr. 70.106
Phone: +49-8441-490260
Fax : +49-8441-81860
http://www.ems-wuensche.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists