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
