Re: [PATCH v3 11/11] nvmet: Optionally use PCI P2P memory

[Logan Gunthorpe]

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