Date:   Tue, 17 Oct 2017 23:15:00 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     srinivas.kandagatla@...aro.org
Cc:     gregkh@...uxfoundation.org, broonie@...nel.org,
        alsa-devel@...a-project.org, sdharia@...eaurora.org, bp@...e.de,
        poeschel@...onage.de, treding@...dia.com,
        gong.chen@...ux.intel.com, andreas.noever@...il.com,
        alan@...ux.intel.com, mathieu.poirier@...aro.org, daniel@...ll.ch,
        jkosina@...e.cz, sharon.dvir1@...l.huji.ac.il, joe@...ches.com,
        davem@...emloft.net, james.hogan@...tec.com,
        michael.opdenacker@...e-electrons.com, robh+dt@...nel.org,
        pawel.moll@....com, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kheitke@...ience.com, linux-arm-msm@...r.kernel.org, arnd@...db.de
Subject: Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@...aro.org wrote:

> From: Sagar Dharia <sdharia@...eaurora.org>
> 
> Slimbus devices use value-element, and information elements to
> control device parameters (e.g. value element is used to represent
> gain for codec, information element is used to represent interrupt
> status for codec when codec interrupt fires).
> Messaging APIs are used to set/get these value and information
> elements. Slimbus specification uses 8-bit "transaction IDs" for
> messages where a read-value is anticipated. Framework uses a table
> of pointers to store those TIDs and responds back to the caller in
> O(1).

I think we can implement this "optimization" with less complex code,
regardless I don't think we need to mention this in the commit
message...

[..]
> diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c
[..]
> +/**
> + * slim_msg_response: Deliver Message response received from a device to the
> + *	framework.
> + * @ctrl: Controller handle
> + * @reply: Reply received from the device
> + * @len: Length of the reply
> + * @tid: Transaction ID received with which framework can associate reply.
> + * Called by controller to inform framework about the response received.
> + * This helps in making the API asynchronous, and controller-driver doesn't need
> + * to manage 1 more table other than the one managed by framework mapping TID
> + * with buffers
> + */
> +void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)

Even if tid and len comes from the spec I recommend you making them int
and size_t.

> +{
> +	struct slim_val_inf *msg;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->txn_lock, flags);
> +	msg = ctrl->tid_tbl[tid];
> +	if (msg == NULL || msg->rbuf == NULL) {

if (!msg || !msg->rbuf)


When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL?
Should we reject it earlier?

> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +		dev_err(&ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
> +				tid, len);
> +		return;
> +	}
> +	ctrl->tid_tbl[tid] = NULL;
> +	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +
> +	memcpy(msg->rbuf, reply, len);
> +	if (msg->comp_cb)
> +		msg->comp_cb(msg->ctx, 0);
> +}
> +EXPORT_SYMBOL_GPL(slim_msg_response);
[..]
> +int slim_processtxn(struct slim_controller *ctrl,
> +				struct slim_msg_txn *txn)
> +{
> +	int ret, i = 0;
> +	unsigned long flags;
> +	u8 *buf;
> +	bool async = false;
> +	struct slim_cb_data cbd;
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
> +
> +	if (!txn->msg->comp_cb) {
> +		txn->msg->comp_cb = slim_sync_default_cb;
> +		cbd.comp = &done;
> +		txn->msg->ctx = &cbd;
> +	} else {
> +		async = true;
> +	}
> +
> +	buf = slim_get_tx(ctrl, txn, need_tid);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		for (i = 0; i < ctrl->last_tid; i++) {
> +			if (ctrl->tid_tbl[i] == NULL)
> +				break;
> +		}
> +		if (i >= ctrl->last_tid) {
> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +				slim_return_tx(ctrl, -ENOMEM);
> +				return -ENOMEM;
> +			}
> +			ctrl->last_tid++;
> +		}
> +		ctrl->tid_tbl[i] = txn->msg;
> +		txn->tid = i;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +
> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
> +
> +	if (!ret && !async) { /* sync transaction */
> +		/* Fine-tune calculation after bandwidth management */
> +		unsigned long ms = txn->rl + 100;
> +
> +		ret = wait_for_completion_timeout(&done,
> +						  msecs_to_jiffies(ms));
> +		if (!ret)
> +			slim_return_tx(ctrl, -ETIMEDOUT);
> +
> +		ret = cbd.ret;
> +	}
> +
> +	if (ret && need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		/* Invalidate the transaction */
> +		ctrl->tid_tbl[txn->tid] = NULL;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +	if (ret)
> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
> +			txn->mt, txn->mc, txn->la, ret);

if (ret) {
	if (need_tid)
		drop();
	
	dev_err();
}

Would probably make this a little bit cleaner...

> +	if (!async) {
> +		txn->msg->comp_cb = NULL;
> +		txn->msg->ctx = NULL;

I believe txn->msg is always required, so you don't need to do this
contidionally.

> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_processtxn);
[..]
> +int slim_request_val_element(struct slim_device *sb,
> +				struct slim_val_inf *msg)
> +{
> +	struct slim_controller *ctrl = sb->ctrl;
> +
> +	if (!ctrl)
> +		return -EINVAL;

>From patch 1 I believe it's invalid for sb->ctrl to be NULL, so there
shouldn't be a need to check this.

> +
> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
> +}
> +EXPORT_SYMBOL_GPL(slim_request_val_element);
[..]
> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if (ctrl->rx.tail == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		return -ENODATA;
> +	}
> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
> +				ctrl->rx.sl_sz);
> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_return_rx);
> +

