| 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: Thu, 30 Mar 2023 13:40:08 +0800
From: Peter Hong <peter_hong@...tek.com.tw>
To: Marc Kleine-Budde <mkl@...gutronix.de>
CC: <wg@...ndegger.com>, <michal.swiatkowski@...ux.intel.com>,
<Steen.Hegelund@...rochip.com>, <mailhol.vincent@...adoo.fr>,
<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <frank.jungclaus@....eu>,
<linux-kernel@...r.kernel.org>, <linux-can@...r.kernel.org>,
<netdev@...r.kernel.org>, <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support
Hi Marc,
Marc Kleine-Budde 於 2023/3/27 下午 11:09 寫道:
> On 27.03.2023 13:10:48, Ji-Ze Hong (Peter Hong) wrote:
>> This patch add support for Fintek USB to 2CAN controller support.
> In addition to Vincent's comment, please describe your change
> declarative, e.g.:
>
> | Add support for the Fintek USB to 2CAN controller.
>
> Please add an entry in the MAINTAINERS file.
>
We'll add description to MAINTAINERS file as following:
FINTEK F81604 USB to 2CAN DEVICE DRIVERS
M: Ji-Ze Hong (Peter Hong) <peter_hong@...tek.com.tw>
L: linux-can@...r.kernel.org
S: Maintained
Is this OK?
>> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
> allocate the URB dynamic with kmalloc() and use urb->transfer_flags |=
> URB_FREE_BUFFER for automatic free()ing.
Could I use kmalloc on device probe() and kfree on disconnect() ?
It may reduce a lot malloc/free times.
>> +
>> + struct urb *write_urb;
>> + u8 bulk_write_buffer[F81604_DATA_SIZE];
> With just 1 TX URB the TX would be quite slow. Does your hardware
> support more than 1 TX URB at a time? USB devices usually answer with
> NAK it they are busy, automatic and transparent retry is handled by the
> USB host controller.
This device only NAK when TX EP buf full. It maybe malfunction when sending
TX URB while CAN transmitting.
>> +};
>> +
>> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
>> +struct f81604_int_data {
>> + u8 sr;
>> + u8 isrc;
>> + u8 ier;
>> + u8 alc;
>> + u8 ecc;
>> + u8 ewlr;
>> + u8 rxerr;
>> + u8 txerr;
>> + u8 val;
>> +} __packed;
> I think this struct is always aligned to 4 bytes, so add a __aligned(4)
> behind __packed.
>> +
>> +struct f81604_sff {
>> + __be16 id;
>> + u8 data[CAN_MAX_DLEN];
>> +} __packed;
> __aligned(2)
>
>> +
>> +struct f81604_eff {
>> + __be32 id;
>> + u8 data[CAN_MAX_DLEN];
>> +} __packed;
> __aligned(2)
>
>> +
>> +struct f81604_bulk_data {
>> + u8 cmd;
>> +
>> + /* According for F81604 DLC define:
>> + * #define F81604_DLC_LEN_MASK 0x0f
>> + * #define F81604_DLC_EFF_BIT BIT(7)
>> + * #define F81604_DLC_RTR_BIT BIT(6)
> no need to duplicate code
>
>> + */
>> + u8 dlc;
>> +
>> + union {
>> + struct f81604_sff sff;
>> + struct f81604_eff eff;
>> + };
>> +} __packed;
> __aligned(4)
I had add following aligned is OK.
struct f81604_int_data; __aligned(4)
struct f81604_sff; __aligned(2)
struct f81604_eff; __aligned(2)
But when I add __aligned(4) to struct f81604_bulk_data, It'll generate
error about size != 14.
./include/linux/build_bug.h:78:41: error: static assertion failed:
"sizeof(struct f81604_bulk_data) == F81604_DATA_SIZE"
So I'll add align to these structs without f81604_bulk_data. Is it OK?
>> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
>> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
>> + priv->bulk_read_buffer[i], F81604_BULK_SIZE,
>> + f81604_read_bulk_callback, netdev);
>> + }
>> +
>> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!priv->write_urb)
>> + goto error;
>> +
>> + usb_fill_bulk_urb(priv->write_urb, priv->dev,
>> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
>> + priv->bulk_write_buffer, F81604_DATA_SIZE,
>> + f81604_write_bulk_callback, netdev);
>> +
>> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!priv->int_urb)
>> + goto error;
>> +
>> + usb_fill_int_urb(priv->int_urb, priv->dev,
>> + usb_rcvintpipe(priv->dev, int_in_addr[id]),
>> + priv->int_read_buffer, F81604_INT_SIZE,
>> + f81604_read_int_callback, netdev, 1);
> Does the endpoint descriptor specify a proper interval?
The interval of interrupt endpoint is 1 (1ms).
Bus 001 Device 010: ID 2c42:1709 FINTEK USB TO CANBUS BRIDGE
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 1.10
bDeviceClass 0
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 64
idVendor 0x2c42
idProduct 0x1709
bcdDevice 0.01
iManufacturer 1 FINTEK
iProduct 2 USB TO CANBUS BRIDGE
iSerial 3 88635600168801
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x003c
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0xa0
(Bus Powered)
Remote Wakeup
MaxPower 100mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 6
bInterfaceClass 0
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03 EP 3 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Device Status: 0xa780
(Bus Powered)
Thanks
Powered by blists - more mailing lists