[net v3] net: wwan: t7xx: Fix napi rx poll issue

[Jinjian Song]

From: Larysa Zaremba <larysa.zaremba at intel.com>

>> Fixes: 5545b7b9f294 ("net: wwan: t7xx: Add NAPI support")
>> Signed-off-by: Jinjian Song <jinjian.song at fibocom.com>
>> ---
>> v3:
>>  * Only Use READ_ONCE/WRITE_ONCE when the lock protecting ctlb->ccmni_inst
>>    is not held.
>
>What do you mean by "lock protecting ctlb->ccmni_inst"? Please specify.

Hi Larysa,

This description might have been a bit simplified. This process is as follow:

In patch v1, I directly set ctlb->ccmni_inst. This may be not safe, as the NAPI
processing and the driver's internal interface might not be synchronized. Therefoe,
following Jakub's suggestion, I add READ_ONCE/WRITE_ONCE in all places where this
pointer is accessed.

In patch v2, Paolo suggested using READ_ONCE in places that are not protected by locks.
Some interfaces are protected by synchronization mechanisms, so it's unnecesssary to add them there.
Therefore, I removed READ_ONCE from the interfaces.

>> @@ -441,7 +442,7 @@ static void t7xx_ccmni_recv_skb(struct t7xx_ccmni_ctrl *ccmni_ctlb, struct sk_bu
>>  
>>  static void t7xx_ccmni_queue_tx_irq_notify(struct t7xx_ccmni_ctrl *ctlb, int qno)
>>  {
>> -	struct t7xx_ccmni *ccmni = ctlb->ccmni_inst[0];
>> +	struct t7xx_ccmni *ccmni = READ_ONCE(ctlb->ccmni_inst[0]);
>>  	struct netdev_queue *net_queue;
>> 
>
>You do not seem to check if ccmni is NULL here, so given ctlb->ccmni_inst[0] is 
>not being hot-swapped, I guess that there are some guarantees of it not being 
>NULL at this moment, so I would drop READ_ONCE here.

This ctlb->ccmni_inst[0] is checked in the upper-level interface:
static void t7xx_ccmni_queue_state_notify([...]) {
	[...]
	if (!READ_ONCE(ctlb->ccmni_inst[0])) {
		return;
	}

	if (state == DMPAIF_TXQ_STATE_IRQ)
		t7xx_ccmni_queue_tx_irq_notify(ctlb, qno);
	else if (state == DMPAIF_TXQ_STATE_FULL)
		t7xx_ccmni_queue_tx_full_notify(ctlb, qno);
}

Since this is part of the driver's internal logic for handing queue events, would it be
safer to add READ_ONCE here as well?

>> @@ -453,7 +454,7 @@ static void t7xx_ccmni_queue_tx_irq_notify(struct t7xx_ccmni_ctrl *ctlb, int qno
>>  
>>  static void t7xx_ccmni_queue_tx_full_notify(struct t7xx_ccmni_ctrl *ctlb, int qno)
>>  {
>> -	struct t7xx_ccmni *ccmni = ctlb->ccmni_inst[0];
>> +	struct t7xx_ccmni *ccmni = READ_ONCE(ctlb->ccmni_inst[0]);
>>  	struct netdev_queue *net_queue;
>>
>
>Same as above, either READ_ONCE is not needed or NULL check is required.

Yes, This function in the same upper-level interface.

>  	if (atomic_read(&ccmni->usage) > 0) {
> @@ -471,7 +472,7 @@ static void t7xx_ccmni_queue_state_notify(struct t7xx_pci_dev *t7xx_dev,
>  	if (ctlb->md_sta != MD_STATE_READY)
>  		return;
>  
> -	if (!ctlb->ccmni_inst[0]) {
> +	if (!READ_ONCE(ctlb->ccmni_inst[0])) {
>  		dev_warn(&t7xx_dev->pdev->dev, "No netdev registered yet\n");
>  		return;
>  	}
> -- 
> 2.34.1
> 
> 

Thanks.

Jinjian,
Best Regards.