Re: [PATCH 2/2] i3c/master: add the mipi-i3c-hci driver

[Nicolas Pitre <nico@fluxnic.net>]

On Thu, 20 Aug 2020, Boris Brezillon wrote:

> On Thu, 20 Aug 2020 12:34:13 -0400 (EDT)
> Nicolas Pitre <nico@fluxnic.net> wrote:
> 
> > On Thu, 20 Aug 2020, Miquel Raynal wrote:
> 
> > > > +
> > > > +#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.

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

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

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

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

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c