Hi Dan, Thanks for the comment. I'd like to do check and fix it. And to be honest, I don't use smatch tool to do static analysis. Looks interesting. Let me enable it to see how it helps/works. Thanks a lot. On Mon, Nov 18, 2019 at 3:36 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > 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 >
