Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210201183017.3339130-1-jxgao@google.com> <20210201183017.3339130-4-jxgao@google.com>
 <YBhOrK+kea7PNmXn@smile.fi.intel.com>
In-Reply-To: <YBhOrK+kea7PNmXn@smile.fi.intel.com>
From:   Jianxiong Gao <jxgao@google.com>
Date:   Mon, 1 Feb 2021 11:35:52 -0800
Message-ID: <CAMGD6P0qc0FZagmimTCuYsbJXo-WBHjtgoWSRzs63EULybTzNg@mail.gmail.com>
Subject: Re: [PATCH V2 3/3] Adding device_dma_parameters->offset_preserve_mask
 to NVMe driver.
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Erdem Aktas <erdemaktas@google.com>, Marc Orr <marcorr@google.com>,
        Christoph Hellwig <hch@lst.de>, m.szyprowski@samsung.com,
        Robin Murphy <robin.murphy@arm.com>,
        gregkh@linuxfoundation.org, Saravana Kannan <saravanak@google.com>,
        heikki.krogerus@linux.intel.com, rafael.j.wysocki@intel.com,
        dan.j.williams@intel.com, bgolaszewski@baylibre.com,
        jroedel@suse.de, iommu@lists.linux-foundation.org,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Keith Busch <kbusch@kernel.org>, axboe@fb.com,
        sagi@grimberg.me, linux-nvme@lists.infradead.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 1, 2021 at 10:56 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 01, 2021 at 10:30:17AM -0800, Jianxiong Gao wrote:
> > NVMe driver relies on the address offset to function properly.
> > This patch adds the offset preserve mask to NVMe driver when mapping
> > via dma_map_sg_attrs and unmapping via nvme_unmap_sg. The mask
> > depends on the page size defined by CC.MPS register of NVMe
> > controller.
>
> ...
>
> >       if (is_pci_p2pdma_page(sg_page(iod->sg)))
> >               nr_mapped = pci_p2pdma_map_sg_attrs(dev->dev, iod->sg,
> >                               iod->nents, rq_dma_dir(req), DMA_ATTR_NO_WARN);
> >       else
> >               nr_mapped = dma_map_sg_attrs(dev->dev, iod->sg, iod->nents,
> >                                            rq_dma_dir(req), DMA_ATTR_NO_WARN);
> > +
> > +     offset_ret = dma_set_min_align_mask(dev->dev, 0);
> > +     if (offset_ret) {
> > +             dev_warn(dev->dev, "dma_set_min_align_mask failed to reset offset\n");
> > +             goto out_free_sg;
> > +     }
>
> Seems like rebasing effect which makes empty line goes in the middle of some
> other group of code lines.
>
> >       if (!nr_mapped)
> >               goto out_free_sg;
>
> Perhaps it should be here?
Yes you are correct, it should be

     else
              nr_mapped = dma_map_sg_attrs(dev->dev, iod->sg, iod->nents,
                                           rq_dma_dir(req), DMA_ATTR_NO_WARN);
      if (!nr_mapped)
              goto out_free_sg;
+
+     offset_ret = dma_set_min_align_mask(dev->dev, 0);
+     if (offset_ret) {
+             dev_warn(dev->dev, "dma_set_min_align_mask failed to
reset offset\n");
+             goto out_free_sg;
+     }

Thanks for pointing it out.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Jianxiong Gao