> From: Leon Romanovsky <leon@xxxxxxxxxx>
> 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?
