> Subject: Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly
>
> On 10/21/19 7:10 AM, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 07:10:28PM -0700, Bart Van Assche wrote:
> >> The effect of the dma_set_max_seg_size() call in setup_dma_device()
> >> is as follows:
> >> - If device->dev.dma_parms is NULL, that call has no effect at all.
> >> - If device->dev.dma_parms is not NULL, since that pointer points at
> >> the DMA parameters of the parent device, modify the DMA limits of the
> >> parent device.
> >>
> >> Both actions are wrong. Instead of changing the DMA parameters of the
> >> parent device, use the DMA parameters of the parent device if these
> >> parameters are available.
> >>
> >> Compile-tested only.
> >>
> >> Cc: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> >> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> >> Cc: Adit Ranadive <aditr@xxxxxxxxxx>
> >> Cc: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
> >> Cc: Gal Pressman <galpress@xxxxxxxxxx>
> >> Cc: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
> >> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions
> >> in SGEs")
> >> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> >> drivers/infiniband/core/device.c | 13 +++++++++++--
> >> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/device.c
> >> b/drivers/infiniband/core/device.c
> >> index 536310fb6510..b33d684a2a99 100644
> >> +++ b/drivers/infiniband/core/device.c
> >> @@ -1199,9 +1199,18 @@ static void setup_dma_device(struct ib_device
> *device)
> >> WARN_ON_ONCE(!parent);
> >> device->dma_device = parent;
> >> }
> >> - /* Setup default max segment size for all IB devices */
> >> - dma_set_max_seg_size(device->dma_device, SZ_2G);
> >>
> >> + if (!device->dev.dma_parms) {
> >> + if (parent) {
> >> + /*
> >> + * The caller did not provide DMA parameters. Use the
> >> + * DMA parameters of the parent device.
> >> + */
> >> + device->dev.dma_parms = parent->dma_parms;
> >> + } else {
> >> + WARN_ON_ONCE(true);
> >> + }
> >> + }
> >> }
> >
> > We really do want to, by default, increase the max_seg_size above 64k
> > though, so this approach doesn't seem right either.
>
> How about replacing this patch by the patch below and by moving this patch to
> the end of this series?
>
> Thanks,
>
> Bart.
>
>
> Subject: [PATCH] RDMA/core: Set DMA parameters correctly
>
> The dma_set_max_seg_size() call in setup_dma_device() does not have any effect
> since device->dev.dma_parms is NULL. Fix this by initializing
> device->dev.dma_parms first.
>
> Cc: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: Adit Ranadive <aditr@xxxxxxxxxx>
> Cc: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
> Cc: Gal Pressman <galpress@xxxxxxxxxx>
> Cc: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
> SGEs")
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/infiniband/core/device.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a667636f74bf..a523d844ad9d 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1199,9 +1199,21 @@ static void setup_dma_device(struct ib_device *device)
> WARN_ON_ONCE(!parent);
> device->dma_device = parent;
> }
> - /* Setup default max segment size for all IB devices */
> - dma_set_max_seg_size(device->dma_device, SZ_2G);
>
> + if (!device->dev.dma_parms) {
> + if (parent) {
> + /*
> + * The caller did not provide DMA parameters, so
> + * 'parent' probably represents a PCI device. The PCI
> + * core sets the maximum segment size to 64
> + * KB. Increase this parameter to 2G.
> + */
> + device->dev.dma_parms = parent->dma_parms;
> + dma_set_max_seg_size(device->dma_device, SZ_2G);
Did you mean dma_set_max_seg_size(&device->dev, SZ_2G)?
device->dma_device could be pointing to parent if the caller
did not provide dma_ops. So wont this update the parent device
dma params?
Also do we want to ensure all callers device max_seg_sz
params >= threshold (=2G)? If so, perhaps we can do something
similar to vb2_dma_contig_set_max_seg_size()
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/media/common/videobuf2/videobuf2-dma-contig.c#L734
