On Mon, Oct 05, 2020 at 02:51:10PM +0100, Christoph Hellwig wrote:
> >
> > -static void setup_dma_device(struct ib_device *device)
> > +static void setup_dma_device(struct ib_device *device,
> > + struct device *dma_device)
> > {
> > + if (!dma_device) {
> > /*
> > + * If the caller does not provide a DMA capable device then the
> > + * IB device will be used. In this case the caller should fully
> > + * setup the ibdev for DMA. This usually means using
> > + * dma_virt_ops.
> > */
> > +#ifdef CONFIG_DMA_OPS
> > + if (WARN_ON(!device->dev.dma_ops))
> > + return;
> > +#endif
>
> Per the discussion last round I think this needs to warn if the ops
> is not dma_virt_ops, or even better force dma_virt_ops here.
>
> Something like:
>
> if (!dma_device) {
> if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_VIRT_OPS)))
> return -EINVAL;
> device->dev.dma_ops = &dma_virt_ops;
>
> > + if (WARN_ON(!device->dev.dma_parms))
> > + return;
>
> I think you either want this check to operate on the dma_device and be
> called for both branches, or removed entirely now that the callers setup
> the dma params.
I would say that all those if(WARN_...) return are too zealous. They
can't be in our subsystem, so it is better to simply delete all if()s
and left blank WARN_ON(..).
Something like that:
if (!dma_device) {
WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_VIRT_OPS))
device->dev.dma_ops = &dma_virt_ops;
....
