Hello Florian,
Am 20.07.2018 um 14:36 schrieb Florian Ferg:
> 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;
> }
I meant snd and rcv to be handled by just one function. Having the
return code include is also a good idea.
>>> +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:
That's fine!
> 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)?
Ah, OK, I was not aware of that!
>
>>
>>> +
>>> + 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?
Well, not really! Your code is correct. Sorry for the noise!
>
>>> + 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?
Well, it seems not to be conesequently used in other drivers, especially
the USB drivers. Either remove it or use "unlikely" in the "hot" path
handling RX and TX consistantly.
Looking for some recommendation when to use "likely/iunlikely" in the
kernel I found:
https://kernelnewbies.org/FAQ/LikelyUnlikely
Error cases are usually very very very unlikely.
>>> +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!
You are welcome!
Wolfgang.
--
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
