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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3585 bytes --]

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:
> ...
> > > +#define CLDMA_NUM 2
> > I tried to understand its purpose but it seems that only one of the
> > indexes is used in the arrays where this define gives the size? Related to
> > this, ID_CLDMA0 is not used anywhere?
> 
> The modem HW has 2 CLDMAs, idx 0 for the app processor (SAP) and idx 1 for the
> modem (MD).
> 
> CLDMA_NUM is defined as 2 to reflect the HW capabilities but mainly to have a
> cleaner upcoming
> 
> patches, which will use ID_CLDMA0.

Please note this in your commit message then and I think it should be 
fine to leave it as is (or use 1 sized array, if you prefer to).

> If having array's of size 1 is not a problem then we can define CLDMA_NUM as 1
> and
> 
> play with the CLDMA indexes.
> 
> ...
>
> > > +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?

> > > +/**
> > > + * t7xx_cldma_send_skb() - Send control data to modem.
> > > + * @md_ctrl: CLDMA context structure.
> > > + * @qno: Queue number.
> > > + * @skb: Socket buffer.
> > > + * @blocking: True for blocking operation.
> > > + *
> > > + * Send control packet to modem using a ring buffer.
> > > + * If blocking is set, it will wait for completion.
> > > + *
> > > + * Return:
> > > + * * 0		- Success.
> > > + * * -ENOMEM	- Allocation failure.
> > > + * * -EINVAL	- Invalid queue request.
> > > + * * -EBUSY	- Resource lock failure.
> > > + */
> > > +int t7xx_cldma_send_skb(struct cldma_ctrl *md_ctrl, int qno, struct
> > > sk_buff *skb, bool blocking)
> > > +{
> > > +	struct cldma_request *tx_req;
> > > +	struct cldma_queue *queue;
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	if (qno >= CLDMA_TXQ_NUM)
> > > +		return -EINVAL;
> > > +
> > > +	queue = &md_ctrl->txq[qno];
> > > +
> > > +	spin_lock_irqsave(&md_ctrl->cldma_lock, flags);
> > > +	if (!(md_ctrl->txq_active & BIT(qno))) {
> > > +		ret = -EBUSY;
> > > +		spin_unlock_irqrestore(&md_ctrl->cldma_lock, flags);
> > > +		goto allow_sleep;
> > > +	}
> > ...
> > > +		if (!blocking) {
> > > +			ret = -EBUSY;
> > > +			break;
> > > +		}
> > > +
> > > +		ret = wait_event_interruptible_exclusive(queue->req_wq,
> > > queue->budget > 0);
> > > +	} while (!ret);
> > > +
> > > +allow_sleep:
> > > +	return ret;
> > > +}
> > First of all, if I interpreted the call chains correctly, this function is
> > always called with blocking=true.
> > 
> > Second, the first codepath returning -EBUSY when not txq_active seems
> > twisted/reversed logic to me (not active => busy ?!?).
> 
> What about -EINVAL?
> 
> Other codes considered: -EPERM, -ENETDOWN.

How about -EIO.

-- 
 i.