| 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
| ||
|
Message-ID: <CAOoeyxULns52vAwzsLoXB+BwT+CN+VGBwqrg61pjKJH8bTD5bw@mail.gmail.com>
Date: Thu, 10 Apr 2025 10:40:34 +0800
From: Ming Yu <a0282524688@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
andi.shyti@...nel.org, mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com,
alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-rtc@...r.kernel.org, linux-usb@...r.kernel.org,
Ming Yu <tmyu0@...oton.com>
Subject: Re: [PATCH v9 4/7] can: Add Nuvoton NCT6694 CANFD support
Dear Marc,
Thank you for reviewing.
Marc Kleine-Budde <mkl@...gutronix.de> 於 2025年4月9日 週三 下午6:21寫道：
...
> > +static void nct6694_canfd_handle_state_change(struct net_device *ndev, u8 status)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + enum can_state new_state, rx_state, tx_state;
> > + struct can_berr_counter bec;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > +
> > + nct6694_canfd_get_berr_counter(ndev, &bec);
> > + can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state);
> > +
> > + new_state = max(tx_state, rx_state);
> > +
> > + /* state hasn't changed */
> > + if (new_state == priv->can.state)
> > + return;
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > +
> > + can_change_state(ndev, cf, tx_state, rx_state);
> > +
> > + if (new_state == CAN_STATE_BUS_OFF) {
> > + can_bus_off(ndev);
>
> What does your device do when it goes into bus off? Does it recover itself?
>
No, the device does not support automatic bus-off recovery. It
requires an explicit CAN Setting and Initialization(CMD0) command to
re-initialize the controller after entering bus-off state.
> > + } else if (cf) {
> > + cf->can_id |= CAN_ERR_CNT;
> > + cf->data[6] = bec.txerr;
> > + cf->data[7] = bec.rxerr;
> > + }
> > +
> > + if (skb)
> > + nct6694_canfd_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_canfd_handle_bus_err(struct net_device *ndev, u8 bus_err)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > +
> > + if (bus_err == NCT6694_CANFD_EVT_ERR_NO_ERROR)
> > + return;
>
> I think this has already been checked nct6694_canfd_irq()
>
Drop it in v10.
> > +
...
> > +static int nct6694_canfd_start(struct net_device *ndev)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > + const struct can_bittiming *n_bt = &priv->can.bittiming;
> > + struct nct6694_canfd_setting *setting __free(kfree) = NULL;
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CANFD_MOD,
> > + .cmd = NCT6694_CANFD_SETTING,
> > + .sel = ndev->dev_port,
> > + .len = cpu_to_le16(sizeof(*setting))
> > + };
> > + int ret;
> > +
> > + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > + if (!setting)
> > + return -ENOMEM;
> > +
> > + setting->nbr = cpu_to_le32(n_bt->bitrate);
> > + setting->dbr = cpu_to_le32(d_bt->bitrate);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_MON);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_NISO);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_LBCK);
> > +
> > + setting->nbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NSJW,
> > + n_bt->sjw - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NBRP,
> > + n_bt->brp - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG2,
> > + n_bt->phase_seg2 - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG1,
> > + n_bt->prop_seg + n_bt->phase_seg1 - 1));
> > +
> > + setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
> > + d_bt->sjw - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
> > + d_bt->brp - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
> > + d_bt->phase_seg2 - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
> > + d_bt->prop_seg + d_bt->phase_seg1 - 1));
>
> What does your device do, if you set the bitrates _and_ the bit timing
> parameters? They are redundant.
>
The firmware calculates the default bit timing parameters when it
receives the bitrates, and then overwrites them if it later receives
explicit bit timing parameters.
To avoid confusion and ensure consistent behavior, I will remove the
bitrate setting logic in next patch. Instead, the bit timing will be
determined solely based on the provided bit timing parameters.
> > + setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1 |
> > + NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP;
> > +
> > + ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> > + if (ret)
> > + return ret;
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + return 0;
> > +}
>
Best regards,
Ming
Powered by blists - more mailing lists