On Tue, Oct 06, 2020 at 02:59:32AM +0000, Parav Pandit wrote:
>
>
> > 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?
I will change it now and resend.
Thanks
