Re: [PATCH 1/3] e1000: increase skb size to prevent dma over skb boundary

[Franco Fichtner <franco@lastsummer.de>]

Hi Neil,

Neil Horman wrote:
> Update e1000 driver to not allow dma beyond the end of the allocated skb
>     
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>
>
> e1000_main.c |   34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
>
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index 7e855f9..7600deb 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -1667,6 +1667,19 @@ int e1000_setup_all_rx_resources(struct e1000_adapter *adapter)
>  	return err;
>  }
>  
> +static inline u32 normalize_rx_len(u32 len)
> +{
> +        u32 match, last_match;
> +
>   
Skip newline and get rid of last_match. Also, there is a whitespace error...
> +
> +        for (match = 0x100; match <= 0x4000; match *= 2) {
> +                if (len <= match)
> +                        return match;
> +        }
> +
>   
> +        return 0;
> +}
>   
You should return (match >> 1), which is the largest size possible. (Which
is exactly what you need here).
> +
>  /**
>   * e1000_setup_rctl - configure the receive control registers
>   * @adapter: Board private structure
> @@ -1675,6 +1688,7 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
>  	u32 rctl;
> +	u32 normed_rx_len;
>  
>  	rctl = er32(RCTL);
>  
> @@ -1697,7 +1711,25 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
>  	/* Setup buffer sizes */
>  	rctl &= ~E1000_RCTL_SZ_4096;
>  	rctl |= E1000_RCTL_BSEX;
> -	switch (adapter->rx_buffer_len) {
> +
> +	/*
> +	 * We need to normalize the rx_buffer_len here
> +	 * since the hardware only knows about 7 discrete
> +	 * frame lengths here.  To accomodate that we need
> +	 * to set the rx length in the hardware to the next highest
> +	 * size over the rx_buffer_len, then increase rx_buffer_len
> +	 * to match it, so that we can get a full mtu sized frame 
> +	 */
> +	normed_rx_len = normalize_rx_len(adapter->rx_buffer_len);
> +	
> +	if (!normed_rx_len) {
> +		printk(KERN_ERR "No valid rx len found, assume 2048\n");
> +		normed_rx_len = 0x800;
> +	}
> +
> +	adapter->rx_buffer_len = normed_rx_len;
>   
If you modify rx_buffer_len anyway, then get rid of normed_rx_len and
do a quick

    adapter->rx_buffer_len = normalize_rx_len(adapter->rx_buffer_len);

instead. With the modification above, it never fails, so no need to check
for !normed_rx_len.

But I don't really know the context of this change. Is it okay to shorten
rx_buffer_len here? Why was it not set appropriately as the driver
expects?

Oh, BTW, the default case in the switch statement is stupid and should
be removed.

> +
> +	switch (normed_rx_len) {
>  		case E1000_RXBUFFER_256:
>  			rctl |= E1000_RCTL_SZ_256;
>  			rctl &= ~E1000_RCTL_BSEX;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Franco