From: Peter Hong <peter_hong@fintek.com.tw>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: <wg@grandegger.com>, <mkl@pengutronix.de>,
<michal.swiatkowski@linux.intel.com>,
<Steen.Hegelund@microchip.com>, <davem@davemloft.net>,
<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
<frank.jungclaus@esd.eu>, <linux-kernel@vger.kernel.org>,
<linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
<hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V2] can: usb: f81604: add Fintek F81604 support
Date: Thu, 23 Mar 2023 13:11:20 +0800 [thread overview]
Message-ID: <f71f1f59-f729-2c8c-f6da-8474be2074b1@fintek.com.tw> (raw)
In-Reply-To: <CAMZ6RqJWg1H6Yo3nhsa-Kk-WdU=ZH39ecWaE6wiuKRJe1gLMkQ@mail.gmail.com>
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?
> +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.
>> +/* 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?
Thanks.
next prev parent reply other threads:[~2023-03-23 5:13 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-21 8:11 [PATCH V2] can: usb: f81604: add Fintek F81604 support Ji-Ze Hong (Peter Hong)
2023-03-21 12:42 ` Steen.Hegelund
2023-03-21 15:50 ` Vincent MAILHOL
2023-03-23 5:11 ` Peter Hong [this message]
2023-03-23 5:54 ` Vincent MAILHOL
2023-03-23 9:22 ` Vincent MAILHOL
2023-03-26 2:30 ` Vincent MAILHOL
2023-03-24 16:13 ` Marc Kleine-Budde
2023-03-26 2:37 ` Vincent MAILHOL
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f71f1f59-f729-2c8c-f6da-8474be2074b1@fintek.com.tw \
--to=peter_hong@fintek.com.tw \
--cc=Steen.Hegelund@microchip.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=frank.jungclaus@esd.eu \
--cc=hpeter+linux_kernel@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=michal.swiatkowski@linux.intel.com \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).