Please provide kerneldoc for exported symbols.

> +void slim_return_tx(struct slim_controller *ctrl, int err)
> +{
> +	unsigned long flags;
> +	int idx;
> +	struct slim_pending cur;
> +
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +	idx = ctrl->tx.head;
> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
> +	cur = ctrl->pending_wr[idx];

Why is this doing struct copy?

> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	if (!cur.cb)
> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
> +	else
> +		cur.cb(cur.ctx, err);
> +
> +	up(&ctrl->tx_sem);
> +}
> +EXPORT_SYMBOL_GPL(slim_return_tx);
[..]
>  /**
> + * struct slim_val_inf: Slimbus value or information element
> + * @start_offset: Specifies starting offset in information/value element map
> + * @num_bytes: upto 16. This ensures that the message will fit the slicesize
> + *		per slimbus spec
> + * @comp_cb: Callback if this read/write is asynchronous
> + * @ctx: Argument for comp_cb
> + */
> +struct slim_val_inf {
> +	u16			start_offset;
> +	u8			num_bytes;
> +	u8			*rbuf;

This is not mentioned in the kerneldoc. Use void * for data buffers.

> +	const u8		*wbuf;

Can a message ever be read and write? Otherwise it should be sufficient
to only have one data pointer.

> +	void			(*comp_cb)(void *ctx, int err);
> +	void			*ctx;
> +};
> +
[..]
> +/**
> + * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
> + * @base: virtual base address for this buffer
> + * @phy: physical address for this buffer (this is useful if controller can
> + *	  DMA the buffers for TX and RX to/from controller hardware
> + * @lock: lock protecting head and tail
> + * @head: index where buffer is returned back
> + * @tail: index from where buffer is consumed
> + * @sl_sz: byte-size of each slot in this buffer
> + * @n:	  number of elements in this circular ring, note that this needs to be
> + *	1 more than actual buffers to allow for one open slot
> + */

Is this ringbuffer mechanism defined in the slimbus specification? Looks
like something specific to the Qualcomm controller, rather than
something that should be enforced in the framework.

> +struct slim_ctrl_buf {
> +	void		*base;
> +	phys_addr_t	phy;
> +	spinlock_t	lock;
> +	int		head;
> +	int		tail;
> +	int		sl_sz;
> +	int		n;
> +};
[..]
> +/**
>   * struct slim_controller: Controls every instance of SLIMbus
>   *				(similar to 'master' on SPI)
>   *	'Manager device' is responsible for  device management, bandwidth
> @@ -139,6 +246,16 @@ struct slim_addrt {
>   * @addrt: Logical address table
>   * @num_dev: Number of active slimbus slaves on this bus
>   * @wq: Workqueue per controller used to notify devices when they report present
> + * @tid_tbl: Table of transactions having transaction ID
> + * @txn_lock: Lock to protect table of transactions
> + * @rx: RX buffers used by controller to receive messages. Ctrl may receive more
> + *	than 1 message (e.g. multiple report-present messages or messages from
> + *	multiple slaves).
> + * @tx: TX buffers used by controller to transmit messages. Ctrl may have
> + *	ability to send/queue multiple messages to HW at once.
> + * @pending_wr: Pending write transactions to be acknowledged by controller

This is out list of pending write requests, yet it's implemented as an
array used in a complex ring buffer fashion. Wouldn't it be easier to
just have this as a linked list of slim_pending struct?

> + * @tx_sem: Semaphore for available TX buffers for this controller
> + * @last_tid: Last used entry for TID transactions
>   * @xfer_msg: Transfer a message on this controller (this can be a broadcast
>   *	control/status message like data channel setup, or a unicast message
>   *	like value element read/write.
> @@ -161,6 +278,15 @@ struct slim_controller {
>  	struct slim_addrt	*addrt;
>  	u8			num_dev;
>  	struct workqueue_struct *wq;
> +	struct slim_val_inf	*tid_tbl[SLIM_MAX_TIDS];
> +	u8			last_tid;

I suggest that you replace these two with an idr, rather than having a
fixed size array and then last_tid as an optimization to limit how far
you linear search for an empty space.

> +	spinlock_t		txn_lock;
> +	struct slim_ctrl_buf	tx;
> +	struct slim_ctrl_buf	rx;
> +	struct slim_pending	*pending_wr;
> +	struct semaphore	tx_sem;

Please don't use semaphores. If you keep pending_wr as a list you can
use list_empty() instead...

> +	int			(*xfer_msg)(struct slim_controller *ctrl,
> +					    struct slim_msg_txn *tx, void *buf);

I believe buf has fixed size, so please document this.

>  	int			(*set_laddr)(struct slim_controller *ctrl,
>  					     struct slim_eaddr *ea, u8 laddr);
>  	int			(*get_laddr)(struct slim_controller *ctrl,
> @@ -295,5 +421,40 @@ static inline void slim_set_devicedata(struct slim_device *dev, void *data)
>  {
>  	dev_set_drvdata(&dev->dev, data);
>  }

Regards,
Bjorn

Powered by blists - more mailing lists