Re: dma_alloc_coherent bug in latest ntb_tool changes

[Serge Semin <fancer.lancer@gmail.com>]

On Thu, Jun 07, 2018 at 06:33:48PM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> 
> 
> On 07/06/18 06:24 PM, Serge Semin wrote:
> > You also said:
> >>> It seems cleaner to use the NTB device but then we at least have to set the
> >>> coherent_mask to something sane ...
> > It doesn't seem clearer now?
> 
> It still seems cleaner... But I'm not convinced it's safe. I'm on the
> fence really.
> 
> > Only hardware driver knows what dma address range is supported by device. 
> 
> Well given that all the hardware drivers only copy what's set in the pci
> device I don't think that applies.
> 
> > Regarding the ntb_register_device(). Since all the currently supported NTB devices
> > are PCIe-based, we can temporarily move the coherency mask setting to ntb.c. But
> > NTB API is still an abstraction, so I wouldn't do this, since there might be
> > non-PCIe-based devices.
> 
> NTB is a long way from being non-pci based. Especially seeing it's a PCI
> standard. If and when that day comes where someone wants to create some
> non-PCI NTB we'd probably have an ntb_register for pci devices and one
> for other devices -- but then it's that person's problem. The general
> rule of thumb for kernel development is not to write code for an
> imagined future but to write good code for now.
> > You don't have to:
> > https://github.com/torvalds/linux/commit/417cf39cfea9c680aa7c278c8d8a0ca879cacf0a#diff-4cc577115709fbd1fb24e8803f9d629d
> 
> That's the commit that fixed an issue. It didn't cause the issue or even
> talk about the change that would break it. Nor did it come up when a
> bisect was done. That's why changes should be done in small chunks. It
> would be different if there was a commit that made the change from pdevs
> to ntb devs right after this commit explaining the consequences and that
> it was fixed in the previous commit.
> 
> Logan
> 

You saw the commit, lets halt the lecture about thumbs and chunks. Topic is about
the DMA mask setting. I still think, that it's more logical to place the
masks initialization to the hardware drivers code, since it's the hardware-specific
setting. I am also convinced, that we should proper set NTB device masks, so they
would correspond to the real NTB devices hidden behind the API. That's it.

-Sergey