| 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: Mon, 19 Jun 2023 10:25:27 -0400
From: Eric Stahl <ericstahl@...ntech.com>
To: Vincent Mailhol <vincent.mailhol@...il.com>,
Peter Seiderer <ps.report@....net>, marm@...-networks.de
Cc: linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
Wolfgang Grandegger <wg@...ndegger.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9] can: usb: IXXAT USB-to-CAN adapters drivers
Hi all,
The socketcan@...-networks.de is bouncing for me. I removed both Florian
Ferg's email (flfe@...-networks.de) and sockectcan@...-networks.de. I
saw Markus from hms commenting on the last thread (PATCH v8) resolving
those issues, so I've added him to this chain. I'm not sure if they are
still a problem.
I've backported Peter's PATCH v9 and it works well for me. It seems that
hms is distributing a driver that does not incorporate the proposed
changes from the past reviews
(https://forum.hms-networks.com/t/socketcan-driver-for-linux-20-04/70299/30).
Their driver has not worked for me with higher level CANopen libraries,
but Peter's v8/v9 and Florian's past contributions work well. I would
like to get ixxat's active pci socketcan drivers pulled into the mainline.
I don't mean to hijack this thread, but I was wondering if it's
appropriate to add those pci drivers to this patch to get reviewed, or
if it's more appropriate to open a separate patch.
Thanks,
Eric
On 5/31/23 03:25, Vincent Mailhol wrote:
> Hi Florian,
>
> Here is another batch of comments.
>
> On Tue. 23 May 2023 at 05:02, Peter Seiderer <ps.report@....net> wrote:
>> From: Florian Ferg <flfe@...-networks.de>
>>
>> This patch adds the driver for the IXXAT USB-to-CAN interfaces. There
>> is an adapter for the older communication layer cl1 and another
>> adapter for the newer communication layer cl2.
>>
>> Signed-off-by: Florian Ferg <flfe@...-networks.de>
>> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>> Signed-off-by: Peter Seiderer <ps.report@....net>
>> ---
>> Changes v1 -> v7 (Florian Ferg <flfe@...-networks.de>):
>> - for detailed history see
>> https://codeberg.org/psreport/socketcan-linux-ix-usb-can
>>
>> Changes v7 -> v8 (Marc Kleine-Budde <mkl@...gutronix.de>)
>> - port to recent net-next
>>
>> Changes v8 -> v9 (Peter Seiderer <ps.report@....net>)
>> Addressed review comments by Vincent Mailhol:
>> - update copyright headers (use SPDX identifier 'GPL-2.0-only'
>> instead of 'GPL-2.0', remove full GPL boilerplate)
>> - reorder includes (lexicographic order)
>> - remove IXXAT_USB_MODES defines (move modes directly in the
>> declaration of structs)
>> - remove bittiming defines (move directly in declaration of structs)
>> - remove _EP1_IN/_EP1_OUT defines (move directly in declaration of structs)
>> - remove rcv_size/snd_size vars (use sizeof(*cmd) and sizeof(cmd->res) directly)
>> - use GENMASK/FIELD_PREP (IXXAT_USB_CAN_BTR1_TIME_SEG1_MASK/IXXAT_USB_CAN_BTR1_TIME_SEG2_MASK)
>> - update dev_err/netdev_err (use %pe, remove 'Error:' prefix)
>> - use U32_MAX instead of 0x00000000FFFFFFFF
>> - do not increase the rx_bytes count when receiving a remove frame
>> - ixxat_can_msg_cl1/ixxat_can_msg_cl2 use union __le32 status
>> - do not increase the statistics for error frames
>> - do not use parenthesis in log messages
>> - remove double brackets (sparse workaround for struct init)
>> - fill netdev->ethtool_ops with default
>> - fill netdev->dev_port
>> - use FIELD_GET/FIELD_PREP instead of IXXAT_USB_DECODE_DLC/IXXAT_USB_ENCODE_DLC
>> - use driver_info intead of open coded ixxat_usb_get_adapter
>> Addressed checkpatch.pl warnings:
>> - change MODULE_LICENSE to 'GPL' - checkpatch.pl WARNING: Prefer "GPL" over "GPL v2"
>> ---
>> drivers/net/can/usb/Kconfig | 17 +
>> drivers/net/can/usb/Makefile | 1 +
>> drivers/net/can/usb/ixxat_usb/Makefile | 2 +
>> drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c | 100 ++
>> drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c | 176 +++
>> .../net/can/usb/ixxat_usb/ixxat_usb_core.c | 1277 +++++++++++++++++
>> .../net/can/usb/ixxat_usb/ixxat_usb_core.h | 511 +++++++
>> 7 files changed, 2084 insertions(+)
>> create mode 100644 drivers/net/can/usb/ixxat_usb/Makefile
>> create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c
>> create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
>> create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
>> create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h
>>
>> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
>> index 58fcd2b34820..b50706f2a5e6 100644
>> --- a/drivers/net/can/usb/Kconfig
>> +++ b/drivers/net/can/usb/Kconfig
>> @@ -62,6 +62,23 @@ config CAN_GS_USB
>> choose Y for built in support,
>> M to compile as module (module will be named: gs_usb).
>>
>> +config CAN_IXXAT_USB
>> + tristate "IXXAT USB-to-CAN interfaces"
>> + help
>> + This driver adds support for IXXAT USB-to-CAN devices.
>> +
>> + The driver provides support for the following devices:
>> + - IXXAT USB-to-CAN compact
>> + - IXXAT USB-to-CAN embedded
>> + - IXXAT USB-to-CAN professional
>> + - IXXAT USB-to-CAN automotive
>> + - IXXAT USB-to-CAN FD compact
>> + - IXXAT USB-to-CAN FD professional
>> + - IXXAT USB-to-CAN FD automotive
>> + - IXXAT USB-to-CAN FD MiniPCIe
>> + - IXXAT USB-to-CAR
>> + - IXXAT CAN-IDM101
>> +
>> config CAN_KVASER_USB
>> tristate "Kvaser CAN/USB interface"
>> help
>> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
>> index 8b11088e9a59..52b4cc66ff30 100644
>> --- a/drivers/net/can/usb/Makefile
>> +++ b/drivers/net/can/usb/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_CAN_ESD_USB) += esd_usb.o
>> obj-$(CONFIG_CAN_ETAS_ES58X) += etas_es58x/
>> obj-$(CONFIG_CAN_F81604) += f81604.o
>> obj-$(CONFIG_CAN_GS_USB) += gs_usb.o
>> +obj-$(CONFIG_CAN_IXXAT_USB) += ixxat_usb/
>> obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
>> obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
>> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>> diff --git a/drivers/net/can/usb/ixxat_usb/Makefile b/drivers/net/can/usb/ixxat_usb/Makefile
>> new file mode 100644
>> index 000000000000..125d2705979f
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ixxat_usb/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_CAN_IXXAT_USB) += ixxat_usb2can.o
>> +ixxat_usb2can-y = ixxat_usb_cl1.o ixxat_usb_cl2.o ixxat_usb_core.o
>> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c
>> new file mode 100644
>> index 000000000000..cfd95ff722cd
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2018 HMS Industrial Networks <socketcan@...-networks.de>
>> + */
>> +
>> +#include <linux/can/dev.h>
>> +#include <linux/kernel.h>
>> +#include <linux/usb.h>
>> +
>> +#include "ixxat_usb_core.h"
>> +
>> +#define IXXAT_USB_CLOCK 8000000
>
> Nitpick, use the MEGA suffix for linux/units.h
>
> #define IXXAT_USB_CLOCK (8 * MEGA /* Hz */)
>
>> +#define IXXAT_USB_BUFFER_SIZE_RX 512
>> +#define IXXAT_USB_BUFFER_SIZE_TX 256
>> +
>> +#define IXXAT_USB_BTMODE_TSM_CL1 0x80
>> +
>> +#define IXXAT_USB_CAN_CMD_INIT 0x325
>> +
>> +#define IXXAT_USB_CAN_BTR0_BRP_MASK GENMASK(5, 0)
>> +#define IXXAT_USB_CAN_BTR0_SJW_MASK GENMASK(7, 6)
>> +
>> +#define IXXAT_USB_CAN_BTR1_TIME_SEG1_MASK GENMASK(3, 0)
>> +#define IXXAT_USB_CAN_BTR1_TIME_SEG2_MASK GENMASK(6, 4)
>> +
>> +static const struct can_bittiming_const usb2can_bt = {
>> + .name = IXXAT_USB_CTRL_NAME,
>> + .tseg1_min = 1,
>> + .tseg1_max = 16,
>> + .tseg2_min = 1,
>> + .tseg2_max = 8,
>> + .sjw_max = 4,
>> + .brp_min = 1,
>> + .brp_max = 64,
>> + .brp_inc = 1,
>> +};
>> +
>> +static int ixxat_usb_init_ctrl(struct ixxat_usb_device *dev)
>> +{
>> + const struct can_bittiming *bt = &dev->can.bittiming;
>> + const u16 port = dev->ctrl_index;
>> + int err;
>> + struct ixxat_usb_init_cl1_cmd *cmd;
>> + u8 opmode = IXXAT_USB_OPMODE_EXTENDED | IXXAT_USB_OPMODE_STANDARD;
>> + u8 btr0 = FIELD_PREP(IXXAT_USB_CAN_BTR0_BRP_MASK, bt->brp - 1) |
>> + FIELD_PREP(IXXAT_USB_CAN_BTR0_SJW_MASK, bt->sjw - 1);
>> + u8 btr1 = FIELD_PREP(IXXAT_USB_CAN_BTR1_TIME_SEG1_MASK, bt->prop_seg + bt->phase_seg1 - 1) |
>> + FIELD_PREP(IXXAT_USB_CAN_BTR1_TIME_SEG2_MASK, bt->phase_seg2 - 1);
> Remove those opmode, btr0 and btr1 variables and directly assign the
> values to cmd->opmode, cmd->btr0 and cmd->btr1.
>
> The issue here is that the use of these variables get scattered for no reason.
>
> For example, opmode is initialised just above...
>
>> + cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>> + btr1 |= IXXAT_USB_BTMODE_TSM_CL1;
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
>> + opmode |= IXXAT_USB_OPMODE_ERRFRAME;
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>> + opmode |= IXXAT_USB_OPMODE_LISTONLY;
> ... then specific flags get assigned here in the middle of the function ...
>
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.size = cpu_to_le32(sizeof(*cmd) - sizeof(cmd->res));
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_CAN_CMD_INIT);
>> + cmd->req.port = cpu_to_le16(port);
>> + cmd->mode = opmode;
> ... and finally, you assign it here.
>
> Instead of having the pieces of code scattered in three places, you could do:
>
> cmd->mode = IXXAT_USB_OPMODE_EXTENDED | IXXAT_USB_OPMODE_STANDARD;
> if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> cmd->opmode |= IXXAT_USB_OPMODE_ERRFRAME;
> if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> cmd->opmode |= IXXAT_USB_OPMODE_LISTONLY;
>
> in one single block of code. This is easier to read because now I do
> not have to look at different places to understand what the function
> does with that opmode.
>
> This comment applies to other functions as well. There are many uses
> of intermediate variables which could easily be omitted.
>
>> + cmd->btr0 = btr0;
>> + cmd->btr1 = btr1;
>> +
>> + err = ixxat_usb_send_cmd(dev->udev, port, cmd, sizeof(*cmd), &cmd->res,
>> + sizeof(cmd->res));
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +const struct ixxat_usb_adapter usb2can_cl1 = {
>> + .clock = IXXAT_USB_CLOCK,
>> + .bt = &usb2can_bt,
>> + .btd = NULL,
>> + .modes = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_BERR_REPORTING |
>> + CAN_CTRLMODE_LISTENONLY,
>> + .buffer_size_rx = IXXAT_USB_BUFFER_SIZE_RX,
>> + .buffer_size_tx = IXXAT_USB_BUFFER_SIZE_TX,
>> + .ep_msg_in = {
>> + 1 | USB_DIR_IN,
>> + 2 | USB_DIR_IN,
>> + 3 | USB_DIR_IN,
>> + 4 | USB_DIR_IN,
>> + 5 | USB_DIR_IN,
>> + },
>> + .ep_msg_out = {
>> + 1 | USB_DIR_OUT,
>> + 2 | USB_DIR_OUT,
>> + 3 | USB_DIR_OUT,
>> + 4 | USB_DIR_OUT,
>> + 5 | USB_DIR_OUT,
>> + },
>> + .ep_offs = 0,
>> + .init_ctrl = ixxat_usb_init_ctrl
>> +};
>> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
>> new file mode 100644
>> index 000000000000..268544f52f1e
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
>> @@ -0,0 +1,176 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2018 HMS Industrial Networks <socketcan@...-networks.de>
>> + */
>> +
>> +#include <linux/can/dev.h>
>> +#include <linux/kernel.h>
>> +#include <linux/usb.h>
>> +
>> +#include "ixxat_usb_core.h"
>> +
>> +#define IXXAT_USB_CLOCK 80000000
> Nitpick, use the MEGA suffix for linux/units.h
>
> #define IXXAT_USB_CLOCK (80 * MEGA /* Hz */)
>
>> +#define IXXAT_USB_BUFFER_SIZE_RX 512
>> +#define IXXAT_USB_BUFFER_SIZE_TX 512
>> +
>> +#define IXXAT_USB_CAN_CMD_INIT 0x337
>> +
>> +static const struct can_bittiming_const usb2can_bt = {
>> + .name = IXXAT_USB_CTRL_NAME,
>> + .tseg1_min = 1,
>> + .tseg1_max = 256,
>> + .tseg2_min = 1,
>> + .tseg2_max = 256,
>> + .sjw_max = 128,
>> + .brp_min = 2,
>> + .brp_max = 513,
> 513 is really uncommon. Are you sure this isn't 512?
>
>> + .brp_inc = 1,
>> +};
>> +
>> +static const struct can_bittiming_const usb2can_btd = {
>> + .name = IXXAT_USB_CTRL_NAME,
>> + .tseg1_min = 1,
>> + .tseg1_max = 256,
>> + .tseg2_min = 1,
>> + .tseg2_max = 256,
>> + .sjw_max = 128,
>> + .brp_min = 2,
>> + .brp_max = 513,
> Idem.
>
>> + .brp_inc = 1,
>> +};
>> +
>> +static const struct can_bittiming_const canidm_bt = {
>> + .name = IXXAT_USB_CTRL_NAME,
>> + .tseg1_min = 1,
>> + .tseg1_max = 256,
>> + .tseg2_min = 1,
>> + .tseg2_max = 128,
>> + .sjw_max = 128,
>> + .brp_min = 1,
>> + .brp_max = 512,
>> + .brp_inc = 1
>> +};
>> +
>> +static const struct can_bittiming_const canidm_btd = {
>> + .name = IXXAT_USB_CTRL_NAME,
>> + .tseg1_min = 1,
>> + .tseg1_max = 32,
>> + .tseg2_min = 1,
>> + .tseg2_max = 16,
>> + .sjw_max = 8,
>> + .brp_min = 1,
>> + .brp_max = 32,
>> + .brp_inc = 1
>> +};
>> +
>> +static int ixxat_usb_init_ctrl(struct ixxat_usb_device *dev)
>> +{
>> + const struct can_bittiming *bt = &dev->can.bittiming;
>> + const struct can_bittiming *btd = &dev->can.data_bittiming;
>> + const u16 port = dev->ctrl_index;
>> + int err;
>> + struct ixxat_usb_init_cl2_cmd *cmd;
>> + u32 btmode = IXXAT_USB_BTMODE_NAT;
>> + u8 exmode = 0;
>> + u8 opmode = IXXAT_USB_OPMODE_EXTENDED | IXXAT_USB_OPMODE_STANDARD;
>> +
>> + cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>> + btmode = IXXAT_USB_BTMODE_TSM;
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
>> + opmode |= IXXAT_USB_OPMODE_ERRFRAME;
>> + if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>> + opmode |= IXXAT_USB_OPMODE_LISTONLY;
>> + if (dev->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>> + exmode |= IXXAT_USB_EXMODE_EXTDATA | IXXAT_USB_EXMODE_FASTDATA;
>> +
>> + if (!(dev->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO))
>> + exmode |= IXXAT_USB_EXMODE_ISOFD;
>> + }
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.size = cpu_to_le32(sizeof(*cmd) - sizeof(cmd->res));
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_CAN_CMD_INIT);
>> + cmd->req.port = cpu_to_le16(port);
>> + cmd->opmode = opmode;
>> + cmd->exmode = exmode;
>> + cmd->sdr.mode = cpu_to_le32(btmode);
>> + cmd->sdr.bps = cpu_to_le32(bt->brp);
> The BPS (bitrate per second) and the BRP (bitrate prescaler) are two
> different things. I am not sure how this is supposed to work.
>
>> + cmd->sdr.ts1 = cpu_to_le16(bt->prop_seg + bt->phase_seg1);
>> + cmd->sdr.ts2 = cpu_to_le16(bt->phase_seg2);
>> + cmd->sdr.sjw = cpu_to_le16(bt->sjw);
>> + cmd->sdr.tdo = 0;
>> +
>> + if (exmode) {
>> + cmd->fdr.mode = cpu_to_le32(btmode);
>> + cmd->fdr.bps = cpu_to_le32(btd->brp);
>> + cmd->fdr.ts1 = cpu_to_le16(btd->prop_seg + btd->phase_seg1);
>> + cmd->fdr.ts2 = cpu_to_le16(btd->phase_seg2);
>> + cmd->fdr.sjw = cpu_to_le16(btd->sjw);
>> + cmd->fdr.tdo = cpu_to_le16(btd->brp * (btd->phase_seg1 + 1 +
>> + btd->prop_seg));
>> + }
>> +
>> + err = ixxat_usb_send_cmd(dev->udev, port, cmd, sizeof(*cmd), &cmd->res,
>> + sizeof(cmd->res));
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +const struct ixxat_usb_adapter usb2can_cl2 = {
>> + .clock = IXXAT_USB_CLOCK,
>> + .bt = &usb2can_bt,
>> + .btd = &usb2can_btd,
>> + .modes = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
>> + CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_FD |
>> + CAN_CTRLMODE_FD_NON_ISO,
> Does your device allow you to send and receive Classical CAN frames
> with DLC greater than 8? c.f. CAN_CTRLMODE_CC_LEN8_DLC
>
> https://elixir.bootlin.com/linux/v6.3/source/include/uapi/linux/can/netlink.h#L103
>
>> + .buffer_size_rx = IXXAT_USB_BUFFER_SIZE_RX,
>> + .buffer_size_tx = IXXAT_USB_BUFFER_SIZE_TX,
>> + .ep_msg_in = {
>> + 1 | USB_DIR_IN,
>> + 2 | USB_DIR_IN,
>> + 3 | USB_DIR_IN,
>> + 4 | USB_DIR_IN,
>> + 5 | USB_DIR_IN
>> + },
>> + .ep_msg_out = {
>> + 1 | USB_DIR_OUT,
>> + 2 | USB_DIR_OUT,
>> + 3 | USB_DIR_OUT,
>> + 4 | USB_DIR_OUT,
>> + 5 | USB_DIR_OUT,
>> + },
>> + .ep_offs = 1,
>> + .init_ctrl = ixxat_usb_init_ctrl
>> +};
>> +
>> +const struct ixxat_usb_adapter can_idm = {
>> + .clock = IXXAT_USB_CLOCK,
>> + .bt = &canidm_bt,
>> + .btd = &canidm_btd,
>> + .modes = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
>> + CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_FD |
>> + CAN_CTRLMODE_FD_NON_ISO,
>> + .buffer_size_rx = IXXAT_USB_BUFFER_SIZE_RX,
>> + .buffer_size_tx = IXXAT_USB_BUFFER_SIZE_TX,
>> + .ep_msg_in = {
>> + 2 | USB_DIR_IN,
>> + 4 | USB_DIR_IN,
>> + 6 | USB_DIR_IN,
>> + 8 | USB_DIR_IN,
>> + 10 | USB_DIR_IN,
>> + },
>> + .ep_msg_out = {
>> + 1 | USB_DIR_OUT,
>> + 3 | USB_DIR_OUT,
>> + 5 | USB_DIR_OUT,
>> + 7 | USB_DIR_OUT,
>> + 9 | USB_DIR_OUT,
>> + },
>> + .ep_offs = 0,
>> + .init_ctrl = ixxat_usb_init_ctrl
>> +};
>> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
>> new file mode 100644
>> index 000000000000..8787d27a3886
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
>> @@ -0,0 +1,1277 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2018 HMS Industrial Networks <socketcan@...-networks.de>
>> + */
>> +
>> +#include <linux/can/dev.h>
>> +#include <linux/ethtool.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kthread.h>
>> +#include <linux/module.h>
>> +#include <linux/usb.h>
>> +
>> +#include "ixxat_usb_core.h"
>> +
>> +MODULE_AUTHOR("Marcel Schmidt <socketcan@...-networks.de>");
>> +MODULE_DESCRIPTION("CAN driver for IXXAT USB-to-CAN / CAN FD adapters");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Table of devices that work with this driver */
>> +static const struct usb_device_id ixxat_usb_table[] = {
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_COMPACT_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl1, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_EMBEDDED_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl1, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_PROFESSIONAL_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl1, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_AUTOMOTIVE_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl1, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_FD_COMPACT_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl2, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_FD_PROFESSIONAL_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl2, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_FD_AUTOMOTIVE_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl2, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAN_FD_PCIE_MINI_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl2, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, USB2CAR_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&usb2can_cl2, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, CAN_IDM101_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&can_idm, },
>> + { USB_DEVICE(IXXAT_USB_VENDOR_ID, CAN_IDM200_PRODUCT_ID),
>> + .driver_info = (kernel_ulong_t)&can_idm, },
>> + { } /* Terminating entry */
>> +};
>> +
>> +MODULE_DEVICE_TABLE(usb, ixxat_usb_table);
>> +
>> +void ixxat_usb_setup_cmd(struct ixxat_usb_dal_req *req,
>> + struct ixxat_usb_dal_res *res)
>> +{
>> + req->size = cpu_to_le32(sizeof(*req));
>> + req->port = cpu_to_le16(0xffff);
> U16_MAX. Alternatively, if this value has a semantic meaning, declare
> it as a #define. Something like that:
>
> #define IXXAT_USB_PORT_UNDEF U16_MAX
>
>> + req->socket = cpu_to_le16(0xffff);
> Same.
>
>> + req->code = cpu_to_le32(0);
>> +
>> + res->res_size = cpu_to_le32(sizeof(*res));
>> + res->ret_size = cpu_to_le32(0);
>> + res->code = cpu_to_le32(0xffffffff);
> Same but with U32_MAX.
>
>> +}
>> +
>> +int ixxat_usb_send_cmd(struct usb_device *dev, const u16 port, void *req,
> ^^^^^^^^^
>
> If you declare some of the arguments as const, follow the logic until
> the end. As far as I can see, req is constant.
>
> int ixxat_usb_send_cmd(struct usb_device *dev, const u16 port, const
> void *req,
>
>> + const u16 req_size, void *res, const u16 res_size)
>> +{
>> + const int to = msecs_to_jiffies(IXXAT_USB_MSG_TIMEOUT);
>> + const u8 rq = 0xff;
> Here you have req as a parameter and rq as a variable. The names are
> too similar (both are an abbreviation of request) and this can be
> misleading. Try to be consistent within your different functions.
> Instead of "req", use for example "cmd".
>
>> + const u8 rti = USB_TYPE_VENDOR | USB_DIR_IN;
>> + const u8 rto = USB_TYPE_VENDOR | USB_DIR_OUT;
>> + int i;
>> + int pos = 0;
>> + int rcp = usb_rcvctrlpipe(dev, 0);
>> + int scp = usb_sndctrlpipe(dev, 0);
>> + int ret = 0;
>> + struct ixxat_usb_dal_res *dal_res = res;
>> +
>> + for (i = 0; i < IXXAT_USB_MAX_COM_REQ; ++i) {
>> + ret = usb_control_msg(dev, scp, rq, rto, port, 0, req, req_size,
>> + to);
>> + if (ret < 0)
>> + msleep(IXXAT_USB_MSG_CYCLE);
>> + else
>> + break;
>> + }
>> +
>> + if (ret < 0) {
>> + dev_err(&dev->dev, "TX command failure: %pe\n", ERR_PTR(ret));
>> + goto fail;
>> + }
>> +
>> + for (i = 0; i < IXXAT_USB_MAX_COM_REQ; ++i) {
>> + const int rs = res_size - pos;
>> + void *rb = res + pos;
>> +
>> + ret = usb_control_msg(dev, rcp, rq, rti, port, 0, rb, rs, to);
>> + if (ret < 0) {
>> + msleep(IXXAT_USB_MSG_CYCLE);
>> + continue;
>> + }
>> +
>> + pos += ret;
>> + if (pos < res_size)
>> + msleep(IXXAT_USB_MSG_CYCLE);
>> + else
>> + break;
>> + }
>> +
>> + if (pos != res_size)
>> + ret = -EBADMSG;
>> +
>> + if (ret < 0) {
>> + dev_err(&dev->dev, "RX command failure: %pe\n", ERR_PTR(ret));
>> + goto fail;
>> + }
>> +
>> + ret = le32_to_cpu(dal_res->code);
>> +
>> +fail:
>> + return ret;
>> +}
>> +
>> +static void ixxat_usb_update_ts_now(struct ixxat_usb_device *dev, u32 ts_now)
>> +{
>> + u32 *ts_dev = &dev->time_ref.ts_dev_0;
>> + ktime_t *kt_host = &dev->time_ref.kt_host_0;
>> + u64 timebase = (u64)U32_MAX - (u64)(*ts_dev) + (u64)ts_now;
>> +
>> + *kt_host = ktime_add_us(*kt_host, timebase);
>> + *ts_dev = ts_now;
>> +}
>> +
>> +static void ixxat_usb_get_ts_tv(struct ixxat_usb_device *dev, u32 ts,
>> + ktime_t *k_time)
>> +{
>> + ktime_t tmp_time = dev->time_ref.kt_host_0;
>> +
>> + if (ts < dev->time_ref.ts_dev_last)
>> + ixxat_usb_update_ts_now(dev, ts);
>> +
>> + dev->time_ref.ts_dev_last = ts;
>> + tmp_time = ktime_add_us(tmp_time, ts - dev->time_ref.ts_dev_0);
>> +
>> + if (k_time)
>> + *k_time = tmp_time;
>> +}
>> +
>> +static void ixxat_usb_set_ts_now(struct ixxat_usb_device *dev, u32 ts_now)
>> +{
>> + dev->time_ref.ts_dev_0 = ts_now;
>> + dev->time_ref.kt_host_0 = ktime_get_real();
>> + dev->time_ref.ts_dev_last = ts_now;
>> +}
>> +
>> +static int ixxat_usb_get_dev_caps(struct usb_device *dev,
>> + struct ixxat_dev_caps *dev_caps)
>> +{
>> + int i;
>> + int err;
>> + struct ixxat_usb_caps_cmd *cmd;
>> + const u32 cmd_size = sizeof(*cmd);
> As a global comment, those *_size variables do not have a strong
> purpose. Using directly sizeof(*cmd) in the code is unambiguous and
> concise enough. This comment applies to other functions as well.
>
>> + const u32 req_size = sizeof(cmd->req);
>> + const u32 rcv_size = cmd_size - req_size;
> Here, one thing you could do is:
>
> #define IXXAT_USB_RCV_SIZE(cmd) (sizeof(*cmd) + sizeof(cmd->req))
>
> and then use IXXAT_USB_RCV_SIZE(cmd) in place of rcv_size in your code.
>
>> + const u32 snd_size = req_size + sizeof(cmd->res);
>> + u16 num_ctrl;
>> +
>> + cmd = kmalloc(cmd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_BRD_CMD_GET_DEVCAPS);
>> + cmd->res.res_size = cpu_to_le32(rcv_size);
>> +
>> + err = ixxat_usb_send_cmd(dev, le16_to_cpu(cmd->req.port), cmd, snd_size,
>> + &cmd->res, rcv_size);
>> + if (err)
>> + goto fail;
>> + dev_caps->bus_ctrl_count = cmd->caps.bus_ctrl_count;
>> + num_ctrl = le16_to_cpu(dev_caps->bus_ctrl_count);
>> + if (num_ctrl > ARRAY_SIZE(dev_caps->bus_ctrl_types)) {
>> + err = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + for (i = 0; i < num_ctrl; i++)
>> + dev_caps->bus_ctrl_types[i] = cmd->caps.bus_ctrl_types[i];
>> +
>> +fail:
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_get_dev_info(struct usb_device *dev,
>> + struct ixxat_dev_info *dev_info)
>> +{
>> + int err;
>> + struct ixxat_usb_info_cmd *cmd;
>> + const u32 cmd_size = sizeof(*cmd);
>> + const u32 req_size = sizeof(cmd->req);
>> + const u32 rcv_size = cmd_size - req_size;
>> + const u32 snd_size = req_size + sizeof(cmd->res);
>> +
>> + cmd = kmalloc(cmd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_BRD_CMD_GET_DEVINFO);
>> + cmd->res.res_size = cpu_to_le32(rcv_size);
>> +
>> + err = ixxat_usb_send_cmd(dev, le16_to_cpu(cmd->req.port), cmd, snd_size,
>> + &cmd->res, rcv_size);
>> + if (err)
>> + goto fail;
> This goto label is used only once. You can directly return here:
>
> if (err) {
> kfree(cmd);
> return err;
> }
>
>> + if (dev_info) {
>> + memcpy(dev_info->device_id, &cmd->info.device_id,
>> + sizeof(cmd->info.device_id));
>> + memcpy(dev_info->device_name, &cmd->info.device_name,
>> + sizeof(cmd->info.device_name));
>> + dev_info->device_fpga_version = cmd->info.device_fpga_version;
>> + dev_info->device_version = cmd->info.device_version;
>> + }
>> +
>> +fail:
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_start_ctrl(struct ixxat_usb_device *dev, u32 *time_ref)
>> +{
>> + const u16 port = dev->ctrl_index;
>> + int err;
>> + struct ixxat_usb_start_cmd *cmd;
>> + const u32 cmd_size = sizeof(*cmd);
>> + const u32 req_size = sizeof(cmd->req);
>> + const u32 rcv_size = cmd_size - req_size;
>> + const u32 snd_size = req_size + sizeof(cmd->res);
>> +
>> + cmd = kmalloc(cmd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_CAN_CMD_START);
>> + cmd->req.port = cpu_to_le16(port);
>> + cmd->res.res_size = cpu_to_le32(rcv_size);
>> + cmd->time = 0;
>> +
>> + err = ixxat_usb_send_cmd(dev->udev, port, cmd, snd_size, &cmd->res,
>> + rcv_size);
>> + if (err)
>> + goto fail;
>> +
>> + if (time_ref)
> I do not think this if () is needed. All the callers of
> ixxat_usb_start_ctrl() pass a valid pointer.
>
>> + *time_ref = le32_to_cpu(cmd->time);
> I do not get the logic. cmd is not modified by ixxat_usb_send_cmd(),
> right? So, here, cmd->time would still be zero. Am I missing
> something?
>
>> +
>> +fail:
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_stop_ctrl(struct ixxat_usb_device *dev)
>> +{
>> + const u16 port = dev->ctrl_index;
>> + int err;
>> + struct ixxat_usb_stop_cmd *cmd;
>> + const u32 rcv_size = sizeof(cmd->res);
>> + const u32 snd_size = sizeof(*cmd);
>> +
>> + cmd = kmalloc(snd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.size = cpu_to_le32(snd_size - rcv_size);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_CAN_CMD_STOP);
>> + cmd->req.port = cpu_to_le16(port);
>> + cmd->action = cpu_to_le32(IXXAT_USB_STOP_ACTION_CLEARALL);
>> +
>> + err = ixxat_usb_send_cmd(dev->udev, port, cmd, snd_size, &cmd->res,
>> + rcv_size);
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_power_ctrl(struct usb_device *dev, u8 mode)
>> +{
>> + int err;
>> + struct ixxat_usb_power_cmd *cmd;
>> + const u32 rcv_size = sizeof(cmd->res);
>> + const u32 snd_size = sizeof(*cmd);
>> +
>> + cmd = kmalloc(snd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.size = cpu_to_le32(snd_size - rcv_size);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_BRD_CMD_POWER);
>> + cmd->mode = mode;
>> +
>> + err = ixxat_usb_send_cmd(dev, le16_to_cpu(cmd->req.port), cmd, snd_size,
>> + &cmd->res, rcv_size);
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_reset_ctrl(struct ixxat_usb_device *dev)
>> +{
>> + const u16 port = dev->ctrl_index;
>> + int err;
>> + struct ixxat_usb_dal_cmd *cmd;
>> + const u32 snd_size = sizeof(*cmd);
>> + const u32 rcv_size = sizeof(cmd->res);
>> +
>> + cmd = kmalloc(snd_size, GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + ixxat_usb_setup_cmd(&cmd->req, &cmd->res);
>> + cmd->req.code = cpu_to_le32(IXXAT_USB_CAN_CMD_RESET);
>> + cmd->req.port = cpu_to_le16(port);
>> +
>> + err = ixxat_usb_send_cmd(dev->udev, port, cmd, snd_size, &cmd->res,
>> + rcv_size);
>> + kfree(cmd);
>> + return err;
>> +}
>> +
>> +static void ixxat_usb_stop_queue(struct ixxat_usb_device *dev)
> The name is misleading. This does way more than just stopping the
> queue. You should use some keywords such as kill or free in this
> function name.
>
> Also, considering you do not have a ixxat_usb_start_queue(), it may be
> better to move netif_stop_queue(netdev) out of this function and call
> it separately.
>
>> +{
>> + struct net_device *netdev = dev->netdev;
>> + u32 i;
>> +
>> + netif_stop_queue(netdev);
>> + usb_kill_anchored_urbs(&dev->rx_submitted);
>> + usb_kill_anchored_urbs(&dev->tx_submitted);
>> + atomic_set(&dev->active_tx_urbs, 0);
>> + for (i = 0; i < IXXAT_USB_MAX_TX_URBS; i++) {
>> + if (dev->tx_contexts[i].echo_index != IXXAT_USB_MAX_TX_URBS) {
>> + can_free_echo_skb(netdev, i, NULL);
>> + dev->tx_contexts[i].echo_index = IXXAT_USB_MAX_TX_URBS;
>> + }
>> + }
>> +}
>> +
>> +static int ixxat_usb_restart(struct ixxat_usb_device *dev)
>> +{
>> + int err;
>> + struct net_device *netdev = dev->netdev;
>> + u32 t;
>> +
>> + ixxat_usb_stop_queue(dev);
>> + err = ixxat_usb_stop_ctrl(dev);
> ixxat_usb_stop_ctrl() kills all the anchored urbs...
>
>> + if (err)
>> + goto fail;
>> +
>> + err = ixxat_usb_start_ctrl(dev, &t);
> ... however, ixxat_usb_start_ctrl() does not seem to reallocate those
> urbs. Am I missing something? Was this restart function actually
> tested?
>
> Also, for a restart, you do not need to release and reallocate your
> ressources. Just sending the stop and start commands to the device
> should be sufficient.
>
>> + if (err)
>> + goto fail;
>> +
>> + dev->can.state = CAN_STATE_ERROR_ACTIVE;
>> + netif_wake_queue(netdev);
>> +
>> +fail:
>> + return err;
> If the fail label does only a return, no need for it. Just return the
> error instead of doing a goto jump.
>
>> +}
>> +
>> +static int ixxat_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>> +{
>> + struct ixxat_usb_device *dev = netdev_priv(netdev);
>> +
>> + if (mode != CAN_MODE_START)
>> + return -EOPNOTSUPP;
>> +
>> + return ixxat_usb_restart(dev);
>> +}
>> +
>> +static int ixxat_usb_get_berr_counter(const struct net_device *netdev,
>> + struct can_berr_counter *bec)
>> +{
>> + struct ixxat_usb_device *dev = netdev_priv(netdev);
>> +
>> + *bec = dev->bec;
>> + return 0;
>> +}
>> +
>> +static int ixxat_usb_handle_canmsg(struct ixxat_usb_device *dev,
>> + struct ixxat_can_msg *rx)
>> +{
>> + const u32 ixx_flags = le32_to_cpu(rx->base.flags);
>> + const u8 dlc = FIELD_GET(IXXAT_USB_MSG_FLAGS_DLC, ixx_flags);
>> + struct canfd_frame *cf;
>> + struct net_device *netdev = dev->netdev;
>> + struct sk_buff *skb;
>> + u8 flags = 0;
>> + u8 len;
>> + u8 min_size;
>> +
>> + if (ixx_flags & IXXAT_USB_FDMSG_FLAGS_EDL) {
>> + if (ixx_flags & IXXAT_USB_FDMSG_FLAGS_FDR)
>> + flags |= CANFD_BRS;
>> +
>> + if (ixx_flags & IXXAT_USB_FDMSG_FLAGS_ESI)
>> + flags |= CANFD_ESI;
>> +
>> + len = can_fd_dlc2len(dlc);
>> + } else {
>> + len = can_cc_dlc2len(dlc);
>> + }
>> +
>> + min_size = sizeof(rx->base) + len;
>> +
>> + if (dev->adapter == &usb2can_cl1)
>> + min_size += sizeof(rx->cl1) - sizeof(rx->cl1.data);
>> + else
>> + min_size += sizeof(rx->cl2) - sizeof(rx->cl2.data);
>> +
>> + if (rx->base.size < (min_size - 1)) {
>> + netdev_err(netdev, "Invalid can data message size\n");
>> + return -EBADMSG;
>> + }
>> +
>> + if (ixx_flags & IXXAT_USB_MSG_FLAGS_OVR) {
>> + netdev->stats.rx_over_errors++;
>> + netdev->stats.rx_errors++;
>> + netdev_err(netdev, "Message overflow\n");
>> + }
>> +
>> + if (ixx_flags & IXXAT_USB_FDMSG_FLAGS_EDL)
>> + skb = alloc_canfd_skb(netdev, &cf);
>> + else
>> + skb = alloc_can_skb(netdev, (struct can_frame **)&cf);
>> +
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + cf->can_id = le32_to_cpu(rx->base.msg_id);
>> + cf->len = len;
>> + cf->flags |= flags;
>> +
>> + if (ixx_flags & IXXAT_USB_MSG_FLAGS_EXT)
>> + cf->can_id |= CAN_EFF_FLAG;
>> +
>> + if (ixx_flags & IXXAT_USB_MSG_FLAGS_RTR) {
>> + cf->can_id |= CAN_RTR_FLAG;
>> + } else {
>> + if (dev->adapter == &usb2can_cl1)
>> + memcpy(cf->data, rx->cl1.data, len);
>> + else
>> + memcpy(cf->data, rx->cl2.data, len);
>> +
>> + netdev->stats.rx_bytes += cf->len;
>> + }
>> +
>> + ixxat_usb_get_ts_tv(dev, le32_to_cpu(rx->base.time), &skb->tstamp);
>> +
>> + netdev->stats.rx_packets++;
>> +
>> + netif_rx(skb);
>> +
>> + return 0;
>> +}
>> +
>> +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;
>> + enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
>> + u32 raw_status;
>> + u8 min_size = sizeof(rx->base) + sizeof(raw_status);
>> +
>> + if (dev->adapter == &usb2can_cl1)
>> + min_size += sizeof(rx->cl1) - sizeof(rx->cl1.data);
>> + else
>> + min_size += sizeof(rx->cl2) - sizeof(rx->cl2.data);
>> +
>> + if (rx->base.size < (min_size - 1)) {
>> + netdev_err(netdev, "Invalid can status message size\n");
>> + return -EBADMSG;
>> + }
>> +
>> + if (dev->adapter == &usb2can_cl1)
>> + raw_status = le32_to_cpu(rx->cl1.status);
>> + else
>> + raw_status = le32_to_cpu(rx->cl2.status);
>> +
>> + if (raw_status != IXXAT_USB_CAN_STATUS_OK) {
>> + if (raw_status & IXXAT_USB_CAN_STATUS_BUSOFF) {
>> + dev->can.can_stats.bus_off++;
>> + new_state = CAN_STATE_BUS_OFF;
>> + can_bus_off(netdev);
>> + } else {
>> + if (raw_status & IXXAT_USB_CAN_STATUS_ERRLIM) {
>> + dev->can.can_stats.error_warning++;
>> + new_state = CAN_STATE_ERROR_WARNING;
>> + }
>> +
>> + if (raw_status & IXXAT_USB_CAN_STATUS_ERR_PAS) {
>> + dev->can.can_stats.error_passive++;
>> + new_state = CAN_STATE_ERROR_PASSIVE;
>> + }
>> +
>> + if (raw_status & IXXAT_USB_CAN_STATUS_OVERRUN)
>> + new_state = CAN_STATE_MAX;
>> + }
>> + }
>> +
>> + if (new_state == CAN_STATE_ERROR_ACTIVE) {
>> + dev->bec.txerr = 0;
>> + dev->bec.rxerr = 0;
>> + }
>> +
>> + if (new_state != CAN_STATE_MAX)
>> + dev->can.state = new_state;
>> +
>> + skb = alloc_can_err_skb(netdev, &can_frame);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + switch (new_state) {
>> + case CAN_STATE_ERROR_ACTIVE:
>> + can_frame->can_id |= CAN_ERR_CRTL;
>> + can_frame->data[1] |= CAN_ERR_CRTL_ACTIVE;
>> + break;
>> + case CAN_STATE_ERROR_WARNING:
>> + can_frame->can_id |= CAN_ERR_CRTL;
>> + can_frame->data[1] |= CAN_ERR_CRTL_TX_WARNING;
>> + can_frame->data[1] |= CAN_ERR_CRTL_RX_WARNING;
>> + break;
>> + case CAN_STATE_ERROR_PASSIVE:
>> + can_frame->can_id |= CAN_ERR_CRTL;
>> + can_frame->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
>> + can_frame->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
>> + break;
>> + case CAN_STATE_BUS_OFF:
>> + can_frame->can_id |= CAN_ERR_BUSOFF;
>> + break;
>> + case CAN_STATE_MAX:
>> + can_frame->can_id |= CAN_ERR_CRTL;
>> + can_frame->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
>> + break;
>> + default:
>> + netdev_err(netdev, "Unhandled can status %d\n",
>> + new_state);
>> + break;
>> + }
>> +
>> + netif_rx(skb);
>> +
>> + return 0;
>> +}
>> +
>> +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;
>> + u8 raw_error;
>> + u8 min_size = sizeof(rx->base) + IXXAT_USB_CAN_ERROR_LEN;
>> +
>> + if (dev->adapter == &usb2can_cl1)
>> + min_size += sizeof(rx->cl1) - sizeof(rx->cl1.data);
>> + else
>> + min_size += sizeof(rx->cl2) - sizeof(rx->cl2.data);
>> +
>> + if (rx->base.size < (min_size - 1)) {
>> + netdev_err(netdev, "Invalid can error message size\n");
>> + return -EBADMSG;
>> + }
>> +
>> + if (dev->can.state == CAN_STATE_BUS_OFF)
>> + return 0;
>> +
>> + if (dev->adapter == &usb2can_cl1) {
>> + raw_error = rx->cl1.data[IXXAT_USB_CAN_ERROR_CODE];
>> + dev->bec.rxerr = rx->cl1.data[IXXAT_USB_CAN_ERROR_COUNTER_RX];
>> + dev->bec.txerr = rx->cl1.data[IXXAT_USB_CAN_ERROR_COUNTER_TX];
>> + } else {
>> + raw_error = rx->cl2.data[IXXAT_USB_CAN_ERROR_CODE];
>> + dev->bec.rxerr = rx->cl2.data[IXXAT_USB_CAN_ERROR_COUNTER_RX];
>> + dev->bec.txerr = rx->cl2.data[IXXAT_USB_CAN_ERROR_COUNTER_TX];
>> + }
>> +
>> + if (raw_error == IXXAT_USB_CAN_ERROR_ACK)
>> + netdev->stats.tx_errors++;
>> + else
>> + netdev->stats.rx_errors++;
>> +
>> + skb = alloc_can_err_skb(netdev, &can_frame);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + switch (raw_error) {
>> + case IXXAT_USB_CAN_ERROR_ACK:
>> + can_frame->can_id |= CAN_ERR_ACK;
>> + break;
>> + case IXXAT_USB_CAN_ERROR_BIT:
>> + can_frame->can_id |= CAN_ERR_PROT;
>> + can_frame->data[2] |= CAN_ERR_PROT_BIT;
>> + break;
>> + case IXXAT_USB_CAN_ERROR_CRC:
>> + can_frame->can_id |= CAN_ERR_PROT;
>> + can_frame->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
>> + break;
>> + case IXXAT_USB_CAN_ERROR_FORM:
>> + can_frame->can_id |= CAN_ERR_PROT;
>> + can_frame->data[2] |= CAN_ERR_PROT_FORM;
>> + break;
>> + case IXXAT_USB_CAN_ERROR_STUFF:
>> + can_frame->can_id |= CAN_ERR_PROT;
>> + can_frame->data[2] |= CAN_ERR_PROT_STUFF;
>> + break;
>> + default:
>> + can_frame->can_id |= CAN_ERR_PROT;
>> + can_frame->data[2] |= CAN_ERR_PROT_UNSPEC;
>> + break;
>> + }
>> +
>> + netif_rx(skb);
>> +
>> + return 0;
>> +}
>> +
>> +static void ixxat_usb_decode_buf(struct urb *urb)
>> +{
>> + struct ixxat_usb_device *dev = urb->context;
>> + struct net_device *netdev = dev->netdev;
>> + struct ixxat_can_msg *can_msg;
>> + int err = 0;
>> + u32 pos = 0;
>> + u8 *data = urb->transfer_buffer;
> Do not use an opaque pointer. Directly assign it to can_msg:
>
> struct ixxat_can_msg *can_msg = urb->transfer_buffer;
>
>> +
>> + while (pos < urb->actual_length) {
>> + u32 time;
>> + u8 size;
>> + u8 type;
>> +
>> + can_msg = (struct ixxat_can_msg *)&data[pos];
>> + if (!can_msg || !can_msg->base.size) {
> ^^^^^^^^
>
> I do not think that can_msg can be NULL here.
>
> I see some redundancy here. If can_msg->base.size is zero, then then
> next check (size < sizeof(can_msg->base) would also fail, right? I see
> no need to make this a special case. Try to do the size sanitation in
> a single if.
>
>> + err = -ENOTSUPP;
> As discussed previously, -EBADMSG or -EMSGSIZE (as you prefer).
>
>> + netdev_err(netdev, "Unsupported usb msg: %pe\n",
>> + ERR_PTR(err));
>> + break;
> This will print twice in the log (here and after the fail tag). Only
> one log message should be enough.
>
> Maybe you can remove the error message and instead goto fail? Thanks
> to the %pe, you would get:
>
> Buffer decoding failed: -EBADMSG
>
> in the log which I think is sufficient.
>
>> + }
>> +
>> + size = can_msg->base.size + 1;
>> + if (size < sizeof(can_msg->base) ||
>> + (pos + size) > urb->actual_length) {
>> + err = -EBADMSG;
>> + netdev_err(netdev,
>> + "Invalid usb message size: %pe\n",
>> + ERR_PTR(err));
>> + break;
> Same as above: try to print only one log message.
>
>> + }
>> +
>> + type = le32_to_cpu(can_msg->base.flags);
>> + type &= IXXAT_USB_MSG_FLAGS_TYPE;
>> + time = le32_to_cpu(can_msg->base.time);
>> +
>> + switch (type) {
>> + case IXXAT_USB_CAN_DATA:
>> + err = ixxat_usb_handle_canmsg(dev, can_msg);
>> + if (err)
>> + goto fail;
>> + break;
>> + case IXXAT_USB_CAN_STATUS:
>> + err = ixxat_usb_handle_status(dev, can_msg);
>> + if (err)
>> + goto fail;
>> + break;
>> + case IXXAT_USB_CAN_ERROR:
>> + err = ixxat_usb_handle_error(dev, can_msg);
>> + if (err)
>> + goto fail;
>> + break;
>> + case IXXAT_USB_CAN_TIMEOVR:
>> + ixxat_usb_get_ts_tv(dev, time, NULL);
>> + break;
>> + case IXXAT_USB_CAN_INFO:
>> + case IXXAT_USB_CAN_WAKEUP:
>> + case IXXAT_USB_CAN_TIMERST:
>> + break;
>> + default:
>> + netdev_err(netdev,
>> + "Unhandled rec type 0x%02x : ignored\n",
>> + type);
>> + break;
>> + }
>> +
>> + pos += size;
>> + }
>> +
>> +fail:
>> + if (err)
>> + netdev_err(netdev, "Buffer decoding failed: %pe\n", ERR_PTR(err));
>> +}
>> +
>> +static int ixxat_usb_encode_msg(struct ixxat_usb_device *dev,
>> + struct sk_buff *skb, u8 *obuf)
>> +{
>> + int 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;
>> + u32 flags = 0;
>> + u32 msg_id = 0;
>> +
>> + if (cf->can_id & CAN_RTR_FLAG)
>> + flags |= IXXAT_USB_MSG_FLAGS_RTR;
>> +
>> + if (cf->can_id & CAN_EFF_FLAG) {
>> + flags |= IXXAT_USB_MSG_FLAGS_EXT;
>> + msg_id = cf->can_id & CAN_EFF_MASK;
>> + } else {
>> + msg_id = cf->can_id & CAN_SFF_MASK;
>> + }
>> +
>> + if (can_is_canfd_skb(skb)) {
>> + flags |= IXXAT_USB_FDMSG_FLAGS_EDL;
>> +
>> + if (!(cf->can_id & CAN_RTR_FLAG) && (cf->flags & CANFD_BRS))
>> + flags |= IXXAT_USB_FDMSG_FLAGS_FDR;
>> +
>> + flags |= FIELD_PREP(IXXAT_USB_MSG_FLAGS_DLC, can_fd_len2dlc(cf->len));
>> + } else {
>> + flags |= FIELD_PREP(IXXAT_USB_MSG_FLAGS_DLC, cf->len);
>> + }
>> +
>> + msg_base->flags = cpu_to_le32(flags);
>> + msg_base->msg_id = cpu_to_le32(msg_id);
>> + msg_base->size = sizeof(*msg_base) + cf->len - 1;
>> + if (dev->adapter == &usb2can_cl1) {
>> + msg_base->size += sizeof(can_msg.cl1);
>> + msg_base->size -= sizeof(can_msg.cl1.data);
>> + memcpy(can_msg.cl1.data, cf->data, cf->len);
>> + } else {
>> + msg_base->size += sizeof(can_msg.cl2);
>> + msg_base->size -= sizeof(can_msg.cl2.data);
>> + memcpy(can_msg.cl2.data, cf->data, cf->len);
>> + }
>> +
>> + size = msg_base->size + 1;
>> + memcpy(obuf, &can_msg, size);
>> + return size;
>> +}
>> +
>> +static void ixxat_usb_read_bulk_callback(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)
>> + ixxat_usb_decode_buf(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_callback, dev);
>> +
>> + usb_anchor_urb(urb, &dev->rx_submitted);
>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>> + if (!err)
>> + return;
>> +
>> + usb_unanchor_urb(urb);
>> +
>> + if (err == -ENODEV)
>> + netif_device_detach(netdev);
>> + else
>> + netdev_err(netdev,
>> + "Failed to resubmit read bulk urb: %pe\n", ERR_PTR(err));
>> +}
>> +
>> +static void ixxat_usb_write_bulk_callback(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;
>> +
>> + if (!netif_device_present(netdev))
>> + return;
>> +
>> + if (!urb->status) {
>> + netdev->stats.tx_packets++;
>> + netdev->stats.tx_bytes += can_get_echo_skb(netdev, context->echo_index, NULL);
>> + } else {
>> + netdev_err(netdev, "Tx urb aborted: %pe\n", ERR_PTR(urb->status));
>> + can_free_echo_skb(netdev, context->echo_index, NULL);
>> + }
>> +
>> + context->echo_index = IXXAT_USB_MAX_TX_URBS;
>> + atomic_dec(&dev->active_tx_urbs);
>> +
>> + if (!urb->status)
>> + netif_wake_queue(netdev);
>> +}
>> +
>> +static netdev_tx_t ixxat_usb_start_xmit(struct sk_buff *skb,
>> + struct net_device *netdev)
>> +{
>> + int err;
>> + int size;
>> + struct ixxat_usb_device *dev = netdev_priv(netdev);
>> + struct ixxat_tx_urb_context *context = NULL;
>> + struct net_device_stats *stats = &netdev->stats;
>> + struct urb *urb;
>> + u8 *obuf;
>> + u32 i;
>> +
>> + 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;
>> +
>> + size = ixxat_usb_encode_msg(dev, skb, obuf);
>> +
>> + context->echo_index = i;
>> +
>> + urb->transfer_buffer_length = size;
>> + usb_anchor_urb(urb, &dev->tx_submitted);
>> + can_put_echo_skb(skb, netdev, i, 0);
>> + atomic_inc(&dev->active_tx_urbs);
>> +
>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>> + if (err) {
>> + can_free_echo_skb(netdev, i, NULL);
>> + 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,
>> + "Submitting tx-urb failed: %pe\n", ERR_PTR(err));
>> + }
>> + } else {
>> + if (atomic_read(&dev->active_tx_urbs) >= IXXAT_USB_MAX_TX_URBS)
>> + netif_stop_queue(netdev);
>> + }
>> +
>> + return NETDEV_TX_OK;
>> +}
>> +
>> +static int ixxat_usb_setup_rx_urbs(struct ixxat_usb_device *dev)
>> +{
>> + int i;
>> + int err = 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_RX_URBS; i++) {
>> + struct urb *urb;
>> + u8 *buf;
>> +
>> + urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!urb) {
>> + err = -ENOMEM;
>> + netdev_err(netdev, "No memory for URBs: %pe\n",
>> + ERR_PTR(err));
>> + break;
>> + }
>> +
>> + buf = kmalloc(adapter->buffer_size_rx, GFP_KERNEL);
>> + if (!buf) {
>> + usb_free_urb(urb);
>> + err = -ENOMEM;
>> + netdev_err(netdev,
>> + "No memory for USB-buffer: %pe\n", ERR_PTR(err));
>> + break;
>> + }
>> +
>> + usb_fill_bulk_urb(urb, udev,
>> + usb_rcvbulkpipe(udev, dev->ep_msg_in), buf,
>> + adapter->buffer_size_rx,
>> + ixxat_usb_read_bulk_callback, dev);
>> +
>> + urb->transfer_flags |= URB_FREE_BUFFER;
>> + usb_anchor_urb(urb, &dev->rx_submitted);
>> +
>> + err = usb_submit_urb(urb, GFP_KERNEL);
>> + if (err) {
>> + usb_unanchor_urb(urb);
>> + kfree(buf);
>> + usb_free_urb(urb);
>> +
>> + if (err == -ENODEV)
>> + netif_device_detach(netdev);
>> +
>> + break;
>> + }
>> +
>> + usb_free_urb(urb);
>> + }
>> +
>> + if (i == 0)
>> + netdev_err(netdev, "Couldn't setup any rx-URBs\n");
>> +
>> + return err;
>> +}
>> +
>> +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;
>> + u8 *buf;
>> +
>> + urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!urb) {
>> + ret = -ENOMEM;
>> + netdev_err(netdev, "No memory for URBs: %pe\n",
>> + ERR_PTR(ret));
>> + break;
>> + }
>> +
>> + buf = kmalloc(adapter->buffer_size_tx, GFP_KERNEL);
>> + if (!buf) {
>> + usb_free_urb(urb);
>> + ret = -ENOMEM;
>> + netdev_err(netdev,
>> + "No memory for USB-buffer: %pe\n", ERR_PTR(ret));
>> + break;
>> + }
>> +
>> + context = dev->tx_contexts + i;
>> + context->dev = dev;
>> + context->urb = urb;
>> +
>> + usb_fill_bulk_urb(urb, udev,
>> + usb_sndbulkpipe(udev, dev->ep_msg_out), buf,
>> + adapter->buffer_size_tx,
>> + ixxat_usb_write_bulk_callback, context);
>> +
>> + urb->transfer_flags |= URB_FREE_BUFFER;
>> + }
>> +
>> + if (i == 0) {
>> + netdev_err(netdev, "Couldn't setup any tx-URBs\n");
>> + usb_kill_anchored_urbs(&dev->rx_submitted);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void ixxat_usb_disconnect(struct usb_interface *intf)
>> +{
>> + struct ixxat_usb_device *dev;
>> + struct ixxat_usb_device *prev_dev;
>> +
>> + /* unregister the given device and all previous devices */
>> + for (dev = usb_get_intfdata(intf); dev; dev = prev_dev) {
>> + prev_dev = dev->prev_dev;
>> + unregister_netdev(dev->netdev);
>> + free_candev(dev->netdev);
>> + }
>> +
>> + usb_set_intfdata(intf, NULL);
>> +}
>> +
>> +static int ixxat_usb_start(struct ixxat_usb_device *dev)
>> +{
>> + int err;
>> + int i;
>> + u32 time_ref = 0;
>> + const struct ixxat_usb_adapter *adapter = dev->adapter;
>> +
>> + err = ixxat_usb_setup_rx_urbs(dev);
>> + if (err)
>> + return err;
>> +
>> + err = ixxat_usb_setup_tx_urbs(dev);
>> + if (err)
>> + return err;
>> +
>> + /* Try to reset the controller, in case it is already initialized
>> + * from a previous unclean shutdown
>> + */
>> + ixxat_usb_reset_ctrl(dev);
>> +
>> + if (adapter->init_ctrl) {
>> + err = adapter->init_ctrl(dev);
>> + if (err)
>> + goto fail;
>> + }
>> +
>> + if (!(dev->state & IXXAT_USB_STATE_STARTED)) {
>> + err = ixxat_usb_start_ctrl(dev, &time_ref);
>> + if (err)
>> + goto fail;
>> +
>> + ixxat_usb_set_ts_now(dev, time_ref);
>> + }
>> +
>> + dev->bec.txerr = 0;
>> + dev->bec.rxerr = 0;
>> +
>> + dev->state |= IXXAT_USB_STATE_STARTED;
>> + dev->can.state = CAN_STATE_ERROR_ACTIVE;
>> + return 0;
>> +
>> +fail:
>> + if (err == -ENODEV)
>> + netif_device_detach(dev->netdev);
>> +
>> + netdev_err(dev->netdev, "Couldn't submit control: %pe\n", ERR_PTR(err));
>> +
>> + for (i = 0; i < IXXAT_USB_MAX_TX_URBS; i++) {
>> + usb_free_urb(dev->tx_contexts[i].urb);
>> + dev->tx_contexts[i].urb = NULL;
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_open(struct net_device *netdev)
>> +{
>> + struct ixxat_usb_device *dev = netdev_priv(netdev);
>> + int err;
>> +
>> + /* common open */
>> + err = open_candev(netdev);
>> + if (err)
>> + goto fail;
>> +
>> + /* finally start device */
>> + err = ixxat_usb_start(dev);
>> + if (err) {
>> + netdev_err(netdev, "Couldn't start device: %pe\n", ERR_PTR(err));
>> + close_candev(netdev);
>> + goto fail;
>> + }
>> +
>> + netif_start_queue(netdev);
>> +
>> +fail:
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_stop(struct net_device *netdev)
>> +{
>> + int err = 0;
>> + struct ixxat_usb_device *dev = netdev_priv(netdev);
>> +
>> + ixxat_usb_stop_queue(dev);
>> + if (dev->state & IXXAT_USB_STATE_STARTED) {
>> + err = ixxat_usb_stop_ctrl(dev);
>> + if (err)
>> + netdev_warn(netdev, "Error %d: Cannot stop device\n",
>> + err);
>> + }
>> +
>> + dev->state &= ~IXXAT_USB_STATE_STARTED;
>> + close_candev(netdev);
>> + dev->can.state = CAN_STATE_STOPPED;
>> + return err;
>> +}
>> +
>> +static const struct net_device_ops ixxat_usb_netdev_ops = {
>> + .ndo_open = ixxat_usb_open,
>> + .ndo_stop = ixxat_usb_stop,
>> + .ndo_start_xmit = ixxat_usb_start_xmit
>> +};
>> +
>> +static const struct ethtool_ops ixxat_usb_ethtool_ops = {
>> + .get_ts_info = ethtool_op_get_ts_info,
>> +};
>> +
>> +static int ixxat_usb_create_dev(struct usb_interface *intf,
>> + const struct ixxat_usb_adapter *adapter,
>> + u16 ctrl_index)
>> +{
>> + struct usb_device *udev = interface_to_usbdev(intf);
>> + struct ixxat_usb_device *dev;
>> + struct net_device *netdev;
>> + int err;
>> + int i;
>> +
>> + netdev = alloc_candev(sizeof(*dev), IXXAT_USB_MAX_TX_URBS);
>> + if (!netdev) {
>> + dev_err(&intf->dev, "Cannot allocate candev\n");
>> + return -ENOMEM;
>> + }
>> +
>> + dev = netdev_priv(netdev);
>> + dev->udev = udev;
>> + dev->netdev = netdev;
>> + dev->adapter = adapter;
>> + dev->ctrl_index = ctrl_index;
>> + dev->state = IXXAT_USB_STATE_CONNECTED;
>> +
>> + i = ctrl_index + adapter->ep_offs;
>> + dev->ep_msg_in = adapter->ep_msg_in[i];
>> + dev->ep_msg_out = adapter->ep_msg_out[i];
>> +
>> + dev->can.clock.freq = adapter->clock;
>> + dev->can.bittiming_const = adapter->bt;
>> + dev->can.data_bittiming_const = adapter->btd;
>> +
>> + dev->can.do_set_mode = ixxat_usb_set_mode;
>> + dev->can.do_get_berr_counter = ixxat_usb_get_berr_counter;
>> +
>> + dev->can.ctrlmode_supported = adapter->modes;
>> +
>> + netdev->netdev_ops = &ixxat_usb_netdev_ops;
>> + netdev->ethtool_ops = &ixxat_usb_ethtool_ops;
>> +
>> + netdev->flags |= IFF_ECHO;
>> + netdev->dev_port = ctrl_index;
>> +
>> + init_usb_anchor(&dev->rx_submitted);
>> + init_usb_anchor(&dev->tx_submitted);
>> +
>> + atomic_set(&dev->active_tx_urbs, 0);
>> +
>> + for (i = 0; i < IXXAT_USB_MAX_TX_URBS; i++)
>> + dev->tx_contexts[i].echo_index = IXXAT_USB_MAX_TX_URBS;
>> +
>> + dev->prev_dev = usb_get_intfdata(intf);
>> + usb_set_intfdata(intf, dev);
>> +
>> + SET_NETDEV_DEV(netdev, &intf->dev);
>> + err = register_candev(netdev);
>> + if (err) {
>> + dev_err(&intf->dev, "Failed to register can device: %pe\n",
>> + ERR_PTR(err));
>> + goto free_candev;
>> + }
>> +
>> + if (dev->prev_dev)
>> + (dev->prev_dev)->next_dev = dev;
>> +
>> + err = ixxat_usb_get_dev_info(udev, &dev->dev_info);
>> + if (err) {
>> + dev_err(&intf->dev,
>> + "Failed to get device information: %pe\n", ERR_PTR(err));
>> + goto unreg_candev;
>> + }
>> +
>> + netdev_info(netdev, "%s: Connected Channel %u (device %s)\n",
>> + dev->dev_info.device_name, ctrl_index,
>> + dev->dev_info.device_id);
>> +
>> + return 0;
>> +
>> +unreg_candev:
>> + unregister_candev(netdev);
>> +free_candev:
>> + usb_set_intfdata(intf, dev->prev_dev);
>> + free_candev(netdev);
>> + return err;
>> +}
>> +
>> +static int ixxat_usb_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct usb_device *udev = interface_to_usbdev(intf);
>> + struct usb_host_interface *host_intf = intf->altsetting;
>> + const struct ixxat_usb_adapter *adapter;
>> + struct ixxat_dev_caps dev_caps;
>> + u16 i;
>> + int err;
>> +
>> + usb_reset_configuration(udev);
>> +
>> + adapter = (const struct ixxat_usb_adapter *)id->driver_info;
>> +
>> + for (i = 0; i < host_intf->desc.bNumEndpoints; i++) {
>> + const u8 epaddr = host_intf->endpoint[i].desc.bEndpointAddress;
>> + int match;
>> + u8 j;
>> +
>> + /* Check if usb-endpoint address matches known usb-endpoints */
>> + for (j = 0; j < IXXAT_USB_MAX_CHANNEL; j++) {
>> + u8 ep_msg_in = adapter->ep_msg_in[j];
>> + u8 ep_msg_out = adapter->ep_msg_in[j];
>> +
>> + if (epaddr == ep_msg_in || epaddr == ep_msg_out) {
>> + match = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (!match)
>> + return -ENODEV;
>> + }
>> +
>> + err = ixxat_usb_power_ctrl(udev, IXXAT_USB_POWER_WAKEUP);
>> + if (err)
>> + return err;
>> +
>> + msleep(IXXAT_USB_POWER_WAKEUP_TIME);
>> +
>> + err = ixxat_usb_get_dev_caps(udev, &dev_caps);
>> + if (err) {
>> + dev_err(&intf->dev,
>> + "Failed to get device capabilities: %pe\n", ERR_PTR(err));
>> + return err;
>> + }
>> +
>> + err = -ENODEV;
>> + for (i = 0; i < le16_to_cpu(dev_caps.bus_ctrl_count); i++) {
>> + u16 dev_bustype = le16_to_cpu(dev_caps.bus_ctrl_types[i]);
>> + u8 bustype = IXXAT_USB_BUS_TYPE(dev_bustype);
>> +
>> + if (bustype == IXXAT_USB_BUS_CAN)
>> + err = ixxat_usb_create_dev(intf, adapter, i);
>> +
>> + if (err) {
>> + /* deregister already created devices */
>> + ixxat_usb_disconnect(intf);
>> + return err;
>> + }
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static struct usb_driver ixxat_usb_driver = {
>> + .name = IXXAT_USB_DRIVER_NAME,
>> + .probe = ixxat_usb_probe,
>> + .disconnect = ixxat_usb_disconnect,
>> + .id_table = ixxat_usb_table,
>> +};
>> +
>> +module_usb_driver(ixxat_usb_driver);
>> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h
>> new file mode 100644
>> index 000000000000..56c6f3b938d8
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h
>> @@ -0,0 +1,511 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2018 HMS Industrial Networks <socketcan@...-networks.de>
>> + */
>> +
>> +#ifndef IXXAT_USB_CORE_H
>> +#define IXXAT_USB_CORE_H
>> +
>> +#define IXXAT_USB_DRIVER_NAME "ixxat_usb2can"
> Remove and use KBUILD_MODNAME instead.
>
>> +#define IXXAT_USB_CTRL_NAME "ixxat_usb"
>> +
>> +#define IXXAT_USB_VENDOR_ID 0x08d8
>> +
>> +/* supported device ids: CL1 */
>> +#define USB2CAN_COMPACT_PRODUCT_ID 0x0008
>> +#define USB2CAN_EMBEDDED_PRODUCT_ID 0x0009
>> +#define USB2CAN_PROFESSIONAL_PRODUCT_ID 0x000A
>> +#define USB2CAN_AUTOMOTIVE_PRODUCT_ID 0x000B
>> +
>> +/* supported device ids: CL2 */
>> +#define USB2CAN_FD_COMPACT_PRODUCT_ID 0x0014
>> +#define USB2CAN_FD_PROFESSIONAL_PRODUCT_ID 0x0016
>> +#define USB2CAN_FD_AUTOMOTIVE_PRODUCT_ID 0x0017
>> +#define USB2CAN_FD_PCIE_MINI_PRODUCT_ID 0x001B
>> +#define USB2CAR_PRODUCT_ID 0x001C
>> +#define CAN_IDM101_PRODUCT_ID 0xFF12
>> +#define CAN_IDM200_PRODUCT_ID 0xFF13
>> +
>> +#define IXXAT_USB_BUS_CAN 1
>> +
>> +#define IXXAT_USB_BUS_TYPE(type) ((u8)(((type) >> 8) & 0x00FF))
> Do:
>
> #define IXXAT_USB_BUS_TYPE_MASK 0xff00
>
> and then use FIELD_GET(IXXAT_USB_BUS_TYPE_MASK, type) instead.
>
>> +#define IXXAT_USB_STATE_CONNECTED BIT(0)
>> +#define IXXAT_USB_STATE_STARTED BIT(1)
>> +
>> +#define IXXAT_USB_MAX_CHANNEL 5
>> +#define IXXAT_USB_MAX_TYPES 32
>> +#define IXXAT_USB_MAX_RX_URBS 4
>> +#define IXXAT_USB_MAX_TX_URBS 10
>> +#define IXXAT_USB_MAX_COM_REQ 10
>> +
>> +#define IXXAT_USB_MSG_TIMEOUT 50
>> +#define IXXAT_USB_MSG_CYCLE 20
>> +
>> +#define IXXAT_USB_POWER_WAKEUP 0
>> +#define IXXAT_USB_POWER_WAKEUP_TIME 500
>> +
>> +#define IXXAT_USB_OPMODE_STANDARD BIT(0)
>> +#define IXXAT_USB_OPMODE_EXTENDED BIT(1)
>> +#define IXXAT_USB_OPMODE_ERRFRAME BIT(2)
>> +#define IXXAT_USB_OPMODE_LISTONLY BIT(3)
>> +
>> +#define IXXAT_USB_EXMODE_EXTDATA BIT(0)
>> +#define IXXAT_USB_EXMODE_FASTDATA BIT(1)
>> +#define IXXAT_USB_EXMODE_ISOFD BIT(2)
>> +
>> +#define IXXAT_USB_BTMODE_NAT BIT(0)
>> +#define IXXAT_USB_BTMODE_TSM BIT(1)
>> +
>> +#define IXXAT_USB_STOP_ACTION_CLEARALL 3
>> +
>> +#define IXXAT_RESTART_TASK_CYCLE_TIME 20
>> +
>> +#define IXXAT_USB_CAN_DATA 0x00
>> +#define IXXAT_USB_CAN_INFO 0x01
>> +#define IXXAT_USB_CAN_ERROR 0x02
>> +#define IXXAT_USB_CAN_STATUS 0x03
>> +#define IXXAT_USB_CAN_WAKEUP 0x04
>> +#define IXXAT_USB_CAN_TIMEOVR 0x05
>> +#define IXXAT_USB_CAN_TIMERST 0x06
>> +
>> +#define IXXAT_USB_CAN_STATUS_OK 0x00000000
>> +#define IXXAT_USB_CAN_STATUS_OVERRUN 0x00000002
>> +#define IXXAT_USB_CAN_STATUS_ERRLIM 0x00000004
>> +#define IXXAT_USB_CAN_STATUS_BUSOFF 0x00000008
>> +#define IXXAT_USB_CAN_STATUS_ERR_PAS 0x00002000
>> +
>> +#define IXXAT_USB_CAN_ERROR_LEN 5
>> +
>> +#define IXXAT_USB_CAN_ERROR_CODE 0
>> +#define IXXAT_USB_CAN_ERROR_COUNTER_RX 3
>> +#define IXXAT_USB_CAN_ERROR_COUNTER_TX 4
>> +
>> +#define IXXAT_USB_CAN_ERROR_STUFF 1
>> +#define IXXAT_USB_CAN_ERROR_FORM 2
>> +#define IXXAT_USB_CAN_ERROR_ACK 3
>> +#define IXXAT_USB_CAN_ERROR_BIT 4
>> +#define IXXAT_USB_CAN_ERROR_CRC 6
>> +
>> +#define IXXAT_USB_MSG_FLAGS_TYPE 0x000000FF
>> +#define IXXAT_USB_MSG_FLAGS_DLC 0x000F0000
>> +#define IXXAT_USB_MSG_FLAGS_OVR 0x00100000
>> +#define IXXAT_USB_MSG_FLAGS_RTR 0x00400000
>> +#define IXXAT_USB_MSG_FLAGS_EXT 0x00800000
>> +
>> +#define IXXAT_USB_FDMSG_FLAGS_EDL 0x00000400
>> +#define IXXAT_USB_FDMSG_FLAGS_FDR 0x00000800
>> +#define IXXAT_USB_FDMSG_FLAGS_ESI 0x00001000
>> +
>> +#define IXXAT_USB_CAN_CMD_START 0x326
>> +#define IXXAT_USB_CAN_CMD_STOP 0x327
>> +#define IXXAT_USB_CAN_CMD_RESET 0x328
>> +
>> +#define IXXAT_USB_BRD_CMD_GET_DEVCAPS 0x401
>> +#define IXXAT_USB_BRD_CMD_GET_DEVINFO 0x402
>> +#define IXXAT_USB_BRD_CMD_POWER 0x421
>> +
>> +/**
>> + * struct ixxat_can_msg_base - IXXAT CAN message base (CL1/CL2)
>> + * @size: Message size (this field excluded)
>> + * @time: Message timestamp
>> + * @msg_id: Message ID
>> + * @flags: Message flags
>> + *
>> + * Contains the common fields of an IXXAT CAN message on both CL1 and CL2
>> + * devices
>> + */
>> +struct ixxat_can_msg_base {
>> + u8 size;
>> + __le32 time;
> Is this an hardware timestamp? If so, you can populate
> skb_shared_hwtstamps->hwtstamp (c.f. function skb_hwtstamps()). Then
> ixxat_usb_ethtool_ops needs to be updated accordingly to inform the
> userland that you do support hardware timestamps.
>
>> + __le32 msg_id;
>> + __le32 flags;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_can_msg_cl1 - IXXAT CAN message (CL1)
>> + * @data: Message data (standard CAN frame)
>> + *
>> + * Contains the fields of an IXXAT CAN message on CL1 devices
>> + */
>> +struct ixxat_can_msg_cl1 {
>> + union {
>> + u8 data[CAN_MAX_DLEN];
>> + __le32 status;
>> + } __packed;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_can_msg_cl2 - IXXAT CAN message (CL2)
>> + * @client_id: Client ID
>> + * @data: Message data (CAN FD frame)
>> + *
>> + * Contains the fields of an IXXAT CAN message on CL2 devices
>> + */
>> +struct ixxat_can_msg_cl2 {
>> + __le32 client_id;
>> + union {
>> + u8 data[CANFD_MAX_DLEN];
>> + __le32 status;
>> + } __packed;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_can_msg - IXXAT CAN message
>> + * @base: Base message
>> + * @cl1: Cl1 message
>> + * @cl2: Cl2 message
>> + *
>> + * Contains an IXXAT CAN message
>> + */
>> +struct ixxat_can_msg {
>> + struct ixxat_can_msg_base base;
>> + union {
>> + struct ixxat_can_msg_cl1 cl1;
>> + struct ixxat_can_msg_cl2 cl2;
>> + };
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_dev_caps - Device capabilities
>> + * @bus_ctrl_count: Stores the bus controller counter
>> + * @bus_ctrl_types: Stores the bus controller types
>> + *
>> + * Contains the device capabilities
>> + */
>> +struct ixxat_dev_caps {
>> + __le16 bus_ctrl_count;
>> + __le16 bus_ctrl_types[IXXAT_USB_MAX_TYPES];
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_canbtp Bittiming parameters (CL2)
>> + * @mode: Operation mode
>> + * @bps: Bits per second
>> + * @ts1: First time segment
>> + * @ts2: Second time segment
>> + * @sjw: Synchronization jump width
>> + * @tdo: Transmitter delay offset
>> + *
>> + * Bittiming parameters of a CL2 initialization request
>> + */
>> +struct ixxat_canbtp {
>> + __le32 mode;
>> + __le32 bps;
>> + __le16 ts1;
>> + __le16 ts2;
>> + __le16 sjw;
>> + __le16 tdo;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_dev_info IXXAT usb device information
>> + * @device_name: Name of the device
>> + * @device_id: Device identification ( unique device id)
>> + * @device_version: Device version ( 0, 1, ...)
>> + * @device_fpga_version: Version of FPGA design
>> + *
>> + * Contains device information of IXXAT USB devices
>> + */
>> +struct ixxat_dev_info {
>> + char device_name[16];
>> + char device_id[16];
>> + __le16 device_version;
>> + __le32 device_fpga_version;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_time_ref Time reference
>> + * @kt_host_0: Latest time on the host
>> + * @ts_dev_0: Latest time stamp on the device
>> + * @ts_dev_last: Last device time stamp
>> + *
>> + * Contains time references of the device and the host
>> + */
>> +struct ixxat_time_ref {
>> + ktime_t kt_host_0;
>> + u32 ts_dev_0;
>> + u32 ts_dev_last;
>> +};
>> +
>> +/**
>> + * struct ixxat_tx_urb_context URB content for transmission
>> + * @dev: IXXAT USB device
>> + * @urb: USB request block
>> + * @echo_index: Echo index
>> + * @dlc: Data length code
>> + * @count: Counter
> Those two fields appear in the documentation but are not part of the
> actual struct.
>
>> + *
>> + * Contains content for USB request block transmissions
>> + */
>> +struct ixxat_tx_urb_context {
>> + struct ixxat_usb_device *dev;
>> + struct urb *urb;
>> + u32 echo_index;
>> +};
>> +
>> +/**
>> + * struct ixxat_usb_device IXXAT USB device
>> + * @can: CAN common private data
>> + * @adapter: USB network descriptor
>> + * @udev: USB device
>> + * @netdev: Net_device
>> + * @active_tx_urbs: Active tx urbs
>> + * @tx_submitted: Submitted tx usb anchor
>> + * @tx_contexts: Buffer for tx contexts
>> + * @rx_submitted: Submitted rx usb anchor
>> + * @state: Device state
>> + * @ctrl_index: Controller index
>> + * @ep_msg_in: USB endpoint for incoming messages
>> + * @ep_msg_out: USB endpoint for outgoing messages
>> + * @prev_dev: Previous opened device
>> + * @next_dev: Next opened device in list
>> + * @time_ref: Time reference
>> + * @dev_info: Device information
>> + * @bec: CAN error counter
>> + *
>> + * IXXAT USB-to-CAN device
>> + */
>> +struct ixxat_usb_device {
>> + struct can_priv can;
>> + const struct ixxat_usb_adapter *adapter;
>> + struct usb_device *udev;
>> + struct net_device *netdev;
>> +
>> + atomic_t active_tx_urbs;
>> + struct usb_anchor tx_submitted;
>> + struct ixxat_tx_urb_context tx_contexts[IXXAT_USB_MAX_TX_URBS];
>> + struct usb_anchor rx_submitted;
>> +
>> + u32 state;
>> + u16 ctrl_index;
>> +
>> + u8 ep_msg_in;
>> + u8 ep_msg_out;
>> +
>> + struct ixxat_usb_device *prev_dev;
>> + struct ixxat_usb_device *next_dev;
>> +
>> + struct ixxat_time_ref time_ref;
>> + struct ixxat_dev_info dev_info;
>> +
>> + struct can_berr_counter bec;
>> +};
>> +
>> +/**
>> + * struct ixxat_usb_dal_req IXXAT device request block
>> + * @size: Request size
>> + * @port: Request port
>> + * @socket: Request socket
>> + * @code: Request code
>> + *
>> + * IXXAT device request block
>> + */
>> +struct ixxat_usb_dal_req {
>> + __le32 size;
>> + __le16 port;
>> + __le16 socket;
>> + __le32 code;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_dal_res IXXAT device response block
>> + * @res_size: Expected response size
>> + * @ret_size: Actual response size
>> + * @code: Return code
>> + *
>> + * IXXAT device response block
>> + */
>> +struct ixxat_usb_dal_res {
>> + __le32 res_size;
>> + __le32 ret_size;
>> + __le32 code;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_dal_cmd IXXAT device command
>> + * @req: Request block
>> + * @req: Response block
>> + *
>> + * IXXAT device command
>> + */
>> +struct ixxat_usb_dal_cmd {
>> + struct ixxat_usb_dal_req req;
>> + struct ixxat_usb_dal_res res;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_caps_cmd Device capabilities command
>> + * @req: Request block
>> + * @res: Response block
>> + * @caps: Device capabilities
>> + *
>> + * Can be sent to a device to request its capabilities
>> + */
>> +struct ixxat_usb_caps_cmd {
>> + struct ixxat_usb_dal_req req;
>> + struct ixxat_usb_dal_res res;
>> + struct ixxat_dev_caps caps;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_init_cl1_cmd Initialization command (CL1)
>> + * @req: Request block
>> + * @mode: Operation mode
>> + * @btr0: Bittiming register 0
>> + * @btr1: Bittiming register 1
>> + * @padding: 1 byte padding
>> + * @res: Response block
>> + *
>> + * Can be sent to a CL1 device to initialize it
>> + */
>> +struct ixxat_usb_init_cl1_cmd {
>> + struct ixxat_usb_dal_req req;
>> + u8 mode;
>> + u8 btr0;
>> + u8 btr1;
>> + u8 padding;
>> + struct ixxat_usb_dal_res res;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_init_cl2_cmd Initialization command (CL2)
>> + * @req: Request block
>> + * @opmode: Operation mode
>> + * @exmode: Extended mode
>> + * @sdr: Stadard bittiming parameters
>> + * @fdr: Fast data bittiming parameters
>> + * @_padding: 2 bytes padding
>> + * @res: Response block
>> + *
>> + * Can be sent to a CL2 device to initialize it
>> + */
>> +struct ixxat_usb_init_cl2_cmd {
>> + struct ixxat_usb_dal_req req;
>> + u8 opmode;
>> + u8 exmode;
>> + struct ixxat_canbtp sdr;
>> + struct ixxat_canbtp fdr;
>> + __le16 _padding;
>> + struct ixxat_usb_dal_res res;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_start_cmd Controller start command
>> + * @req: Request block
>> + * @res: Response block
>> + * @time: Timestamp
>> + *
>> + * Can be sent to a device to start its controller
>> + */
>> +struct ixxat_usb_start_cmd {
>> + struct ixxat_usb_dal_req req;
>> + struct ixxat_usb_dal_res res;
>> + __le32 time;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_stop_cmd Controller stop command
>> + * @req: Request block
>> + * @action: Stop action
>> + * @res: Response block
>> + *
>> + * Can be sent to a device to start its controller
>> + */
>> +struct ixxat_usb_stop_cmd {
>> + struct ixxat_usb_dal_req req;
>> + __le32 action;
>> + struct ixxat_usb_dal_res res;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_power_cmd Power command
>> + * @req: Request block
>> + * @mode: Power mode
>> + * @_padding1: 1 byte padding
>> + * @_padding2: 2 byte padding
>> + * @res: Response block
>> + *
>> + * Can be sent to a device to set its power mode
>> + */
>> +struct ixxat_usb_power_cmd {
>> + struct ixxat_usb_dal_req req;
>> + u8 mode;
>> + u8 _padding1;
>> + __le16 _padding2;
> Nitpick: you can also do:
>
> u8 _padding[3];
>
> This comment is an FYI, if you prefer not to fix, also OK.
>
>> + struct ixxat_usb_dal_res res;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_info_cmd Device information command
>> + * @req: Request block
>> + * @res: Response block
>> + * @info: Device information
>> + *
>> + * Can be sent to a device to request its device information
>> + */
>> +struct ixxat_usb_info_cmd {
>> + struct ixxat_usb_dal_req req;
>> + struct ixxat_usb_dal_res res;
>> + struct ixxat_dev_info info;
>> +} __packed;
>> +
>> +/**
>> + * struct ixxat_usb_adapter IXXAT USB device adapter
>> + * @clock: Clock frequency
>> + * @bt: Bittiming constants
>> + * @btd: Data bittiming constants
>> + * @modes: Supported modes
>> + * @buffer_size_rx: Buffer size for receiving
>> + * @buffer_size_tx: Buffer size for transfer
>> + * @ep_msg_in: USB endpoint buffer for incoming messages
>> + * @ep_msg_out: USB endpoint buffer for outgoing messages
>> + * @ep_offs: Endpoint offset (device depended)
>> + *
>> + * Device Adapter for IXXAT USB devices
>> + */
>> +struct ixxat_usb_adapter {
>> + const u32 clock;
>> + const struct can_bittiming_const *bt;
>> + const struct can_bittiming_const *btd;
>> + const u32 modes;
>> + const u16 buffer_size_rx;
>> + const u16 buffer_size_tx;
>> + const u8 ep_msg_in[IXXAT_USB_MAX_CHANNEL];
>> + const u8 ep_msg_out[IXXAT_USB_MAX_CHANNEL];
> Do you really need those ep_msg_in and ep_msg_out arrays? The list of
> end points is advertized by the device. You also have the
> usb_find_common_endpoints() to help you to find which end point to
> use.
>
> At the end, you only use one pair of endpoints:
> ixxat_usb_device->ep_msg_in and ixxat_usb_device->ep_msg_out. Why
> storing the other endpoints if used?
>
>> + const u8 ep_offs;
>> + int (*init_ctrl)(struct ixxat_usb_device *dev);
>> +};
>> +
>> +extern const struct ixxat_usb_adapter usb2can_cl1;
>> +extern const struct ixxat_usb_adapter usb2can_cl2;
>> +extern const struct ixxat_usb_adapter can_idm;
>> +
>> +/**
>> + * ixxat_usb_setup_cmd() - Setup a device command
>> + * @req: Request block
>> + * @res: Response block
>> + *
>> + * This function sets the default values in the request and the response block
>> + * of a device command
>> + */
>> +void ixxat_usb_setup_cmd(struct ixxat_usb_dal_req *req,
>> + struct ixxat_usb_dal_res *res);
>> +
>> +/**
>> + * ixxat_usb_send_cmd() - Send a command to the device
>> + * @dev: USB device
>> + * @port: Command port
>> + * @req: Command request buffer
>> + * @req_size: Command request size
>> + * @res: Command response buffer
>> + * @res_size: Command response size
>> + *
>> + * This function sends a specific command to the device
>> + *
>> + * Return: Negative error code or zero on success
>> + */
>> +int ixxat_usb_send_cmd(struct usb_device *dev, const u16 port, void *req,
>> + const u16 req_size, void *res, const u16 res_size);
>> +
>> +#endif /* IXXAT_USB_CORE_H */
>> --
>> 2.40.1
>>
>
Powered by blists - more mailing lists