Le jeu. 23 mars 2023 à 14:14, Peter Hong <peter_hong@xxxxxxxxxxxxx> a écrit :
>
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/21 下午 11:50 寫道:
> >> +static netdev_tx_t f81604_start_xmit(struct sk_buff *skb,
> >> + struct net_device *netdev)
> >> +{
> >> + struct can_frame *cf = (struct can_frame *)skb->data;
> >> + struct f81604_port_priv *priv = netdev_priv(netdev);
> >> + struct net_device_stats *stats = &netdev->stats;
> >> + int status;
> >> + u8 *ptr;
> >> + u32 id;
> >> +
> >> + if (can_dropped_invalid_skb(netdev, skb))
> >> + return NETDEV_TX_OK;
> >> +
> >> + netif_stop_queue(netdev);
> >> +
> >> + ptr = priv->bulk_write_buffer;
> >> + memset(ptr, 0, F81604_DATA_SIZE);
> >> +
> >> + ptr[0] = F81604_CMD_DATA;
> >> + ptr[1] = min_t(u8, cf->can_dlc & 0xf, 8);
> >> +
> >> + if (cf->can_id & CAN_EFF_FLAG) {
> >> + id = (cf->can_id & CAN_ERR_MASK) << 3;
> >> + ptr[1] |= F81604_EFF_BIT;
> >> + ptr[2] = (id >> 24) & 0xff;
> >> + ptr[3] = (id >> 16) & 0xff;
> >> + ptr[4] = (id >> 8) & 0xff;
> >> + ptr[5] = (id >> 0) & 0xff;
> >> + memcpy(&ptr[6], cf->data, ptr[1]);
> > Rather than manipulating an opaque u8 array, please declare a
> > structure with explicit names.
>
> I had try to declare a struct like below and refactoring code :
>
> struct f81604_bulk_data {
> u8 cmd;
> u8 dlc;
>
> union {
> struct {
> u8 id1, id2;
> u8 data[CAN_MAX_DLEN];
> } sff;
>
> struct {
> u8 id1, id2, id3, id4;
> u8 data[CAN_MAX_DLEN];
> } eff;
> };
> } __attribute__((packed));
>
> This struct can used in TX/RX bulk in/out. Is it ok?
That's nearly it. It is better to declare the struct sff and eff
separately. Also, do not split the id in bytes. Declare it as a little
endian using the __le16 and __le32 types.
Something like this (I let you adjust):
struct f81604_sff {
__le16 id:
u8 data[CAN_MAX_DLEN];
} __attribute__((packed));
struct f81604_eff {
__le32 id;
u8 data[CAN_MAX_DLEN];
} __attribute__((packed));
struct f81604_bulk_data {
u8 cmd;
u8 dlc;
union {
struct f81604_sff sff;
struct f81604_eff eff;
};
} __attribute__((packed));
The __le16 field should be manipulated using cpu_to_leXX() and
leXX_to_cpu() if the field is aligned, if not, it should be
manipulated using {get|set}_unaligned_leXX() (where XX represents the
size in bits).
Also, f81604_bulk_data->dlc is not only a DLC but also carries the
F81604_EFF_BIT flag, right? At least, add documentation to the
structure to clarify this point.
> > +static int f81604_prepare_urbs(struct net_device *netdev)
> > +{
> > + static const u8 bulk_in_addr[F81604_MAX_DEV] = { 0x82, 0x84 };
> > + static const u8 bulk_out_addr[F81604_MAX_DEV] = { 0x01, 0x03 };
> > + static const u8 int_in_addr[F81604_MAX_DEV] = { 0x81, 0x83 };
> > + struct f81604_port_priv *priv = netdev_priv(netdev);
> > + int id = netdev->dev_id;
> > + int i;
> > +
> > + /* initialize to NULL for error recovery */
> > + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> > + priv->read_urb[i] = NULL;
> > priv was allocated with devm_kzalloc() so it should already be zeroed,
> > right? What is the purpose of this loop?
>
> This operation due to following condition:
> f81604_open() -> f81604_close() -> f81604_open() failed.
>
> We had used devm_kzalloc() in f81604_probe(), so first f81604_open() all
> pointers are NULL. But after f81604_close() then f81604_open() second
> times, the URB pointers are not NULLed, it'll makes error on 2nd
> f81604_open()
> with fail.
Makes sense, thanks for the clarification.
Then, please replace your loop by a memset(priv->read_urb, 0,
sizeof(priv->read_urb).
> >> +/* Called by the usb core when driver is unloaded or device is removed */
> >> +static void f81604_disconnect(struct usb_interface *intf)
> >> +{
> >> + struct f81604_priv *priv = usb_get_intfdata(intf);
> >> + int i;
> >> +
> >> + for (i = 0; i < F81604_MAX_DEV; ++i) {
> >> + if (!priv->netdev[i])
> >> + continue;
> >> +
> >> + unregister_netdev(priv->netdev[i]);
> >> + free_candev(priv->netdev[i]);
> >> + }
> > i> +}
>
> Is typo here?
Yes, please ignore.
Yours sincerely,
Vincent Mailhol
