Received-SPF: pass (google.com: domain of linux-wireless+bounces-3544-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: iwlwifi RFC related to iwl_mvm_tx_reclaim
To: Johannes Berg <johannes@sipsolutions.net>,
 linux-wireless <linux-wireless@vger.kernel.org>
References: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
 <9779efb62cec04926e3bb2575a8aecc8bbf0e7ff.camel@sipsolutions.net>
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <2839f687-88af-ecd8-c6cb-7bdaf1808496@candelatech.com>
Date: Tue, 13 Feb 2024 06:13:39 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <9779efb62cec04926e3bb2575a8aecc8bbf0e7ff.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-MW
Content-Transfer-Encoding: 7bit

On 2/13/24 1:37 AM, Johannes Berg wrote:
> On Mon, 2024-02-12 at 15:22 -0800, Ben Greear wrote:
>> I'm seeing a crash due to accessing an 'sta' object in this method that is '-2' as integer.
>>
>> It fails the initial check for null STA, but I'm thinking it might should check for IS_ERR(sta)
>> as well.
>>
>> (I have my own patch that references sta before the IS_ERR check later in the code, and this
>> causes the crash I'm seeing.  I guess upstream will not crash in this situation.).
> 
> Indeed.
> 
>> My question:  Is the patch below a preferred approach, or should I add special checks to where I
>> access sta and only exit the method lower where it already has the IS_ERR(sta) check?
> 
> You can do whatever you want in your tree, but I guess generally I'd
> advocate you assume that the code does what it should ;-)
> 
> In this case, ERR_PTR(-ENOENT) is used to indicate the station is being
> deleted, but has not yet been fully removed, and so indeed we still want
> to reclaim the frames here correctly, which the code does.
> 
> The comment below even kind of explains that?

If sta is NULL, we should still reclaim the frames?  If so the check earlier in the code where
it returns early if sta is NULL could be deleted, and add a null check down near the IS_ERR
check?

I can (and have) fixed the bug in my code, I'm trying to understand if the driver
itself needs improving here to cover an edge case.

Thanks,
Ben

> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com