Hello Wolfgang,
> > + ret = ixxat_usb_snd_cmd(dev->udev, port, cmd, snd_size);
> > + if (ret < 0)
> > + goto fail;
> > +
> > + ret = ixxat_usb_rcv_cmd(dev->udev, port, &cmd->res, rcv_size);
> > + if (ret < 0)
> > + goto fail;
> > +
> > + ret = le32_to_cpu(cmd->res.code);
>
> This pattern repeats five times or more. Could you not handle it by just
> one function?
I will add this in the next patch:
int ixxat_usb_snd_cmd(struct usb_device *dev, const u16 port, void *req,
const u16 req_size, void *res, const u16 res_size)
{
int ret = ixxat_usb_snd_req(dev, port, req, req_size);
struct ixxat_usb_dal_res *dal_res = res;
if (ret < 0)
goto fail;
ret = ixxat_usb_rcv_res(dev, port, res, res_size);
if (ret < 0)
goto fail;
ret = dal_res->code;
fail:
return ret;
}
> > +MODULE_DESCRIPTION("CAN driver for IXXAT USB-to-CAN FD adapters");
>
> The driver is also for non-FD adapters, right?
Yes, I will change it to this:
MODULE_DESCRIPTION("CAN driver for IXXAT USB-to-CAN / CAN FD adapters");
> > +int ixxat_usb_snd_cmd(struct usb_device *dev, const u16 port, void *buf,
> > + const u16 size)
> > +{
> > + const int to = msecs_to_jiffies(IXXAT_USB_MSG_TIMEOUT);
> > + const u8 rq = 0xff;
> > + const u8 rt = USB_TYPE_VENDOR | USB_DIR_OUT;
> > + int i;
> > + int ret = 0;
> > + int scp = usb_sndctrlpipe(dev, 0);
> > +
> > + for (i = 0; i < IXXAT_USB_MAX_COM_REQ; ++i) {
> > + ret = usb_control_msg(dev, scp, rq, rt, port, 0, buf, size, to);
> > + if (ret < 0)
> > + msleep(IXXAT_USB_MSG_CYCLE);
> > + else
> > + break;
> > + }
> > +
> > + if (ret < 0)
> > + dev_info(&dev->dev, "Error %d: TX command failure\n", ret);
>
> dev_info or dev_error?
>
> > +
> > + return ret;
> > +}
Oh, I changed this by mistake. It should be dev_error here and also in
ixxat_usb_rcv_cmd.
> > +static int ixxat_usb_handle_error(struct ixxat_usb_device *dev,
> > + struct ixxat_can_msg *rx)
> > +{
> > + struct net_device *netdev = dev->netdev;
> > + struct can_frame *can_frame;
> > + struct sk_buff *skb = alloc_can_err_skb(netdev, &can_frame);
> > + u8 raw_error;
> > +
> > + if (dev->adapter == &usb2can_cl1) {
> > + raw_error = rx->cl1.data[0];
> > + dev->bec.rxerr = rx->cl1.data[3];
> > + dev->bec.txerr = rx->cl1.data[4];
> > + } else {
> > + raw_error = rx->cl2.data[0];
> > + dev->bec.rxerr = rx->cl2.data[3];
> > + dev->bec.txerr = rx->cl2.data[4];
> > + }
>
> Hm, do the lines avove make sense also if "dev->can.state ==
> CAN_STATE_BUS_OFF"?
>
> > +
> > + if (dev->can.state == CAN_STATE_BUS_OFF)
> > + return 0;
> > +
> > + if (!skb)
> > + return -ENOMEM;
>
> Could you please do the "alloc_can_err_skb()" just before these two
> lines... otherwise the skb might not be freed.
I will fix this in the next patch.
> > +static void ixxat_usb_read_bulk(struct urb *urb)
> > +{
> > + struct ixxat_usb_device *dev = urb->context;
> > + const struct ixxat_usb_adapter *adapter = dev->adapter;
> > + struct net_device *netdev = dev->netdev;
> > + struct usb_device *udev = dev->udev;
> > + int err;
> > +
> > + if (!netif_device_present(netdev))
> > + return;
> > +
> > + switch (urb->status) {
> > + case 0: /* success */
> > + break;
> > + case -EPROTO:
> > + case -EILSEQ:
> > + case -ENOENT:
> > + case -ECONNRESET:
> > + case -ESHUTDOWN:
> > + return;
> > + default:
> > + netdev_err(netdev, "Rx urb aborted /(%d)\n", urb->status);
> > + goto resubmit_urb;
> > + }
> > +
> > + if (urb->actual_length > 0)
> > + if (dev->state & IXXAT_USB_STATE_STARTED)
> > + err = ixxat_usb_decode_buf(urb);
>
> "err" not handled!
>
ixxat_usb_decode_buf prints error messages if something fails and this is
all we can do here. I will change ixxat_usb_decode_buf to a void function:
static void ixxat_usb_decode_buf(struct urb *urb)
> > +
> > +resubmit_urb:
> > + usb_fill_bulk_urb(urb, udev, usb_rcvbulkpipe(udev, dev->ep_msg_in),
> > + urb->transfer_buffer, adapter->buffer_size_rx,
> > + ixxat_usb_read_bulk, dev);
> > +
> > + usb_anchor_urb(urb, &dev->rx_submitted);
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (!err)
> > + return;
>
> Do you need "usb_unanchor_urb()" in case of return?
Wouldn't this stop the system from keeping track of the urb (and we won't
get an ixxat_usb_read_bulk callback)?
>
> > +
> > + usb_unanchor_urb(urb);
> > +
> > + if (err == -ENODEV)
> > + netif_device_detach(netdev);
> > + else
> > + netdev_err(netdev,
> > + "Error %d: Failed to resubmit read bulk urb\n", err);
> > +}
> > +
> > +static void ixxat_usb_write_bulk(struct urb *urb)
> > +{
> > + struct ixxat_tx_urb_context *context = urb->context;
> > + struct ixxat_usb_device *dev;
> > + struct net_device *netdev;
> > +
> > + if (WARN_ON(!context))
> > + return;
> > +
> > + dev = context->dev;
> > + netdev = dev->netdev;
>
> Could be assigned after the if case below.
>
> > + atomic_dec(&dev->active_tx_urbs);
> > +
> > + if (!netif_device_present(netdev))
> > + return;
Only if we change the if case to this:
if (!netif_device_present(context->dev->netdev))
Is this what you suggest?
> > + if (!urb->status) {
> > + netdev->stats.tx_packets += context->count;
> > + netdev->stats.tx_bytes += context->dlc;
> > + } else {
> > + netdev_err(netdev, "Error %d: Tx urb aborted\n", urb->status);
> > + }
> > +
> > + can_get_echo_skb(netdev, context->echo_index);
> > + context->echo_index = IXXAT_USB_MAX_TX_URBS;
> > +
> > + if (!urb->status)
> > + netif_wake_queue(netdev);
> > +}
I found another flaw here. In the next patch I will move the
"atomic_dec(&dev->active_tx_urbs)" right afer the
"context->echo_index = IXXAT_USB_MAX_TX_URBS".
Otherwise it could happen that when the ixxat_usb_start_xmit function
interrupts the ixxat_usb_write_bulk callback, the active_tx_urbs counter
is decremented but the context isnt marked free yet.
> > +static void ixxat_usb_encode_msg(struct ixxat_usb_device *dev,
> > + struct sk_buff *skb, u8 *obuf,
> > + size_t *size)
> > +{
> > + struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> > + struct ixxat_can_msg can_msg = {0};
> > + struct ixxat_can_msg_base *msg_base = &can_msg.base;
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + msg_base->flags |= IXXAT_USB_MSG_FLAGS_RTR;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + msg_base->flags |= IXXAT_USB_MSG_FLAGS_EXT;
> > + msg_base->msg_id = cf->can_id & CAN_EFF_MASK;
> > + } else {
> > + msg_base->msg_id = cf->can_id & CAN_SFF_MASK;
> > + }
> > +
> > + if (can_is_canfd_skb(skb)) {
> > + msg_base->flags |= IXXAT_USB_FDMSG_FLAGS_EDL;
> > +
> > + if (!(cf->can_id & CAN_RTR_FLAG) && (cf->flags & CANFD_BRS))
> > + msg_base->flags |= IXXAT_USB_FDMSG_FLAGS_FDR;
> > +
> > + msg_base->flags |= IXXAT_USB_ENCODE_DLC(can_len2dlc(cf->len));
> > + } else {
> > + msg_base->flags |= IXXAT_USB_ENCODE_DLC(cf->len);
> > + }
> > +
> > + le32_to_cpus(&msg_base->flags);
> > + le32_to_cpus(&msg_base->msg_id);
> > +
> > + msg_base->size = (u8)(sizeof(*msg_base) - 1);
>
> To save two lines in the if case, why not:
>
> msg_base->size = (u8)(sizeof(*msg_base) - 1 + cf->len);
>
> > + if (dev->adapter == &usb2can_cl1) {
> > + msg_base->size += (u8)(sizeof(can_msg.cl1) - CAN_MAX_DLEN);
> > + msg_base->size += cf->len;
> > + memcpy(can_msg.cl1.data, cf->data, cf->len);
> > + } else {
> > + msg_base->size += (u8)(sizeof(can_msg.cl2) - CANFD_MAX_DLEN);
> > + msg_base->size += cf->len;
> > + memcpy(can_msg.cl2.data, cf->data, cf->len);
> > + }
I will fix this in the next patch.
> > +
> > + *size = msg_base->size + 1;
> > + memcpy(obuf, &can_msg, *size);
> > + skb->data_len = *size;
>
> Why do you modify the skb here?
>
> > +}
This is not necessary. I will remove this in the next patch.
> > +static netdev_tx_t ixxat_usb_start_xmit(struct sk_buff *skb,
> > + struct net_device *netdev)
> > +{
> > + struct ixxat_usb_device *dev = netdev_priv(netdev);
> > + struct ixxat_tx_urb_context *context = NULL;
> > + struct net_device_stats *stats = &netdev->stats;
> > + struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> > + struct urb *urb;
> > + u8 *obuf;
> > + int i;
> > + int err;
> > + size_t size = dev->adapter->buffer_size_tx;
> > +
> > + if (can_dropped_invalid_skb(netdev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + for (i = 0; i < IXXAT_USB_MAX_TX_URBS; i++) {
> > + if (dev->tx_contexts[i].echo_index == IXXAT_USB_MAX_TX_URBS) {
> > + context = dev->tx_contexts + i;
> > + break;
> > + }
> > + }
> > +
> > + if (WARN_ON_ONCE(!context))
> > + return NETDEV_TX_BUSY;
> > +
> > + urb = context->urb;
> > + obuf = urb->transfer_buffer;
> > +
> > + ixxat_usb_encode_msg(dev, skb, obuf, &size);
> > +
> > + context->echo_index = i;
> > + context->dlc = cf->len;
> > + context->count = 1;
> > +
> > + urb->transfer_buffer_length = size;
> > + usb_anchor_urb(urb, &dev->tx_submitted);
> > + can_put_echo_skb(skb, netdev, context->echo_index);
> > + atomic_inc(&dev->active_tx_urbs);
> > +
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (unlikely(err)) {
>
> You use "unlikely" just once!
>
> > + can_free_echo_skb(netdev, context->echo_index);
> > + usb_unanchor_urb(urb);
> > + atomic_dec(&dev->active_tx_urbs);
> > +
> > + context->echo_index = IXXAT_USB_MAX_TX_URBS;
> > +
> > + if (err == -ENODEV) {
> > + netif_device_detach(netdev);
> > + } else {
> > + stats->tx_dropped++;
> > + netdev_err(netdev,
> > + "Error %d: Submitting tx-urb failed\n", err);
> > + }
> > + } else {
> > + if (atomic_read(&dev->active_tx_urbs) >= IXXAT_USB_MAX_TX_URBS)
> > + netif_stop_queue(netdev);
> > + }
> > +
> > + return NETDEV_TX_OK;
> > +}
The "unlikely" macro isn't really necessary. Would you suggest to use it
wherever a condition is unexpected or should we just remove it here?
> > +static int ixxat_usb_setup_tx_urbs(struct ixxat_usb_device *dev)
> > +{
> > + int i;
> > + int ret = 0;
> > + const struct ixxat_usb_adapter *adapter = dev->adapter;
> > + struct net_device *netdev = dev->netdev;
> > + struct usb_device *udev = dev->udev;
> > +
> > + for (i = 0; i < IXXAT_USB_MAX_TX_URBS; i++) {
> > + struct ixxat_tx_urb_context *context;
> > + struct urb *urb = NULL;
> > + u8 *buf = NULL;
>
> Preinitialisation not needed!
Will be fixed in the next patch.
Thank you very much for helping us to get the driver in proper shape!
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
