diff mbox series

[RFC,4/8] wifi: ath11k: remove MHI LOOPBACK channels

Message ID 20231110102202.3168243-5-kvalo@kernel.org
State New
Headers show
Series wifi: ath11k: hibernation support | expand

Commit Message

Kalle Valo Nov. 10, 2023, 10:21 a.m. UTC
From: Baochen Qiang <quic_bqiang@quicinc.com>

There is no driver to match these two channels, so
remove them. This fixes warnings from MHI subsystem during suspend:

mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30

Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/mhi.c | 28 ---------------------------
 1 file changed, 28 deletions(-)

Comments

Jeffrey Hugo Nov. 10, 2023, 4:54 p.m. UTC | #1
On 11/10/2023 3:21 AM, Kalle Valo wrote:
> From: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> There is no driver to match these two channels, so
> remove them. This fixes warnings from MHI subsystem during suspend:
> 
> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting

This feels like just masking a real issue.

If LOOPBACK is not being consumed, then the channel should never go into 
the start state.  Why would we be trying to transition to the reset 
state then?

-Jeff
Baochen Qiang Nov. 12, 2023, 4:24 a.m. UTC | #2
On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>
>> There is no driver to match these two channels, so
>> remove them. This fixes warnings from MHI subsystem during suspend:
>>
>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>
> This feels like just masking a real issue.
>
> If LOOPBACK is not being consumed, then the channel should never go 
> into the start state.  Why would we be trying to transition to the 
> reset state then?
>
> -Jeff
That is because, with patch 'bus: mhi: host: add new interfaces to 
handle MHI channels directly' in this patch set, ath11k is able to call 
mhi_unprepare_all_from_transfer(), which will reset all channels.
Jeffrey Hugo Nov. 12, 2023, 4:15 p.m. UTC | #3
On 11/11/2023 9:24 PM, Baochen Qiang wrote:
> 
> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>
>>> There is no driver to match these two channels, so
>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>
>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>
>> This feels like just masking a real issue.
>>
>> If LOOPBACK is not being consumed, then the channel should never go 
>> into the start state.  Why would we be trying to transition to the 
>> reset state then?
>>
>> -Jeff
> That is because, with patch 'bus: mhi: host: add new interfaces to 
> handle MHI channels directly' in this patch set, ath11k is able to call 
> mhi_unprepare_all_from_transfer(), which will reset all channels.

that implementation is flawed if it is causing this.  Looks like you 
never check to see if the channel was prepared in the first place.

If you go fix that, then it looks like this change is not needed.
Baochen Qiang Nov. 13, 2023, 12:30 a.m. UTC | #4
On 11/13/2023 12:15 AM, Jeffrey Hugo wrote:
> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>
>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> There is no driver to match these two channels, so
>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>
>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>
>>> This feels like just masking a real issue.
>>>
>>> If LOOPBACK is not being consumed, then the channel should never go 
>>> into the start state.  Why would we be trying to transition to the 
>>> reset state then?
>>>
>>> -Jeff
>> That is because, with patch 'bus: mhi: host: add new interfaces to 
>> handle MHI channels directly' in this patch set, ath11k is able to 
>> call mhi_unprepare_all_from_transfer(), which will reset all channels.
>
> that implementation is flawed if it is causing this.  Looks like you 
> never check to see if the channel was prepared in the first place.
>
> If you go fix that, then it looks like this change is not needed.

Sure, will check whether channel is in MHI_CH_STATE_ENABLED state in 
next version.
Kalle Valo Nov. 13, 2023, 2:15 p.m. UTC | #5
Jeffrey Hugo <quic_jhugo@quicinc.com> writes:

> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>
>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> There is no driver to match these two channels, so
>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>
>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>
>>> This feels like just masking a real issue.
>>>
>>> If LOOPBACK is not being consumed, then the channel should never go
>>> into the start state.  Why would we be trying to transition to the
>>> reset state then?
>>>
>>> -Jeff
>> That is because, with patch 'bus: mhi: host: add new interfaces to
>> handle MHI channels directly' in this patch set, ath11k is able to
>> call mhi_unprepare_all_from_transfer(), which will reset all
>> channels.
>
> that implementation is flawed if it is causing this.  Looks like you
> never check to see if the channel was prepared in the first place.
>
> If you go fix that, then it looks like this change is not needed.

