Re: [PATCH] staging: rtl8723bs: Use list iterators and helpers

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/7/21 2:11 PM, Dan Carpenter wrote:
> On Mon, Jun 07, 2021 at 01:36:24PM +0200, Fabio Aiuto wrote:
>> On Mon, Jun 07, 2021 at 04:26:48AM -0700, Guenter Roeck wrote:
>>> On Mon, Jun 07, 2021 at 02:17:04PM +0300, Dan Carpenter wrote:
>>>> On Fri, Jun 04, 2021 at 05:54:22PM -0700, Guenter Roeck wrote:
>>>>> On Fri, Jun 04, 2021 at 07:26:33PM +0200, Fabio Aiuto wrote:
>>>>>> Hello Guenter,
>>>>>>
>>>>>> On Wed, Apr 28, 2021 at 10:33:01AM -0700, Guenter Roeck wrote:
>>>>>>> The rtl8723bs driver manually re-implements list helper functions
>>>>>>> and macros in various ways. Replace with existing list helpers.
>>>>>>
>>>>>> I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY)
>>>>>> and applying the tag staging-5.13-rc4, loading r8723bs makes the whole
>>>>>> system freezing while trying to connect to local AP.
>>>>>>
>>>>>> Only a power off is allowed.
>>>>>>
>>>>>> I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0
>>>>>> (staging: rtl8723bs: Use list iterators and helpers)
>>>>>> introduced the bug.
>>>>>>
>>>>>> I'm trying to find out what's wrong with this patch, have you any suggestions?
>>>>>
>>>>> Some of the iterators needed the _safe variant which I didn't take into account.
>>>>> I thought that was fixed, but maybe some locations were missed.
>>>>
>>>> Ah...  Crud.  As near as I can tell Martin fixed a lot of these in the driver
>>>> he is working on, rtl8188eu.  But they still aren't fixed in rtl8723bs.  I looked
>>>> at the functions and the following loops need to changed to list_for_each_safe().
>>>> (Doing a search for list_del_init() helped during review).
>>>>
>>>
>>> Hans wants the patch introducing the problem reverted, so I won't bother
>>> sending a fix. Sorry for the trouble. Learned a lesson (I hope).
>>>
>>> Guenter
>>>
>>>> expire_timeout_chk() (both loops)
>>>> rtw_acl_remove_sta()
>>>> rtw_sta_flush()
>>>> stop_ap_mode()
>>>>
>>>> rtw_free_network_queue()
>>>> chk_bmc_sleepq_hdl()
>>>> rtw_free_all_stainfo()
>>>> rtw_free_xmitframe_queue()
>>>> dequeue_xmitframes_to_sleeping_queue()
>>>> wakeup_sta_to_xmit()  (both loops)
>>>> xmit_delivery_enabled_frames()
>>>>
>>>> xmit_xmitframes()
>>>> cfg80211_rtw_del_station()
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>>>
>>
>> thanks to Guenter suggestion I made the fix needed, if there's no particular
>> hurry to revert the change I'm submitting it today or tomorrow at most.
>> Will cross check with Dan's hint either.
> 
> Thanks Fabio.  I feel like we can fix this.  No need to revert.

Ack. My suggestion for reverting this was because I did not know a fix was
in the works / almost ready, fixing it indeed is better,

Regards,

Hans