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

[Leon Romanovsky <leon@kernel.org>]

On Tue, Oct 06, 2020 at 02:59:32AM +0000, Parav Pandit wrote:
>
>
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Monday, October 5, 2020 8:28 PM
> >
> > 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;
> >       ....
> Looks good to me.
> Will you revise or I should?

I will change it now and resend.

Thanks