[PATCH] rtw88: consider triggering state of simulating fw crash

[PATCH] rtw88: consider triggering state of simulating fw crash

[Ping-Ke Shih]

From: Zong-Zhe Yang <kevin_yang@realtek.com>

In certain cases, triggering fw crash simulation via fw_crash debugfs
will take a while. If the state is queried too early before restart
begins processing, it may mistakenly think restart process has been
done. If some tests are started at this time, something unexpected
might happen due to the follow-up restart process.

To avoid that, we consider the triggering state.

Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/debug.c | 5 ++++-
 drivers/net/wireless/realtek/rtw88/main.c  | 1 +
 drivers/net/wireless/realtek/rtw88/main.h  | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
index babf7fb238cc..682b23502e6e 100644
--- a/drivers/net/wireless/realtek/rtw88/debug.c
+++ b/drivers/net/wireless/realtek/rtw88/debug.c
@@ -886,6 +886,7 @@ static ssize_t rtw_debugfs_set_fw_crash(struct file *filp,
 
 	mutex_lock(&rtwdev->mutex);
 	rtw_leave_lps_deep(rtwdev);
+	set_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags);
 	rtw_write8(rtwdev, REG_HRCV_MSG, 1);
 	mutex_unlock(&rtwdev->mutex);
 
@@ -897,7 +898,9 @@ static int rtw_debugfs_get_fw_crash(struct seq_file *m, void *v)
 	struct rtw_debugfs_priv *debugfs_priv = m->private;
 	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
 
-	seq_printf(m, "%d\n", test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
+	seq_printf(m, "%d\n",
+		   test_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags) ||
+		   test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
 	return 0;
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index cee2acabb042..a0d4d6e31fb4 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -564,6 +564,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
 	int ret = 0;
 
 	set_bit(RTW_FLAG_RESTARTING, rtwdev->flags);
+	clear_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags);
 
 	ret = rtw_fwcd_prep(rtwdev);
 	if (ret)
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 723316347876..bbdd535b64e7 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -363,6 +363,7 @@ enum rtw_flags {
 	RTW_FLAG_BUSY_TRAFFIC,
 	RTW_FLAG_WOWLAN,
 	RTW_FLAG_RESTARTING,
+	RTW_FLAG_RESTART_TRIGGERING,
 
 	NUM_OF_RTW_FLAGS,
 };
-- 
2.25.1

Re: [PATCH] rtw88: consider triggering state of simulating fw crash

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> writes:

> From: Zong-Zhe Yang <kevin_yang@realtek.com>
>
> In certain cases, triggering fw crash simulation via fw_crash debugfs
> will take a while. If the state is queried too early before restart
> begins processing, it may mistakenly think restart process has been
> done. If some tests are started at this time, something unexpected
> might happen due to the follow-up restart process.
>
> To avoid that, we consider the triggering state.
>
> Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/debug.c | 5 ++++-
>  drivers/net/wireless/realtek/rtw88/main.c  | 1 +
>  drivers/net/wireless/realtek/rtw88/main.h  | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
> index babf7fb238cc..682b23502e6e 100644
> --- a/drivers/net/wireless/realtek/rtw88/debug.c
> +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> @@ -886,6 +886,7 @@ static ssize_t rtw_debugfs_set_fw_crash(struct file *filp,
>  
>  	mutex_lock(&rtwdev->mutex);
>  	rtw_leave_lps_deep(rtwdev);
> +	set_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags);
>  	rtw_write8(rtwdev, REG_HRCV_MSG, 1);
>  	mutex_unlock(&rtwdev->mutex);
>  
> @@ -897,7 +898,9 @@ static int rtw_debugfs_get_fw_crash(struct seq_file *m, void *v)
>  	struct rtw_debugfs_priv *debugfs_priv = m->private;
>  	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
>  
> -	seq_printf(m, "%d\n", test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
> +	seq_printf(m, "%d\n",
> +		   test_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags) ||
> +		   test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
>  	return 0;
>  }

You use the verb "consider" both in the title and the commit log, but
it's not really telling much (though I admit my english isn't very
good). From looking at the patch all I see is that it prints the state
of RTW_FLAG_RESTART_TRIGGERING flag. How is that "considering" anything
and how does that improve any of this?

Can you improve the commit log and explain this is in detail? And what's
"it" in this case?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH] rtw88: consider triggering state of simulating fw crash

[Pkshih]

> -----Original Message-----
> From: kvalo=codeaurora.org@mg.codeaurora.org <kvalo=codeaurora.org@mg.codeaurora.org> On
> Behalf Of Kalle Valo
> Sent: Friday, October 1, 2021 2:19 PM
> To: Pkshih <pkshih@realtek.com>
> Cc: tony0620emma@gmail.com; linux-wireless@vger.kernel.org; Kevin Yang
> <kevin_yang@realtek.com>
> Subject: Re: [PATCH] rtw88: consider triggering state of simulating fw crash
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> >
> > In certain cases, triggering fw crash simulation via fw_crash debugfs
> > will take a while. If the state is queried too early before restart
> > begins processing, it may mistakenly think restart process has been
> > done. If some tests are started at this time, something unexpected
> > might happen due to the follow-up restart process.
> >
> > To avoid that, we consider the triggering state.
> >
> > Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > ---
> >  drivers/net/wireless/realtek/rtw88/debug.c | 5 ++++-
> >  drivers/net/wireless/realtek/rtw88/main.c  | 1 +
> >  drivers/net/wireless/realtek/rtw88/main.h  | 1 +
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c
> b/drivers/net/wireless/realtek/rtw88/debug.c
> > index babf7fb238cc..682b23502e6e 100644
> > --- a/drivers/net/wireless/realtek/rtw88/debug.c
> > +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> > @@ -886,6 +886,7 @@ static ssize_t rtw_debugfs_set_fw_crash(struct file *filp,
> >
> >  	mutex_lock(&rtwdev->mutex);
> >  	rtw_leave_lps_deep(rtwdev);
> > +	set_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags);
> >  	rtw_write8(rtwdev, REG_HRCV_MSG, 1);
> >  	mutex_unlock(&rtwdev->mutex);
> >
> > @@ -897,7 +898,9 @@ static int rtw_debugfs_get_fw_crash(struct seq_file *m, void *v)
> >  	struct rtw_debugfs_priv *debugfs_priv = m->private;
> >  	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
> >
> > -	seq_printf(m, "%d\n", test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
> > +	seq_printf(m, "%d\n",
> > +		   test_bit(RTW_FLAG_RESTART_TRIGGERING, rtwdev->flags) ||
> > +		   test_bit(RTW_FLAG_RESTARTING, rtwdev->flags));
> >  	return 0;
> >  }
> 
> You use the verb "consider" both in the title and the commit log, but
> it's not really telling much (though I admit my english isn't very
> good). From looking at the patch all I see is that it prints the state
> of RTW_FLAG_RESTART_TRIGGERING flag. How is that "considering" anything
> and how does that improve any of this?
> 
> Can you improve the commit log and explain this is in detail? And what's
> "it" in this case?
> 

In order to make it clear, we change the title and provide the detail
by v2 [1].

[1] https://lore.kernel.org/linux-wireless/20211001082301.4805-1-pkshih@realtek.com/T/#u

--
Ping-Ke