Re: [PATCH net-next v4 02/13] net: wwan: t7xx: Add control DMA interface

On 1/19/2022 1:52 AM, Ilpo Järvinen wrote:

On Tue, 18 Jan 2022, Martinez, Ricardo wrote:

quoted

On 1/18/2022 6:13 AM, Ilpo Järvinen wrote:

quoted

On Thu, 13 Jan 2022, Ricardo Martinez wrote:

...

quoted

quoted

+static bool t7xx_cldma_qs_are_active(struct t7xx_cldma_hw *hw_info)
+{
+	unsigned int tx_active;
+	unsigned int rx_active;
+
+	tx_active = t7xx_cldma_hw_queue_status(hw_info, CLDMA_ALL_Q, MTK_TX);
+	rx_active = t7xx_cldma_hw_queue_status(hw_info, CLDMA_ALL_Q, MTK_RX);
+	if (tx_active == CLDMA_INVALID_STATUS || rx_active ==
CLDMA_INVALID_STATUS)
These cannot ever be true because of mask in t7xx_cldma_hw_queue_status().

t7xx_cldma_hw_queue_status() shouldn't apply the mask for CLDMA_ALL_Q.

I guess it shouldn't but it currently does apply 0xff (CLDMA_ALL_Q) as
mask in that case. However, this now raises another question, if
0xffffffff (CLDMA_INVALID_STATUS) means status is invalid, should all
callers both single Q and CLDMA_ALL_Q be returned/check/handle that value?

Why would CLDMA_ALL_Q be special in this respect that the INVALID_STATUS
means invalid only with it?

Reading 0xffffffff is used to detect if the PCI link was disconnected,
it is relevant in t7xx_cldma_qs_are_active() because it is a helper function
polled by t7xx_cldma_stop() to wait until the queues are not active anymore.

I think a cleaner implementation would be to use pci_device_is_present()
instead of the CLDMA_INVALID_STATUS check inside t7xx_cldma_qs_are_active()
and keep t7xx_cldma_hw_queue_status() free of that logic.

...