| 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: <20250409-cooperative-elastic-pillbug-672a03-mkl@pengutronix.de>
Date: Wed, 9 Apr 2025 12:21:13 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: a0282524688@...il.com
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
This looks really good now. Some questions and a nitpick inline.
On 09.04.2025 16:27:49, a0282524688@...il.com wrote:
[...]
> +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?
> + } 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()
> +
> + priv->can.can_stats.bus_error++;
> +
> + skb = alloc_can_err_skb(ndev, &cf);
> + if (cf)
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + switch (bus_err) {
> + case NCT6694_CANFD_EVT_ERR_CRC_ERROR:
> + netdev_dbg(ndev, "CRC error\n");
> + ndev->stats.rx_errors++;
> + if (cf)
> + cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> + break;
> +
> + case NCT6694_CANFD_EVT_ERR_STUFF_ERROR:
> + netdev_dbg(ndev, "Stuff error\n");
> + ndev->stats.rx_errors++;
> + if (cf)
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> +
> + case NCT6694_CANFD_EVT_ERR_ACK_ERROR:
> + netdev_dbg(ndev, "Ack error\n");
> + ndev->stats.tx_errors++;
> + if (cf) {
> + cf->can_id |= CAN_ERR_ACK;
> + cf->data[2] |= CAN_ERR_PROT_TX;
> + }
> + break;
> +
> + case NCT6694_CANFD_EVT_ERR_FORM_ERROR:
> + netdev_dbg(ndev, "Form error\n");
> + ndev->stats.rx_errors++;
> + if (cf)
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> +
> + case NCT6694_CANFD_EVT_ERR_BIT_ERROR:
> + netdev_dbg(ndev, "Bit error\n");
> + ndev->stats.tx_errors++;
> + if (cf)
> + cf->data[2] |= CAN_ERR_PROT_TX | CAN_ERR_PROT_BIT;
> + break;
> +
> + default:
> + break;
> + }
> +
> + if (skb)
> + nct6694_canfd_rx_offload(&priv->offload, skb);
> +}
[...]
> +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.
> + 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;
> +}
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists