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
