[PATCH v6 17/18] nvmet: New NVMe PCI endpoint function target driver

[Manivannan Sadhasivam]

On Fri, Dec 20, 2024 at 05:59:05PM +0900, Damien Le Moal wrote:

[...]

> >> +static void nvmet_pci_epf_clear_bar(struct nvmet_pci_epf *nvme_epf)
> >> +{
> >> +	struct pci_epf *epf = nvme_epf->epf;
> >> +
> >> +	pci_epc_clear_bar(epf->epc, epf->func_no, epf->vfunc_no,
> >> +			  &epf->bar[BAR_0]);
> >> +	pci_epf_free_space(epf, nvme_epf->reg_bar, BAR_0, PRIMARY_INTERFACE);
> >> +	nvme_epf->reg_bar = NULL;
> > 
> > Memory for BAR 0 is allocated in nvmet_pci_epf_bind(), but it is freed in both
> > nvmet_pci_epf_epc_deinit() and nvmet_pci_epf_bind(). This will cause NULL ptr
> > dereference if epc_deinit() gets called after nvmet_pci_epf_bind() and then
> > epc_init() is called (we call epc_deinit() once PERST# is deasserted to cleanup
> > the EPF for platforms requiring refclk from host).
> > 
> > So please move pci_epf_free_space() and 'nvme_epf->reg_bar = NULL' to a
> > separate helper nvmet_pci_epf_free_bar() and call that only from
> > nvmet_pci_epf_unbind() (outside of 'epc->init_complete' check).
> 
> I do not get PERST# on the RK3588... So I never noticed this.
> Does this work for you ?
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 8db084f1b20b..4d2a66668e73 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -2170,14 +2170,23 @@ static int nvmet_pci_epf_configure_bar(struct
> nvmet_pci_epf *nvme_epf)
>         return 0;
>  }
> 
> +static void nvmet_pci_epf_free_bar(struct nvmet_pci_epf *nvme_epf)
> +{
> +       struct pci_epf *epf = nvme_epf->epf;
> +
> +       if (!nvme_epf->reg_bar)
> +               return;
> +
> +       pci_epf_free_space(epf, nvme_epf->reg_bar, BAR_0, PRIMARY_INTERFACE);
> +       nvme_epf->reg_bar = NULL;
> +}
> +
>  static void nvmet_pci_epf_clear_bar(struct nvmet_pci_epf *nvme_epf)
>  {
>         struct pci_epf *epf = nvme_epf->epf;
> 
>         pci_epc_clear_bar(epf->epc, epf->func_no, epf->vfunc_no,
>                           &epf->bar[BAR_0]);
> -       pci_epf_free_space(epf, nvme_epf->reg_bar, BAR_0, PRIMARY_INTERFACE);
> -       nvme_epf->reg_bar = NULL;
>  }
> 
>  static int nvmet_pci_epf_init_irq(struct nvmet_pci_epf *nvme_epf)
> @@ -2319,8 +2328,6 @@ static void nvmet_pci_epf_epc_deinit(struct pci_epf *epf)
> 
>         nvmet_pci_epf_deinit_dma(nvme_epf);
>         nvmet_pci_epf_clear_bar(nvme_epf);
> -
> -       mutex_destroy(&nvme_epf->mmio_lock);
>  }
> 
>  static int nvmet_pci_epf_link_up(struct pci_epf *epf)
> @@ -2390,8 +2397,9 @@ static void nvmet_pci_epf_unbind(struct pci_epf *epf)
>         if (epc->init_complete) {
>                 nvmet_pci_epf_deinit_dma(nvme_epf);
>                 nvmet_pci_epf_clear_bar(nvme_epf);
> -               mutex_destroy(&nvme_epf->mmio_lock);
>         }
> +
> +       nvmet_pci_epf_free_bar(nvme_epf);
>  }
> 
>  static struct pci_epf_header nvme_epf_pci_header = {
> 
> > With the above change, I'm able to get this EPF driver working on my Qcom RC/EP
> > setup.
> 
> With the above, does it work for you ?
> 

Yes, it does!

One more suggestion. Since you correctly removed mutex_destroy() from deinit()
and unbind(), you should also switch to devm_mutex_init() in probe().

- Mani

-- 
மணிவண்ணன் சதாசிவம்