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
--
மணிவண்ணன் சதாசிவம்
