Re: [PATCH] nvdimm: nvdimm_bus_register: Avoid adding device to the unregistered bus

[lizhijian@xxxxxxxxxxx]

On 21/03/2023 01:30, Dan Williams wrote:
> lizhijian@xxxxxxxxxxx wrote:
> [..]
>>>> Dan,
>>>>
>>>> Configure the kconfig with ACPI_NFIT [=m] && LIBNVDIMM [=y], and add extra kernel booting parameter
>>>> 'initcall_blacklist=libnvdimm_init'. Then kernel panic!
>>>
>>> That's expected though,
>>
>> Do you mean we just keep it as it is.
> 
> Yes.
> 
>>
>>
>>> you can't block libnvdimm_init and then expect
>>> modules that link to libnvdimm to work.
>> Ah, we would rather see it *unable to work* than panic, isn't it.
> 
> That part is true, but consider the implications of adding error
> handling to all code that can no longer depend on initcall ordering, not
> just libnvdimm. This would be a large paradigm shift.
> 
> Now I do think it would be a good idea to fail device_add() if the bus
> is not registered, but I *think* that happens now as a result of:
> 
> 5221b82d46f2 driver core: bus: bus_add/probe/remove_device() cleanups
> 
> ...can you double check if you have that commit in your tests?

Great, panic is gone after i upgraded to the upstream!


> Now I do think it would be a good idea to fail device_add() if the bus
> is not registered,

BTW, below line 369: device_add() didn't fail in practical.

  354         mutex_init(&nvdimm_bus->reconfig_mutex);
  355         badrange_init(&nvdimm_bus->badrange);
  356         nvdimm_bus->nd_desc = nd_desc;
  357         nvdimm_bus->dev.parent = parent;
  358         nvdimm_bus->dev.type = &nvdimm_bus_dev_type;
  359         nvdimm_bus->dev.groups = nd_desc->attr_groups;
  360         nvdimm_bus->dev.bus = &nvdimm_bus_type;
  361         nvdimm_bus->dev.of_node = nd_desc->of_node;
  362         device_initialize(&nvdimm_bus->dev);
  363         lockdep_set_class(&nvdimm_bus->dev.mutex, &nvdimm_bus_key);
  364         device_set_pm_not_required(&nvdimm_bus->dev);
  365         rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id);
  366         if (rc)
  367                 goto err;
  368
  369         rc = device_add(&nvdimm_bus->dev);
  370         dev_err(&nvdimm_bus->dev, "registration failed: %d\n", rc);

Thanks
Zhijian