[...]
>
> - aic_dev = kmalloc(sizeof(struct aic_dev_data), GFP_ATOMIC | GFP_KERNEL);
> + aic_dev = kmalloc(sizeof(struct aic_dev_data), GFP_ATOMIC);
No, this converts the allocation from a robust one which can sleep into a
flakey one which cannot.
If we want to just clean this code up, we should switch to
GFP_KERNEL|__GFP_HIGH
and add a comment explaining why we're turning on __GFP_HIGH (pointlessly,
I suspect).
However I suspect what the code really meant to do was to use just
GFP_KERNEL.
> So combining GFP_ATOMIC with GFP_KERNEL gives you
> "allow io, allow fs, allow waiting, and use emergency pools when it's
> getting tight" which to me looks like a valid, but probably unwanted
> combination.
As a matter of style, the author there could've written (GFP_KERNEL |
__GFP_HIGH) instead, but I'm not sure how much sense that makes
because once we specify GFP_KERNEL, we essentially bring out the heavy
weaponry to *make* some free space for ourselves if it isn't there
already (and we're prepared to sleep when all that happens) -- so
where's the need left to scavenge into emergency pools anymore?
__GFP_HIGH only makes sense for poor atomic contexts for whom sleeping
(when we try_to_free other pages to satisfy our allocation request) is
not an option, which is precisely how things are presently.
[...]
1. If you're in_atomic() context, that GFP_KERNEL is a BUG and *must*
be removed.
2. If not, that GFP_ATOMIC is redundant / nonsensical and *can* be removed.