On Thu, 20 Aug 2020 12:34:13 -0400 (EDT)
Nicolas Pitre [off-list ref] wrote:

On Thu, 20 Aug 2020, Miquel Raynal wrote:

quoted

quoted

+
+#ccflags-y := -DDEBUG  

Probably a leftover?  

Well, I left it there intentionally as the code is still actively being 
developed, so full debugging can quickly be reactivated by anyone.
I can remove it if deemed too distracting.

How about using dynamic printk instead? I'm pretty sure you don't need
to debug I3C stuff early enough to warrant usage of DDEBUG.

quoted

[...]
  

quoted

+#define CMD_C1_DATA_LENGTH(v)		FIELD_PREP(W1_MASK(63, 48), v)
+#define CMD_C1_OFFSET(v)		FIELD_PREP(W1_MASK(47, 32), v)
+#define CMD_C0_TOC			           W0_BIT_(31)
+#define CMD_C0_ROC			           W0_BIT_(30)
+#define CMD_C0_RNW			           W0_BIT_(29)
+#define CMD_C0_MODE(v)			FIELD_PREP(W0_MASK(28, 26), v)
+#define CMD_C0_16_BIT_SUBOFFSET		W0_bit(25)
+#define CMD_C0_FIRST_PHASE_MODE		           W0_BIT_(24)
+#define CMD_C0_DATA_LENGTH_POSITION(v)	FIELD_PREP(W0_MASK(23, 22), v)
+#define CMD_C0_DEV_INDEX(v)		FIELD_PREP(W0_MASK(20, 16), v)
+#define CMD_C0_CP			           W0_BIT_(15)
+#define CMD_C0_CMD(v)			FIELD_PREP(W0_MASK(14, 7), v)
+#define CMD_C0_TID(v)			FIELD_PREP(W0_MASK(6, 3), v)
+
+/*
+ * Internal Control Command
+ */
+
+#define CMD_0_ATTR_M			FIELD_PREP(CMD_0_ATTR, 0x7)
+
+#define CMD_M1_VENDOR_SPECIFIC		           W1_MASK(63, 32)
+#define CMD_M0_MIPI_RESERVED		           W0_MASK(31, 12)
+#define CMD_M0_MIPI_CMD			           W0_MASK(11, 8)
+#define CMD_M0_VENDOR_INFO_PRESENT	           W0_BIT_(7)
+#define CMD_M0_TID(v)			FIELD_PREP(W0_MASK(6, 3), v)
+
+
+static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
+			       struct hci_xfer *xfer,
+			       u8 ccc_addr, u8 ccc_cmd, bool raw)
+{
+	u_int dat_idx = 0;  

I guess u_int her and below is not the preferred way to declare an unsigned int?  

Why not?

quoted

quoted

+	int mode = hci_get_i3c_mode(hci);
+	u8 *data = xfer->data;
+	u_int data_len = xfer->data_len;
+	bool rnw = xfer->rnw;
+	int ret;
+
+	BUG_ON(raw);  

It looks like 'raw' cannot be used with v1 (at least you seem to take
care of it in v2), so maybe BUG_ON is a bit radical here and you can
simply return an error? I think the use of BUG() is not appreciated in
general.  

That depends. Judgement is needed for BUG() usage.

Here raw is absolutely impossible with v1 hardware and if ever this 
happens this is definitely a software bug that needs fixing right away. 
There is no point returning a runtime error code in that case as the 
upper layer won't know what to do about it.

On the other hand, you absolutely don't want to BUG() on a condition 
that could _eventually_ happen at run time during normal usage. But 
that's not the case here.

Well, people have tried to eradicate BUG() occurrences, so let's not add
new ones if we can avoid it. How about a WARN_ON()+error:

	if (WARN_ON(raw))
		return -EINVAL;

quoted

quoted

+const struct hci_cmd_ops i3c_hci_cmd_v1 = {
+	.prep_ccc		= hci_cmd_v1_prep_ccc,
+	.prep_i3c_xfer		= hci_cmd_v1_prep_i3c_xfer,
+	.prep_i2c_xfer		= hci_cmd_v1_prep_i2c_xfer,
+	.perform_daa		= hci_cmd_v1_daa,  

I know Boris does not like such space alignment :)  

Well... unfortunately for Boris, this is overwhelmingly prevalent in the 
kernel code:

$ git grep "^"$'\t'"\.[^ ]*"$'\t'"*= "

And I do like it.  ;-P

The rational being this preference is that sooner or later someone will
add a field to hci_cmd_ops that messes up your nice formatting :P.
Anyway, that's definitely not a blocker.

quoted

quoted

+void i3c_hci_pio_reset(struct i3c_hci *hci)
+{
+	reg_write(RESET_CONTROL, RX_FIFO_RST|TX_FIFO_RST|RESP_QUEUE_RST);  

Style with missing spaces                  ^ ^  

True. Will fix.

quoted

quoted

+static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
+				struct i3c_ccc_cmd *ccc)
+{
+	struct i3c_hci *hci = to_i3c_hci(m);
+	struct hci_xfer *xfer;
+	bool raw = !!(hci->quirks & HCI_QUIRK_RAW_CCC);
+	bool prefixed = raw && !!(ccc->id & I3C_CCC_DIRECT);
+	u_int nxfers = ccc->ndests + prefixed;
+	DECLARE_COMPLETION_ONSTACK(done);
+	int i, last, ret = 0;
+
+	DBG("cmd=%#x rnw=%d ndests=%d data[0].len=%d",
+	    ccc->id, ccc->rnw, ccc->ndests, ccc->dests[0].payload.len);
+
+	xfer = hci_alloc_xfer(nxfers);
+	if (!xfer)
+		return -ENOMEM;
+
+	if (prefixed) {
+		xfer->data = NULL;
+		xfer->data_len = 0;
+		xfer->rnw = false;
+		hci->cmd->prep_ccc(hci, xfer, I3C_BROADCAST_ADDR,
+				   ccc->id, true);
+		xfer++;
+	}
+
+	for (i = 0; i < nxfers - prefixed; i++) {
+		xfer[i].data = ccc->dests[i].payload.data;
+		xfer[i].data_len = ccc->dests[i].payload.len;
+		xfer[i].rnw = ccc->rnw;
+		ret = hci->cmd->prep_ccc(hci, &xfer[i], ccc->dests[i].addr,
+					 ccc->id, raw);
+		if (ret)
+			goto out;
+		xfer[i].cmd_desc[0] |= CMD_0_ROC;
+	}
+	last = i - 1;
+	xfer[last].cmd_desc[0] |= CMD_0_TOC;
+	xfer[last].completion = &done;
+
+	if (prefixed)
+		xfer--;
+
+	ret = hci->io->queue_xfer(hci, xfer, nxfers);
+	if (ret)
+		goto out;
+	if (!wait_for_completion_timeout(&done, HZ) &&
+	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
+		ret = -ETIME;
+		goto out;
+	}
+	for (i = prefixed; i < nxfers; i++) {
+		if (ccc->rnw)
+			ccc->dests[i - prefixed].payload.len =
+				RESP_DATA_LENGTH(xfer[i].response);
+		if (RESP_STATUS(xfer[i].response) != RESP_SUCCESS) {
+			ret = -EIO;
+			goto out;
+		}
+	}
+
+#if 0
+	if (ccc->rnw) {
+		HEXDUMP("got: ", ccc->dests[0].payload.data,
+				 ccc->dests[0].payload.len);
+	}
+#endif  

I guess this debug block can be dropped too (there are many debug
information the should probably be dropped or turned into dev_info()
or similar).  

Again, hardware bringup from different vendors and other developments 
are still ongoing. I'd wish for those to stay for the time being unless 
people feel strongly enough about these to become a merge show stopper.

Can't we replace that by a dev_dbg() using the %*pE formater?

quoted

quoted

+/*
+ * Paul Kimelman's debug trace log facility.
+ * This is completely vendor and hardware specific.
+ */
+void __PK_debug_trace(struct i3c_hci *hci, const char *func)
+{
+	void __iomem *trcp = (void __iomem *)hci->vendor_data + 7*4;  

Maybe you need to define what is 7*4 , 0*4 below, v >> 27, etc

Also there are many missing spaces between operators (7 * 4,w & (1 <<9).  

I think in this case this is really crossing the distraction threshold. 
This is used to extract debugging traces out of a specific FPGA 
implementation and is of no use to anyone else. I'll just rip that out 
from the next submission.

quoted

quoted

+		if (rh->ibi_data_phys)  

I was told that _phys was a very bad suffix for something which is a
DMA address an not focibly a physical address.  

Fair enough. The HCI spec refers to these as "physical memory" hence the 
suffix. What were you told to use instead?

Maybe _dma instead of _phys?

quoted

quoted

+static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
+				 struct hci_xfer *xfer_list, int n)
+{
+	struct hci_rings_data *rings = hci->io_data;
+	struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring];
+	u_int i;
+	bool did_unqueue = false;
+
+	/* stop the ring */
+	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
+	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
+		/*
+		 * We're deep in it if ever this condition is ever met.
+		 * Hardware might still be writing to memory, etc.
+		 */
+		ERR("unable to abort the ring");
+		BUG();  

Why not just treating the error as always?  

Again, if this ever happens, you're screwed. That means potential DMA 
engines could still be alive and about to scribble over memory that is 
about to be freed which may cause all sorts of impossible-to-find bugs 
in unrelated parts of the kernel. There is no point going on reporting 
such error condition to upper layers until the software, or possibly the 
hardware, is fixed

Again, I think adding a WARN_ON() and letting hci_dma_dequeue_xfer()
return an error code is a good compromise. 

quoted

quoted

+struct hci_xfer {
+	u32 cmd_desc[4];
+	u32 response;
+	bool rnw;
+	void *data;
+	u_int data_len;
+	u_int cmd_tid;
+	struct completion *completion;
+	union {
+		struct {
+			/* PIO specific */
+			struct hci_xfer *next_xfer;
+			struct hci_xfer *next_data;
+			struct hci_xfer *next_resp;
+			u_int data_left;
+			u32 data_word_before_partial;
+		};  

I think anonymous unions are prohibited because the kernel is supposed
to be built with old gcc versions which do not support it.  

Hmmm... let's see:

According to Documentation/process/changes.rst the current minimal 
required gcc version is 4.9.

The oldest gcc I have lying around is 4.5.1 and it supports anonymous 
unions just fine.

So I think we're clear.

We had problems with a buggy version of gcc 4.<something> at some
point, and that was only when initializing anonymous unions, but I
think it's all behind us now.