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

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

On 12/20/24 18:26, Manivannan Sadhasivam wrote:
> 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().

Ah! Yes. Will do.

-- 
Damien Le Moal
Western Digital Research