On 10/4/19 11:02 AM, Anssi Hannula wrote:
> Userspace can signal with CAN_CTRLMODE_BERR_REPORTING whether they need
> reporting of bus errors (CAN_ERR_BUSERROR) or not.
>
> However, xilinx_can driver currently always sends CAN_ERR_BUSERROR
> frames to userspace on bus errors.
>
> To improve performance on error conditions when bus error reporting is
> not needed, avoid sending CAN_ERR_BUSERROR frames unless requested via
> CAN_CTRLMODE_BERR_REPORTING.
>
> The error interrupt is still kept enabled as there is no dedicated state
> transition interrupt, but just disabling error frame submission still
> yields a significant performance improvement. In a simple test with
> continuous bus errors and no userspace programs reading/writing CAN I
> saw system CPU load reduced by 1/3.
>
> Tested on a ZynqMP board with CAN-FD v1.0.
>
> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
> ---
> drivers/net/can/xilinx_can.c | 84 +++++++++++++++++++-----------------
> 1 file changed, 45 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 911b34316c9d..9b9ec07f7e5b 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -471,6 +471,10 @@ static int xcan_chip_start(struct net_device *ndev)
> return err;
>
> /* Enable interrupts */
> + /* We enable the ERROR interrupt even with CAN_CTRLMODE_BERR_REPORTING
> + * disabled as there is no dedicated interrupt for a state change to
> + * ERROR_WARNING/ERROR_PASSIVE.
> + */
> ier = XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |
> XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK |
> XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> @@ -981,11 +985,10 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
> {
> struct xcan_priv *priv = netdev_priv(ndev);
> struct net_device_stats *stats = &ndev->stats;
> - struct can_frame *cf;
> - struct sk_buff *skb;
> + struct can_frame cf;
> u32 err_status;
>
> - skb = alloc_can_err_skb(ndev, &cf);
> + memset(&cf, 0, sizeof(cf));
You can use C99 initializers instead of the memset.
>
> err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
> priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
> @@ -996,32 +999,27 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
> /* Leave device in Config Mode in bus-off state */
> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> can_bus_off(ndev);
> - if (skb)
> - cf->can_id |= CAN_ERR_BUSOFF;
> + cf.can_id |= CAN_ERR_BUSOFF;
> } else {
> enum can_state new_state = xcan_current_error_state(ndev);
>
> if (new_state != priv->can.state)
> - xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
> + xcan_set_error_state(ndev, new_state, &cf);
> }
>
> /* Check for Arbitration lost interrupt */
> if (isr & XCAN_IXR_ARBLST_MASK) {
> priv->can.can_stats.arbitration_lost++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_LOSTARB;
> - cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> - }
> + cf.can_id |= CAN_ERR_LOSTARB;
> + cf.data[0] = CAN_ERR_LOSTARB_UNSPEC;
> }
>
> /* Check for RX FIFO Overflow interrupt */
> if (isr & XCAN_IXR_RXOFLW_MASK) {
> stats->rx_over_errors++;
> stats->rx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> - }
> + cf.can_id |= CAN_ERR_CRTL;
> + cf.data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> }
>
> /* Check for RX Match Not Finished interrupt */
> @@ -1029,68 +1027,76 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
> stats->rx_dropped++;
> stats->rx_errors++;
> netdev_err(ndev, "RX match not finished, frame discarded\n");
> - if (skb) {
> - cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] |= CAN_ERR_CRTL_UNSPEC;
> - }
> + cf.can_id |= CAN_ERR_CRTL;
> + cf.data[1] |= CAN_ERR_CRTL_UNSPEC;
> }
>
> /* Check for error interrupt */
> if (isr & XCAN_IXR_ERROR_MASK) {
> - if (skb)
> - cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> + bool berr_reporting = !!(priv->can.ctrlmode &
> + CAN_CTRLMODE_BERR_REPORTING);
Please use if() instead of !!
> +
> + if (berr_reporting)
> + cf.can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>
> /* Check for Ack error interrupt */
> if (err_status & XCAN_ESR_ACKER_MASK) {
> stats->tx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_ACK;
> - cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> + if (berr_reporting) {
> + cf.can_id |= CAN_ERR_ACK;
> + cf.data[3] = CAN_ERR_PROT_LOC_ACK;
> }
> }
>
> /* Check for Bit error interrupt */
> if (err_status & XCAN_ESR_BERR_MASK) {
> stats->tx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_BIT;
> + if (berr_reporting) {
> + cf.can_id |= CAN_ERR_PROT;
> + cf.data[2] = CAN_ERR_PROT_BIT;
> }
> }
>
> /* Check for Stuff error interrupt */
> if (err_status & XCAN_ESR_STER_MASK) {
> stats->rx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_STUFF;
> + if (berr_reporting) {
> + cf.can_id |= CAN_ERR_PROT;
> + cf.data[2] = CAN_ERR_PROT_STUFF;
> }
> }
>
> /* Check for Form error interrupt */
> if (err_status & XCAN_ESR_FMER_MASK) {
> stats->rx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_FORM;
> + if (berr_reporting) {
> + cf.can_id |= CAN_ERR_PROT;
> + cf.data[2] = CAN_ERR_PROT_FORM;
> }
> }
>
> /* Check for CRC error interrupt */
> if (err_status & XCAN_ESR_CRCER_MASK) {
> stats->rx_errors++;
> - if (skb) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> + if (berr_reporting) {
> + cf.can_id |= CAN_ERR_PROT;
> + cf.data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> }
> }
> priv->can.can_stats.bus_error++;
> }
>
> - if (skb) {
> - stats->rx_packets++;
> - stats->rx_bytes += cf->can_dlc;
> - netif_rx(skb);
> + if (cf.can_id) {
> + struct can_frame *skb_cf;
> + struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf);
> +
> + if (skb) {
> + skb_cf->can_id |= cf.can_id;
> + memcpy(skb_cf->data, cf.data, CAN_ERR_DLC);
> + stats->rx_packets++;
> + stats->rx_bytes += CAN_ERR_DLC;
> + netif_rx(skb);
> + }
> }
>
> netdev_dbg(ndev, "%s: error status register:0x%x\n",
I've send a v2 which incorporates these changes.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
