From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
Phillip Potter <phil@philpotter.co.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
Date: Fri, 05 Nov 2021 16:18:39 +0100 [thread overview]
Message-ID: <3198013.HnMX8GfXRX@localhost.localdomain> (raw)
In-Reply-To: <20211105132551.GB2001@kadam>
On Friday, November 5, 2021 2:25:52 PM CET Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 08:18:47PM +0100, Fabio M. De Francesco wrote:
> > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > report_del_sta_event(). This function is called while holding spinlocks,
> > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> > allocation is high priority and must not sleep.
> >
> > This issue is detected by Smatch which emits the following warning:
> > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> > warn: sleeping in atomic context".
> >
> > After the change, the post-commit hook output the following message:
> > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > kzalloc(sizeof(struct cmd_obj)...)".
> >
> > According to the above "CHECK", use the preferred style in the first
> > kzalloc().
> >
> > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and
kzalloc()")
>
> This is not the correct Fixes tag. The original allocation wrappers
> checked in_interrupt() they did not check in_atomic() so they had same
> bug. The correct tag is:
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for
RTL8188eu driver")
>
> regards,
> dan carpenter
Hello Dan,
I'm sorry but I surely missing something, therefore, before making changes I
need to understand this subject a little better. Let me explain what I am
missing...
The two kzalloc() in report_del_sta_event() are called while spinlocks are
held and bottom halves are disabled by spin_lock_bh(). If I remember it
correctly spin_lock_bh() finally calls __local_bh_disable_ip() to disable
bottom halves on local CPU before actually acquiring the lock.
This is the code and inline documentation of in_interrupt():
/* in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled" */
#define irq_count() (nmi_count() | hardirq_count() | softirq_count())
#define in_interrupt() (irq_count())
And this is the code and inline documentation of in_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)
To summarize, I think that using in_interrupt() in the old wrappers was the
wiser choice. Therefore this patch fixes 79f712ea994d ("staging: r8188eu:
Remove wrappers for kalloc() and kzalloc()").
I know that I have so little experience that I shouldn't even discuss this
topics. However, I would appreciate if you may explain with some more details
why in_atomic() should have been preferred over in_interrupt() in the old
wrappers that were removed with commit 79f712ea994d.
Thank you very much in advance,
Fabio M. De Francesco
next prev parent reply other threads:[~2021-11-05 15:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-01 19:18 [PATCH v3] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context Fabio M. De Francesco
2021-11-05 13:25 ` Dan Carpenter
2021-11-05 15:18 ` Fabio M. De Francesco [this message]
2021-11-05 15:36 ` Dan Carpenter
2021-11-05 16:05 ` Fabio M. De Francesco
2021-11-07 11:43 ` Fabio M. De Francesco
2021-11-07 12:38 ` Greg Kroah-Hartman
2021-11-07 13:15 ` Fabio M. De Francesco
2021-11-07 13:29 ` Greg Kroah-Hartman
2021-11-07 14:03 ` Fabio M. De Francesco
2021-11-07 14:17 ` Greg Kroah-Hartman
2021-11-07 14:30 ` Fabio M. De Francesco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3198013.HnMX8GfXRX@localhost.localdomain \
--to=fmdefrancesco@gmail.com \
--cc=Larry.Finger@lwfinger.net \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=phil@philpotter.co.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).