Re: [PATCH v2] cdx: Rename MCDI_LOGGING to CDX_MCDI_LOGGING

[Greg KH]

On Wed, May 24, 2023 at 11:46:13PM +0530, Abhijit Gangurde wrote:
> MCDI_LOGGING is too generic considering other MCDI users
> SFC_MCDI_LOGGING and SFC_SIENA_MCDI_LOGGING. Rename it to
> CDX_MCDI_LOGGING makes it more domain specific.
> 
> Also, Move CONFIG_CDX_MCDI_LOGGING to header file
> and make logging variable as a configurable sysfs parameter.

No, sorry, that is not allowed.  Just use the normal dynamic debugging
in the kernel like everyone else uses.

Also you didn't document your new sysfs file, which would have not
allowed me to take this no matter what.

> 
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> Suggested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> ---
> Changes v1->v2:
> - Moved CONFIG_CDX_MCDI_LOGGING flag to header file
> - Added sysfs entry to enable/disable mcdi logging
> 
>  drivers/cdx/controller/Kconfig          |  2 +-
>  drivers/cdx/controller/cdx_controller.c | 21 ++++++++++++
>  drivers/cdx/controller/mcdi.c           | 45 ++++++++++++-------------
>  drivers/cdx/controller/mcdi.h           |  8 +++--
>  4 files changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cdx/controller/Kconfig b/drivers/cdx/controller/Kconfig
> index c3e3b9ff8dfe..e7014e9819ea 100644
> --- a/drivers/cdx/controller/Kconfig
> +++ b/drivers/cdx/controller/Kconfig
> @@ -18,7 +18,7 @@ config CDX_CONTROLLER
>  
>  	  If unsure, say N.
>  
> -config MCDI_LOGGING
> +config CDX_MCDI_LOGGING
>  	bool "MCDI Logging for the CDX controller"
>  	depends on CDX_CONTROLLER
>  	help
> diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c
> index dc52f95f8978..e5b1a2c01b87 100644
> --- a/drivers/cdx/controller/cdx_controller.c
> +++ b/drivers/cdx/controller/cdx_controller.c
> @@ -124,6 +124,23 @@ static struct cdx_ops cdx_ops = {
>  	.dev_configure	= cdx_configure_device,
>  };
>  
> +static ssize_t mcdi_logging_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct cdx_controller *cdx = dev_get_drvdata(dev);
> +	bool enable;
> +
> +	if (kstrtobool(buf, &enable) < 0)
> +		return -EINVAL;
> +
> +	if (cdx_enable_mcdi_logging(cdx->priv, enable))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_WO(mcdi_logging);
> +
>  static int xlnx_cdx_probe(struct platform_device *pdev)
>  {
>  	struct cdx_controller *cdx;
> @@ -154,6 +171,9 @@ static int xlnx_cdx_probe(struct platform_device *pdev)
>  	cdx->priv = cdx_mcdi;
>  	cdx->ops = &cdx_ops;
>  
> +	if (device_create_file(&pdev->dev, &dev_attr_mcdi_logging))
> +		dev_warn(&pdev->dev, "Failed to create sysfs file\n");

You just raced with userspace and lost.  Even if this feature was ok,
this is not how to create sysfs files, sorry.

greg k-h