Dan Williams wrote:
> Daisuke Kobayashi (Fujitsu) wrote:
> [..]
> > I would like to report on some additional findings.
> > The process of registering cxl_rcd_groups to struct
> > pci_driver.driver.dev_groups seems to not generate a file in sysfs when
> looking at the contents of the module_pci_driver() macro.
>
> Oh, apologies, and thanks for taking a deeper look. The failure is because my
> suggested example led you astray. I suggested this:
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> 2ff361e756d6..eec04f103aa8 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -971,6 +971,7 @@ static struct pci_driver cxl_pci_driver = {
> .err_handler = &cxl_error_handlers,
> .driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + .dev_groups = cxl_rcd_groups,
> },
> };
>
>
> ...the correct place to put it is here:
>
> @@ -969,6 +969,7 @@ static struct pci_driver cxl_pci_driver = {
> .id_table = cxl_mem_pci_tbl,
> .probe = cxl_pci_probe,
> .err_handler = &cxl_error_handlers,
> + .dev_groups = cxl_rcd_groups,
> .driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
>
>
> ...otherwise __pci_register_driver() will overwrite it. This is a subtle bug given
> probe_type is directly initialized in .driver.
>
> > For this feature, I think it would be best to output the values to a
> > directory of /sys/bus/pci/devices/<pci-addr>/. To output to this
> > directory, the attribute would need to be registered to pci_dev.dev.
> > My current understanding is that the best way to do this would be to
> > register the attribute with device_add_groups(&pdev->dev,
> > cxl_rcd_groups) on probe and remove the files with
>
> No, the dynamic sysfs registration APIs should be avoided when possible.
> The above fix is what you need.
Thank you. With your guidance and the hint in the previous email,
I now understand the idea of "groups" and "dev_groups".
I see that, by registering "cxl_rcd_groups" with "pci_driver.dev_groups",
files are created in device's sysfs when the device is attached.
I will update the patch to reflect this.
