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?
> > +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.
> > +
> > + 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;
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;
};
};
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)
...
}
> > +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;
};
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?
> > +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?
Kind regards,
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
