Re: [bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Nov 18, 2019 at 12:25:14PM +0800, Green Wan wrote:
> Hi Dan,
> 
> Thanks for reaching out. I do have the same question before. If I
> remember it correctly, the 'chan' is returned by to_sf_pdma_chan()
> which contains 'dchan'. 'chan' s unlikely to be NULL since it's
> container.

to_sf_pdma_chan() is really a no-op when you read it carefully.  "chan"
and "dchan" are equivalent.  Smatch parses it correctly.

You are obviously correct that people should check the original "dchan"
instead of the returned "chan", but I feel like some people are clever
enough to know when container_of() is a no-op and deliberately check the
returned value...  These are sorts of people you don't want to get into
a debate with because they're an ultra annoying blend of clever and
dumb.

My other comment here is that Smatch doesn't warn about inconsistent
NULL checking when the pointer is provably non-NULL.  In this case, the
only caller that Smatch doesn't parse correctly is ioat_dma_self_test().
For all the other callers it knows that "dchan" is non-NULL.  In
ioat_dma_self_test() the code looks like:

   330          /* Start copy, using first DMA channel */
   331          dma_chan = container_of(dma->channels.next, struct dma_chan,
   332                                  device_node);

Smatch says that both "dma->channels.next" and "dma_chan" are complete
unknowns.

I could probably safely mark dma_chan as a valid pointer in this
instance which would make the warning disappear...  Maybe I will add a
check to see if "dma->channels.next" can be controlled by the user.

regards,
dan carpenter