On Tue, Dec 17, 2024 at 09:03:08AM -0800, Damien Le Moal wrote:
> 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.
>
I know the difference between vendor_id and subsys_vendor_id :) But as I quoted,
you are using the same vendor id value in 2 places. One in nvmet configfs and
another in epf configfs. But internally, you just reuse the nvmet configfs value
in epf. And this is not evident in the documentation.
> 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.
>
Yeah, maybe you should add it later.
- Mani
--
மணிவண்ணன் சதாசிவம்
