[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a75f5bce-8ab5-7d99-bb27-908d0cc7c6f6@deltatee.com>
Date: Wed, 21 Mar 2018 10:52:25 -0600
From: Logan Gunthorpe <logang@...tatee.com>
To: Christoph Hellwig <hch@....de>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-nvme@...ts.infradead.org, linux-rdma@...r.kernel.org,
linux-nvdimm@...ts.01.org, linux-block@...r.kernel.org,
Stephen Bates <sbates@...thlin.com>,
Jens Axboe <axboe@...nel.dk>,
Keith Busch <keith.busch@...el.com>,
Sagi Grimberg <sagi@...mberg.me>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Jason Gunthorpe <jgg@...lanox.com>,
Max Gurtovoy <maxg@...lanox.com>,
Dan Williams <dan.j.williams@...el.com>,
Jérôme Glisse <jglisse@...hat.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Alex Williamson <alex.williamson@...hat.com>,
Steve Wise <swise@...ngridcomputing.com>
Subject: Re: [PATCH v3 11/11] nvmet: Optionally use PCI P2P memory
On 21/03/18 03:27 AM, Christoph Hellwig wrote:
>> + const char *page, size_t count)
>> +{
>> + struct nvmet_port *port = to_nvmet_port(item);
>> + struct device *dev;
>> + struct pci_dev *p2p_dev = NULL;
>> + bool use_p2pmem;
>> +
>> + switch (page[0]) {
>> + case 'y':
>> + case 'Y':
>> + case 'a':
>> + case 'A':
>> + use_p2pmem = true;
>> + break;
>> + case 'n':
>> + case 'N':
>> + use_p2pmem = false;
>> + break;
>> + default:
>> + dev = bus_find_device_by_name(&pci_bus_type, NULL, page);
>> + if (!dev) {
>> + pr_err("No such PCI device: %s\n", page);
>> + return -ENODEV;
>> + }
>> +
>> + use_p2pmem = true;
>> + p2p_dev = to_pci_dev(dev);
>> +
>> + if (!pci_has_p2pmem(p2p_dev)) {
>> + pr_err("PCI device has no peer-to-peer memory: %s\n",
>> + page);
>> + pci_dev_put(p2p_dev);
>> + return -ENODEV;
>> + }
>> + }
>
> Yikes. Shouldn't auto just be the normal yes case instead of this
> string parsing mess?
Sorry, I don't follow. The code, as is, should automatically select the
device if the user sets it to "yes" or "auto" or "y" or similar.
(Roughly similar to how kstrtobool() works, except '0' or '1' are not
accepted seeing they could overlap with PCI device names). In other
cases, it looks for the specific PCI device name to use exactly.
Are you saying it shouldn't work this way or are you saying the code to
implement it is too messy?
>> + if (rsp->req.sg != &rsp->cmd->inline_sg) {
>> + if (rsp->req.p2p_dev)
>> + pci_p2pmem_free_sgl(rsp->req.p2p_dev, rsp->req.sg,
>> + rsp->req.sg_cnt);
>> + else
>> + sgl_free(rsp->req.sg);
>> + }
>
> Can we factor this into a helper, as the other target drivers (fc for now,
> tcp soon) using sgl allocatins should share the code?
>
> (same for the alloc side)
Sure. Would the helpers belong in core.c?
Thanks,
Logan
Powered by blists - more mailing lists