Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Phillip Potter <phil@philpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        Larry Finger <Larry.Finger@lwfinger.net>
Subject: Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
Date:   Mon, 01 Nov 2021 17:30:47 +0100
Message-ID: <11283844.I1tDBM3C2B@localhost.localdomain>
In-Reply-To: <d1a8eb91-adb9-2163-dc3d-9f86ebdc45e3@lwfinger.net>
References: <20211101142732.5638-1-fmdefrancesco@gmail.com> <d1a8eb91-adb9-2163-dc3d-9f86ebdc45e3@lwfinger.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Monday, November 1, 2021 4:11:26 PM CET Larry Finger wrote:
> On 11/1/21 09:27, 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".
> >
> > []
> >
> I am happy that you caught the error before it destroyed every instance of 
> r8188eu.

I don't think so, since we have run this driver with no problems at all :)

SAC bugs can potentially cause serious system hangs at runtime, but they do 
not always cause problems in real execution as you have noticed here with 
this driver. We have used and tested it hundreds of times with no problems.

> Incidentally, I disagree with checkpatch in that I think that 
> sizeof(struct foo) is more descriptive than sizeof(*bar). 

I agree with you in full, but I felt that I had to change it just because of 
the warning output by that tool. I don't like to have my patches discarded 
because they don't fix checkpatch warnings or introduce new ones. 

> If I wanted to check 
> the resulting value of the sizeof(), the second form requires an additional 
> step. It probably does not matter much to the compiler, but when I have to 
do it 
> manually, the extra effort is not negligible.
> 
> Even though I disagree with the philosophy,
> 
> Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> 

Thanks for your "Acked-by" tag.

Fabio