Re: [PATCH v4 01/17] PCI: endpoint: Automatically create a function specific attributes group

[Manivannan Sadhasivam <mani@kernel.org>]

On Thu, Mar 30, 2023 at 05:53:41PM +0900, Damien Le Moal wrote:
> A PCI endpoint function driver can define function specific attributes
> under its function configfs directory using the add_cfs() endpoint
> driver operation. This is done by tighing up the mkdir operation for
> the function configfs directory to a call to the add_cfs() operation.
> However, there are no checks preventing the user from repeatedly
> creating function specific attribute directories with different names,
> resulting in the same endpoint specific attributes group being added
> multiple times, which also result in an invalid reference counting for
> the attribute groups. E.g., using the pci-epf-ntb function driver as an
> example, the user creates the function as follows:

[...]

> Fix this by modifying pci_epf_cfs_work() to execute the new function
> pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs()
> to obtain the function specific attribute group and the group name
> (directory name) from the endpoint function driver. If the function
> driver defines an attribute group, pci_ep_cfs_add_type_group() then
> proceeds to register this group using configfs_register_group(), thus
> automatically exposing the function type pecific onfigfs attributes to

Still you haven't fixed this typo. But I don't expect you to respin unless there
are other changes.

> the user. E.g.:
> 
> $ modprobe pci-epf-ntb
> $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb
> $ mkdir func0
> $ tree func0
> func0/
> |-- baseclass_code
> |-- cache_line_size
> |-- ...
> |-- pci_epf_ntb.0
> |   |-- db_count
> |   |-- mw1
> |   |-- mw2
> |   |-- mw3
> |   |-- mw4
> |   |-- num_mws
> |   `-- spad_count
> |-- primary
> |-- ...
> `-- vendorid
> 
> With this change, there is no need for the user to create or delete
> directories in the endpoint function attributes directory. The
> pci_epf_type_group_ops group operations are thus removed.
> 
> The documentation for the pci-epf-ntb and pci-epf-vntb function drivers
> are updated to reflect this change, removing the explanations showing
> the need to manually create the sub-directory for the function specific
> attributes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> ---
>  Documentation/PCI/endpoint/pci-ntb-howto.rst  | 11 ++---
>  Documentation/PCI/endpoint/pci-vntb-howto.rst | 13 +++---
>  drivers/pci/endpoint/pci-ep-cfs.c             | 41 +++++++++----------
>  3 files changed, 28 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/PCI/endpoint/pci-ntb-howto.rst b/Documentation/PCI/endpoint/pci-ntb-howto.rst
> index 1884bf29caba..4261e7157ef1 100644
> --- a/Documentation/PCI/endpoint/pci-ntb-howto.rst
> +++ b/Documentation/PCI/endpoint/pci-ntb-howto.rst
> @@ -88,13 +88,10 @@ commands can be used::
>  	# echo 0x104c > functions/pci_epf_ntb/func1/vendorid
>  	# echo 0xb00d > functions/pci_epf_ntb/func1/deviceid
>  
> -In order to configure NTB specific attributes, a new sub-directory to func1
> -should be created::
> -
> -	# mkdir functions/pci_epf_ntb/func1/pci_epf_ntb.0/
> -
> -The NTB function driver will populate this directory with various attributes
> -that can be configured by the user::
> +The PCI endpoint framework also automatically creates a sub-directory in the
> +function attribute directory. This sub-directory has the same name as the name
> +of the function device and is populated with the following NTB specific
> +attributes that can be configured by the user::
>  
>  	# ls functions/pci_epf_ntb/func1/pci_epf_ntb.0/
>  	db_count    mw1         mw2         mw3         mw4         num_mws
> diff --git a/Documentation/PCI/endpoint/pci-vntb-howto.rst b/Documentation/PCI/endpoint/pci-vntb-howto.rst
> index 4ab8e4a26d4b..70d3bc90893f 100644
> --- a/Documentation/PCI/endpoint/pci-vntb-howto.rst
> +++ b/Documentation/PCI/endpoint/pci-vntb-howto.rst
> @@ -84,13 +84,10 @@ commands can be used::
>  	# echo 0x1957 > functions/pci_epf_vntb/func1/vendorid
>  	# echo 0x0809 > functions/pci_epf_vntb/func1/deviceid
>  
> -In order to configure NTB specific attributes, a new sub-directory to func1
> -should be created::
> -
> -	# mkdir functions/pci_epf_vntb/func1/pci_epf_vntb.0/
> -
> -The NTB function driver will populate this directory with various attributes
> -that can be configured by the user::
> +The PCI endpoint framework also automatically creates a sub-directory in the
> +function attribute directory. This sub-directory has the same name as the name
> +of the function device and is populated with the following NTB specific
> +attributes that can be configured by the user::
>  
>  	# ls functions/pci_epf_vntb/func1/pci_epf_vntb.0/
>  	db_count    mw1         mw2         mw3         mw4         num_mws
> @@ -103,7 +100,7 @@ A sample configuration for NTB function is given below::
>  	# echo 1 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/num_mws
>  	# echo 0x100000 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/mw1
>  
> -A sample configuration for virtual NTB driver for virutal PCI bus::
> +A sample configuration for virtual NTB driver for virtual PCI bus::
>  
>  	# echo 0x1957 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_vid
>  	# echo 0x080A > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_pid
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index 4b8ac0ac84d5..b16fc6093c20 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -23,6 +23,7 @@ struct pci_epf_group {
>  	struct config_group group;
>  	struct config_group primary_epc_group;
>  	struct config_group secondary_epc_group;
> +	struct config_group *type_group;
>  	struct delayed_work cfs_work;
>  	struct pci_epf *epf;
>  	int index;
> @@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = {
>  	.release		= pci_epf_release,
>  };
>  
> -static struct config_group *pci_epf_type_make(struct config_group *group,
> -					      const char *name)
> -{
> -	struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item);
> -	struct config_group *epf_type_group;
> -
> -	epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group);
> -	return epf_type_group;
> -}
> -
> -static void pci_epf_type_drop(struct config_group *group,
> -			      struct config_item *item)
> -{
> -	config_item_put(item);
> -}
> -
> -static struct configfs_group_operations pci_epf_type_group_ops = {
> -	.make_group     = &pci_epf_type_make,
> -	.drop_item      = &pci_epf_type_drop,
> -};
> -
>  static const struct config_item_type pci_epf_type = {
> -	.ct_group_ops	= &pci_epf_type_group_ops,
>  	.ct_item_ops	= &pci_epf_ops,
>  	.ct_attrs	= pci_epf_attrs,
>  	.ct_owner	= THIS_MODULE,
>  };
>  
> +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group)
> +{
> +	struct config_group *group;
> +
> +	group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group);
> +	if (!group)
> +		return;
> +
> +	if (IS_ERR(group)) {
> +		pr_err("failed to create epf type specific attributes\n");
> +		return;
> +	}
> +
> +	configfs_register_group(&epf_group->group, group);
> +}
> +
>  static void pci_epf_cfs_work(struct work_struct *work)
>  {
>  	struct pci_epf_group *epf_group;
> @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work)
>  		pr_err("failed to create 'secondary' EPC interface\n");
>  		return;
>  	}
> +
> +	pci_ep_cfs_add_type_group(epf_group);
>  }
>  
>  static struct config_group *pci_epf_make(struct config_group *group,
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்