Re: [PATCH v2] r8169: hardening and stability improvements

[Andrew Lunn <andrew@lunn.ch>]

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 9c601f271c02..66d7dcd8bf7b 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -3981,19 +3981,39 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
>  	int node = dev_to_node(d);
>  	dma_addr_t mapping;
>  	struct page *data;
> +	gfp_t gfp_flags = GFP_KERNEL;
>  
> -	data = alloc_pages_node(node, GFP_KERNEL, get_order(R8169_RX_BUF_SIZE));
> -	if (!data)
> -		return NULL;
> +	/* Use atomic allocation in interrupt/atomic context */
> +	if (in_atomic() || irqs_disabled())
> +		gfp_flags = GFP_ATOMIC;

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */
#define in_atomic()	(preempt_count() != 0)

You need to explain why you ignored this last line, and why this is
safe in this context. For a change like this, which breaks the normal
rules, you want it in a patch of its own, so the explanation can be in
the commit message and it is then very clear why you are ignoring the
rules.

Looking at the rest of the patch, a lot of the changes are
questionable. Splitting it up will help you describe why you are
making each change, why it is needed.

    Andrew

---
pw-bot: cr