Hi Jon,
On Fri, Jul 13, 2012 at 5:44 PM, Jon Mason <jon.mason@xxxxxxxxx> wrote:
Just a few minor comments/questions:
....
> +struct ntb_transport_qp {
> + struct ntb_device *ndev;
> +
> + bool client_ready;
> + bool qp_link;
> + u8 qp_num; /* Only 64 QP's are allowed. 0-63 */
> +
> + void (*tx_handler) (struct ntb_transport_qp *qp);
> + struct tasklet_struct tx_work;
Is it ok to rename the following vars for convenience sake?
> + struct list_head txq;
tx_pend_q - (pending_queue) or tx_out_q - (outstanding_queue) - or
pick any new string you like - other than a mono-syllable definition
> + struct list_head txc;
tx_compl_q - completion queue
> + struct list_head txe;
tx_avail_e - available entry queue
> + spinlock_t txq_lock;
> + spinlock_t txc_lock;
> + spinlock_t txe_lock;
then match the variants accordingly.
> + struct list_head rxq;
> + struct list_head rxc;
> + struct list_head rxe;
> + spinlock_t rxq_lock;
> + spinlock_t rxc_lock;
> + spinlock_t rxe_lock;
similarly the rx-counterpart
..................
> +static void ntb_tx_copy_task(struct ntb_transport_qp *qp,
> + struct ntb_queue_entry *entry,
> + void *offset)
> +{
> + struct ntb_payload_header *hdr = offset;
> + int rc;
> +
> + offset += sizeof(struct ntb_payload_header);
> + memcpy_toio(offset, entry->buf, entry->len);
> +
> + hdr->len = entry->len;
> + hdr->ver = qp->tx_pkts;
> +
> + /* Ensure that the data is fully copied out before setting the flag */
> + wmb();
> + hdr->flags = entry->flags | DESC_DONE_FLAG;
> +
> + rc = ntb_ring_sdb(qp->ndev, qp->qp_num);
> + if (rc)
> + pr_err("%s: error ringing db %d\n", __func__, qp->qp_num);
> +
> + if (entry->len > 0) {
how do you interpret this len variable and decide if it's a good/bad completion?
> + qp->tx_bytes += entry->len;
> +
> + /* Add fully transmitted data to completion queue */
> + ntb_list_add_tail(&qp->txc_lock, &entry->entry, &qp->txc);
> +
> + if (qp->tx_handler)
> + qp->tx_handler(qp);
> + } else
> + ntb_list_add_tail(&qp->txe_lock, &entry->entry, &qp->txe);
I could be wrong but how is the original skb handled if the code path
goes in the else clause?
Also, in the else clause, how about a ntb_list_add_head rather than a _tail.
> +
> +static int ntb_process_tx(struct ntb_transport_qp *qp,
> + struct ntb_queue_entry *entry)
> +{
> + struct ntb_payload_header *hdr;
> + void *offset;
> +
> + offset = qp->tx_offset;
> + hdr = offset;
> +
> + pr_debug("%lld - offset %p, tx %p, entry len %d flags %x buff %p\n",
> + qp->tx_pkts, offset, qp->tx_offset, entry->len, entry->flags,
> + entry->buf);
> + if (hdr->flags) {
> + ntb_list_add_head(&qp->txq_lock, &entry->entry, &qp->txq);
> + qp->tx_ring_full++;
> + return -EAGAIN;
> + }
> +
> + if (entry->len > transport_mtu) {
> + pr_err("Trying to send pkt size of %d\n", entry->len);
> + entry->flags = HW_ERROR_FLAG;
> +
> + ntb_list_add_tail(&qp->txc_lock, &entry->entry, &qp->txc);
> +
> + if (qp->tx_handler)
> + qp->tx_handler(qp);
> +
> + return 0;
> + }
> +
> + ntb_tx_copy_task(qp, entry, offset);
what happens when ntb_sdb_ring returns an error? would you still want
to increment tx_pkts below?
> +
> + qp->tx_offset =
> + (qp->tx_offset +
> + ((transport_mtu + sizeof(struct ntb_payload_header)) * 2) >=
> + qp->tx_mw_end) ? qp->tx_mw_begin : qp->tx_offset + transport_mtu +
> + sizeof(struct ntb_payload_header);
> +
> + qp->tx_pkts++;
> +
> + return 0;
> +}
> +
........................
> +void *ntb_transport_tx_dequeue(struct ntb_transport_qp *qp, unsigned int *len)
> +{
> + struct ntb_queue_entry *entry;
> + void *buf;
> +
> + if (!qp)
> + return NULL;
> +
> + entry = ntb_list_rm_head(&qp->txc_lock, &qp->txc);
> + if (!entry)
> + return NULL;
> +
> + buf = entry->callback_data;
> + if (entry->flags != HW_ERROR_FLAG)
> + *len = entry->len;
> + else
> + *len = -EIO;
> +
> + ntb_list_add_tail(&qp->txe_lock, &entry->entry, &qp->txe);
how about a ntb_list_add_head?
Chetan Loke
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
