Re: [PATCH v4 17/18] nvmet: New NVMe PCI endpoint target driver

[Damien Le Moal <dlemoal@kernel.org>]

On 2024/12/17 8:41, Manivannan Sadhasivam wrote:
>>>> +	/* Create the target controller. */
>>>> +	ret = nvmet_pciep_create_ctrl(nvme_epf, max_nr_queues);
>>>> +	if (ret) {
>>>> +		dev_err(&epf->dev,
>>>> +			"Create NVMe PCI target controller failed\n");
>>>
>>> Failed to create NVMe PCI target controller
>>
>> How is that better ?
>>
> 
> It is common for the error messages to start with 'Failed to...'. Also 'Create
> NVMe PCI target controller failed' doesn't sound correct to me. But I am not a
> native english speaker, so my views could be wrong.

I do not think this is true for all subsystems. But sure, I can change the message.

>>> Why these are coming from somewhere else and not configured within the EPF
>>> driver?
>>
>> They are set through the nvme target configfs. So there is no need to have these
>> again setup through the epf configfs. We just grab the values set for the NVME
>> target subsystem config.
>>
> 
> But in documentation you were configuring the vendor_id twice:
> 
> 	# echo "0x1b96" > nvmepf.0.nqn/attr_vendor_id
> 	...
>         # echo 0x1b96 > nvmepf.0/vendorid
> 
> And that's what confused me. You need to get rid of the second command and add a
> note that the vendor_id used in target configfs will be reused.

vendor_id != subsys_vendor_id :) These are 2 different fields. subsys_vendor_id
is reported by the identify controller command and is also present in the PCI
config space. vendor_id is not reported by the identify controller command and
present only in the PCI config space.

For the config example, I simply used the same values for both fields, but they
can be different. NVMe PCIe specs are a bit of a mess around these IDs...

>>>> +static int nvmet_pciep_epf_link_up(struct pci_epf *epf)
>>>> +{
>>>> +	struct nvmet_pciep_epf *nvme_epf = epf_get_drvdata(epf);
>>>> +	struct nvmet_pciep_ctrl *ctrl = &nvme_epf->ctrl;
>>>> +
>>>> +	dev_info(nvme_epf->ctrl.dev, "PCI link up\n");
>>>
>>> These prints are supposed to come from the controller drivers. So no need to
>>> have them here also.
>>
>> Nope, the controller driver does not print anything. At least the DWC driver
>> does not print anything.
>>
> 
> Which DWC driver? pcie-dw-rockchip? But other drivers like pcie-qcom-ep have
> these prints already. And this EPF driver is not tied to a single controller
> driver. As said earlier, these prints are supposed to be added to the controller
> drivers.

The DWC driver for the rk2588 (drivers/pci/controllers/dwc/*) is missing this
message.


-- 
Damien Le Moal
Western Digital Research