Re: [PATCH rdma-next v1] RDMA: Explicitly pass in the dma_device to ib_register_device

[Leon Romanovsky <leon@kernel.org>]

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