Re: [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 3 Apr 2024 09:40:27 +0000
"Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:

> > Dan Williams wrote:  
> > > Kobayashi,Daisuke wrote:  
> > > > +static struct attribute_group cxl_rcd_group = {
> > > > +		.attrs = cxl_rcd_attrs,
> > > > +		.is_visible = cxl_rcd_visible,
> > > > +};
> > > > +
> > > > +__ATTRIBUTE_GROUPS(cxl_rcd);
> > > > +
> > > >  static int cxl_pci_probe(struct pci_dev *pdev, const struct
> > > > pci_device_id *id)  {
> > > >  	struct pci_host_bridge *host_bridge =
> > > > pci_find_host_bridge(pdev->bus); @@ -806,6 +995,9 @@ static int  
> > > cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)  
> > > >  	if (IS_ERR(mds))
> > > >  		return PTR_ERR(mds);
> > > >  	cxlds = &mds->cxlds;
> > > > +	device_create_file(&pdev->dev, &dev_attr_rcd_link_cap);
> > > > +	device_create_file(&pdev->dev, &dev_attr_rcd_link_ctrl);
> > > > +	device_create_file(&pdev->dev, &dev_attr_rcd_link_status);  
> > >
> > > No need to manually call device_create_file() when the attribute group is
> > > already registered below. I am surprised you did not get duplicate sysfs file
> > > warnings when registering these files twice.
> > >  
> > 
> > Thank you for pointing it out. Remove these calls.
> >   
> If you are aware of the cause, I would appreciate your insight. 
> In my environment, when I removed this device_create_file(), 
> the file was not generated in sysfs. Therefore, I have not been 
> able to remove this manual procedure at the moment. Is there a 
> possibility that simply registering with 
> struct pci_driver.driver.groups will not generate a sysfs file?
> 
> > > >  	pci_set_drvdata(pdev, cxlds);
> > > >
> > > >  	cxlds->rcd = is_cxl_restricted(pdev); @@ -967,6 +1159,7 @@ static
> > > > struct pci_driver cxl_pci_driver = {
> > > >  	.err_handler		= &cxl_error_handlers,
> > > >  	.driver	= {
> > > >  		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
> > > > +		.dev_groups	= cxl_rcd_groups,
Odd though it may seem, try setting
.dev_groups in the outer structure not the inner one.

		.err_handler = &....
		.dev_groups = cxl_rcd_groups,
		.driver = {
			...
		},
Similar to:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sp-pci.c#L592

For reasons I don't follow, __pci_register_driver() overrides the internal one.
Some ancient bit of code migration that never finished?
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L1447

I did some digging.
https://lore.kernel.org/all/20190731124349.4474-2-gregkh@linuxfoundation.org/

So this got added to the driver core fairly recently (only 4 years ago ;)

The the dev_groups was added to pci in
https://lore.kernel.org/all/20210512142648.666476-8-andrey.grodzovsky@amd.com/

I'm not sure why the bounce via pci_driver is needed though.
Greg, looks like this came from usb originally, can you recall the reasoning?

> > > >  	},
> > > >  };
> > > >
> > > > --
> > > > 2.43.0
> > > >
> > > >  
> > >  
> >   
> 
>