netdev - Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4EAF21A6.702@pengutronix.de>
Date:	Mon, 31 Oct 2011 23:31:02 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Reuben Dowle <Reuben.Dowle@...ico.com>
CC:	netdev@...r.kernel.org, linux-can@...r.kernel.org
Subject: Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

On 10/31/2011 11:18 PM, Reuben Dowle wrote:
> Currently the flexcan driver uses hardware local echo. This blindly echos all transmitted frames to all receiving sockets, regardless what CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> 
> This patch now submits transmitted frames to be echoed in the transmit complete interrupt, preserving the reference to the sending socket. This allows the can protocol to correctly handle the local echo.
> 
> Signed-off-by: Reuben Dowle <reuben.dowle@...ico.com>

Patch looks quite good. Can you please wrap the description to about 72
chars?

> 
> ---
>  drivers/net/can/flexcan.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e023379..542ada8 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
>  	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
>  
> -	kfree_skb(skb);
> +	can_put_echo_skb(skb, dev, 0);
>  
>  	/* tx_packets is incremented in flexcan_irq */
>  	stats->tx_bytes += cf->can_dlc;
> @@ -612,6 +612,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  		/* tx_bytes is incremented in flexcan_start_xmit */
>  		stats->tx_packets++;
>  		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
> +		can_get_echo_skb(dev, 0);
>  		netif_wake_queue(dev);
>  	}
>  
> @@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device *dev)
>  	int err;
>  	u32 reg_mcr, reg_ctrl;
>  
> +	can_free_echo_skb(dev, 0);

what about putting this to flexcan_chip_stop? Otherwise you risk a
memleak if you do "ifconfig down; rmmod flexcan"

> +
>  	/* enable module */
>  	flexcan_chip_enable(priv);
>  
> @@ -697,12 +700,13 @@ static int flexcan_chip_start(struct net_device *dev)
>  	 * only supervisor access
>  	 * enable warning int
>  	 * choose format C
> +	 * disable local echo
>  	 *
>  	 */
>  	reg_mcr = flexcan_read(&regs->mcr);
>  	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
>  		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
> -		FLEXCAN_MCR_IDAM_C;
> +		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
>  	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	flexcan_write(reg_mcr, &regs->mcr);
>  
> @@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  		goto failed_map;
>  	}
>  
> -	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
> +	dev = alloc_candev(sizeof(struct flexcan_priv), 1);
>  	if (!dev) {
>  		err = -ENOMEM;
>  		goto failed_alloc;
> @@ -978,7 +982,14 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  
>  	dev->netdev_ops = &flexcan_netdev_ops;
>  	dev->irq = irq;
> -	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
> +
> +	/* Driver supports local echo.
> +	 * We support local echo in hardware, however this is not used because
> +	 * hardware local echo loses the sending socket reference
> +	 * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket options
> +	 *  would not work)
> +	 */

IMHO, you can skip this comment. The patch description is good enough.

> +	dev->flags |= IFF_ECHO;
>  
>  	priv = netdev_priv(dev);
>  	priv->can.clock.freq = clock_freq;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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   |


Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)