On Thu, 20 Aug 2020, Boris Brezillon wrote:

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

quoted

On Thu, 20 Aug 2020, Miquel Raynal wrote:

quoted

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.

Actually I do have DDEBUG set all the time when testing this code. The 
entire log is captured into a file that I can search for issues when 
they come.

I'm too lazy to bother about dynamic printk for now. That is nice when 
debugging a deployed solution where you don't want to skew runtime 
timings too much, but I'm not at the point of doing performance 
measurements yet.

Anyway, this one has crossed the distraction threshold too at this point 
so I'll remove it.

quoted

quoted

quoted

+	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;

In this case I can agree to that. Will do.

However...

quoted

quoted

quoted

+		/*
+		 * 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. 

Here I disagree. You just can't return and let the system go any longer 
if some stray DMA is not stopped, period. No compromize is possible 
here.

quoted

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.

When/if that happens, it is not very difficult to realign the other 
assignments. I don't think it is likely that adding/removing fields here 
will ever become an area of conflicting patch contention. OTOH this 
makes for easier code reading whose occurence is more likely.

Anyway, that's definitely not a blocker.

Good!

quoted

quoted

quoted

+#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?

Oh nice! Didn't know about that.

Looks like %*ph is what I want here.

quoted

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?

No problem, will do.


Nicolas