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

["Martinez, Ricardo" <ricardo.martinez@linux.intel.com>]

On 1/19/2022 1:52 AM, Ilpo Järvinen wrote:
> On Tue, 18 Jan 2022, Martinez, Ricardo wrote:
>
>> On 1/18/2022 6:13 AM, Ilpo Järvinen wrote:
>>> On Thu, 13 Jan 2022, Ricardo Martinez wrote:
...
>>> +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.

...