Hello Florian,
Am 04.07.2018 um 15:12 schrieb Florian Ferg:
> Hello Wolfgang,
>
> thank you very much for your review.
> Before I send another patch I want to discuss some of the issues:
>
>> You changed error handling to "if (ret < 0)" for *all* functions. Please
>> use "ret < 0" only, if a positive return code means success, otherwise
>> "if (err)".
>
> So we should use "if (ret < 0)" if the returncode on success can be
> greater than zero and "if (err)" if only zero means success?
Yes, I think you had that in the first place, at least partially. I
mainly complained about the "if (err < 0)" which should be "if (ret <
0)" but only, if the "ret >= 0" means success. That's *my* preference.
Many developers don't care about ret vs. err. It's a minor issue, though!
>>> +int ixxat_usb_send_cmd(struct usb_device *dev,
>>> + struct ixxat_usb_dal_req *dal_req)
>>> +{
>>> + int ret = 0;
>>> + int i;
>>> + u16 size;
>>> + u16 value;
>>> + u8 request;
>>> + u8 requesttype;
>>> + u8 *buf;
>>> +
>>> + request = 0xff;
>>> + requesttype = USB_TYPE_VENDOR | USB_DIR_OUT;
>>> + value = le16_to_cpu(dal_req->req_port);
>>> + size = le32_to_cpu(dal_req->req_size);
>>
>> Please initialize the values in the declararion above.
>>
>>> + size += sizeof(struct ixxat_usb_dal_res);
>>
>> le32_to_cpu?
>
> dal_req->req_size is __le32 and needs to be converted to __u32.
> sizeof() already returns an integer in cpu format.
Forget it. You convert first and than you add something.
>>> +
>>> + buf = kmalloc(size, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + memcpy(buf, (u8 *)dal_req, size);
>>
>> Cast needed?
>>
>> I'm trying to understand what you are doing here. You assume that behind
>> "dal_req" is "struct ixxat_usb_dal_res". That's not really transparent!
>
> The IXXAT device command always starts with a "struct ixxat_usb_dal_req".
> Then there may or may not follow some special command data which has the
> size stored in dal_req->req_size.
> After the special command data there has to be a
> "struct ixxat_usb_dal_res" indeed. The firmware needs to know the expected
> response size which is stored in this response structure.
>
> Should we use a command structure like this?
>
> struct ixxat_usb_cmd {
> struct ixxat_usb_req req;
> union {
> struct ixxat_usb_caps_req caps_req;
> struct ixxat_usb_init_cl1_req init_cl1_req;
> struct ixxat_usb_init_cl2_req init_cl2_req;
Just one of the lines above, I assume.
> struct ixxat_usb_start_req start_req;
> struct ixxat_usb_stop_req stop_req;
> struct ixxat_usb_power_req power_req;
> struct ixxat_usb_info_req info_req;
> };
> struct ixxat_usb_res res;
> union {
> struct ixxat_usb_caps_res caps_res;
> struct ixxat_usb_init_cl1_res init_cl1_res;
> struct ixxat_usb_init_cl2_res init_cl2_res;
> struct ixxat_usb_start_res start_res;
> struct ixxat_usb_stop_res stop_res;
> struct ixxat_usb_power_res power_res;
> struct ixxat_usb_info_res info_res;
> };
> };
That's more transparent, at least. But do you need the union?
See below:
>
> But then we still need this:
>
> int ixxat_usb_send_cmd(struct usb_device *dev,
> struct ixxat_usb_cmd *cmd)
> {
> ...
> u16 size = le32_to_cpu(cmd->req.size) + sizeof(cmd->res)
> ...
> }
As this is the common part, you probably should provide the value, buf
and size as function arguments.
int ixxat_usb_send_cmd(struct usb_device *dev, u16 value, void *buf, u16
size)
Then you could even save the memcpy()? Does that make sense?
>
>>> +static int ixxat_usb_handle_canmsg(struct ixxat_usb_device *dev,
>>> + struct ixxat_can_msg *rx)
>>> +{
>>> + struct net_device *netdev = dev->netdev;
>>> + struct canfd_frame *can_frame;
>>> + struct sk_buff *skb;
>>> + const u32 flags = le32_to_cpu(rx->flags);
>>> + const u8 dlc = IXXAT_USB_DECODE_DLC(flags);
>>> +
>>> + if (flags & IXXAT_USB_FDMSG_FLAGS_EDL)
>>> + skb = alloc_canfd_skb(netdev, &can_frame);
>>> + else
>>> + skb = alloc_can_skb(netdev, (struct can_frame **)&can_frame);
>>
>> Argh, we should avoid such ugly casts! But I find it in other drivers as
>> well :(.
>
> We could use a union like this:
>
> union can_canfd_frame {
> struct can_frame *cf;
> struct canfd_frame *cfd;
> };
Yes, should be in the CAN header file then. Oliver, what do you think?
>
> static int ixxat_usb_handle_canmsg(struct ixxat_usb_device *dev,
> struct ixxat_can_msg *rx)
> {
> struct net_device *netdev = dev->netdev;
> union can_canfd_frame ccfu;
> struct sk_buff *skb;
> ...
>
> if (flags & IXXAT_USB_FDMSG_FLAGS_EDL)
> skb = alloc_canfd_skb(netdev, &ccfu.cfd);
> else
> skb = alloc_can_skb(netdev, &ccfu.cf);
>
> ...
> }
>
> Do you have any better suggestions to avoid the casts?
For the moment leave it as is!
>
>>> +static int ixxat_usb_handle_status(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;
>>> +
>>> + u32 raw_status = le32_to_cpu(*(u32 *)(rx->data));
>>> + enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
>>> +
>>> + skb = alloc_can_err_skb(netdev, &can_frame);
>>> + if (!skb)
>>> + return -ENOMEM;
>>> +
>>> + if (raw_status == IXXAT_USB_CAN_STATUS_OK) {
>>> + dev->can.state = CAN_STATE_ERROR_ACTIVE;
>>> + can_frame->can_id |= CAN_ERR_CRTL;
>>> + can_frame->data[1] |= CAN_ERR_CRTL_ACTIVE;
>>> + } else if (raw_status & IXXAT_USB_CAN_STATUS_BUSOFF) {
>>> + can_frame->can_id |= CAN_ERR_BUSOFF;
>>> + dev->can.can_stats.bus_off++;
>>> + new_state = CAN_STATE_BUS_OFF;
>>> + can_bus_off(netdev);
>>
>> Does the device recover automatically from bus-off?
>
> No, it doesn't. Is this a problem here?
No. I'm just curious.
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
