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.