BTW what do these loopback channels do? I didn't notice any difference
in the functionality so I'm wondering the reason for these.
Jeffrey Hugo Nov. 13, 2023, 2:26 p.m. UTC | #6
On 11/13/2023 7:15 AM, Kalle Valo wrote:
> Jeffrey Hugo <quic_jhugo@quicinc.com> writes:
> 
>> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>
>>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>
>>>>> There is no driver to match these two channels, so
>>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>>
>>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>>
>>>> This feels like just masking a real issue.
>>>>
>>>> If LOOPBACK is not being consumed, then the channel should never go
>>>> into the start state.  Why would we be trying to transition to the
>>>> reset state then?
>>>>
>>>> -Jeff
>>> That is because, with patch 'bus: mhi: host: add new interfaces to
>>> handle MHI channels directly' in this patch set, ath11k is able to
>>> call mhi_unprepare_all_from_transfer(), which will reset all
>>> channels.
>>
>> that implementation is flawed if it is causing this.  Looks like you
>> never check to see if the channel was prepared in the first place.
>>
>> If you go fix that, then it looks like this change is not needed.
> 
> BTW what do these loopback channels do? I didn't notice any difference
> in the functionality so I'm wondering the reason for these.
> 

The loopback channel is defined as a service where any data the host 
sends to the device is immediately sent back to the host, unmodified. 
The typical usecase is smoke test and performance profiling.

I do not object to the removal of the channel from the atheros devices, 
assuming suitable justification.  Not having a use for the channel seems 
like good justification.  Working around a bug seem more like a hack 
than proper justification.
Kalle Valo Nov. 13, 2023, 3:04 p.m. UTC | #7
Jeffrey Hugo <quic_jhugo@quicinc.com> writes:

> On 11/13/2023 7:15 AM, Kalle Valo wrote:
>
>> Jeffrey Hugo <quic_jhugo@quicinc.com> writes:
>> 
>>> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>>
>>>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>>
>>>>>> There is no driver to match these two channels, so
>>>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>>>
>>>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>>>
>>>>> This feels like just masking a real issue.
>>>>>
>>>>> If LOOPBACK is not being consumed, then the channel should never go
>>>>> into the start state.  Why would we be trying to transition to the
>>>>> reset state then?
>>>>>
>>>>> -Jeff
>>>> That is because, with patch 'bus: mhi: host: add new interfaces to
>>>> handle MHI channels directly' in this patch set, ath11k is able to
>>>> call mhi_unprepare_all_from_transfer(), which will reset all
>>>> channels.
>>>
>>> that implementation is flawed if it is causing this.  Looks like you
>>> never check to see if the channel was prepared in the first place.
>>>
>>> If you go fix that, then it looks like this change is not needed.
>> BTW what do these loopback channels do? I didn't notice any
>> difference
>> in the functionality so I'm wondering the reason for these.
>> 
>
> The loopback channel is defined as a service where any data the host
> sends to the device is immediately sent back to the host, unmodified.
> The typical usecase is smoke test and performance profiling.

Thanks, good to know. We don't have any such functionality in ath11k
right now and if we ever do we can always add the channels back.
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index afeabd6ecc67..d4fe1d1c86e4 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -21,34 +21,6 @@ 
 #define RDDM_DUMP_SIZE	0x420000
 
 static struct mhi_channel_config ath11k_mhi_channels_qca6390[] = {
-	{
-		.num = 0,
-		.name = "LOOPBACK",
-		.num_elements = 32,
-		.event_ring = 0,
-		.dir = DMA_TO_DEVICE,
-		.ee_mask = 0x4,
-		.pollcfg = 0,
-		.doorbell = MHI_DB_BRST_DISABLE,
-		.lpm_notify = false,
-		.offload_channel = false,
-		.doorbell_mode_switch = false,
-		.auto_queue = false,
-	},
-	{
-		.num = 1,
-		.name = "LOOPBACK",
-		.num_elements = 32,
-		.event_ring = 0,
-		.dir = DMA_FROM_DEVICE,
-		.ee_mask = 0x4,
-		.pollcfg = 0,
-		.doorbell = MHI_DB_BRST_DISABLE,
-		.lpm_notify = false,
-		.offload_channel = false,
-		.doorbell_mode_switch = false,
-		.auto_queue = false,
-	},
 	{
 		.num = 20,
 		.name = "IPCR",