Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Jul 2023 15:57:59 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Abhijit Gangurde <abhijit.gangurde@amd.com>
Cc:     linux-kernel@vger.kernel.org, Nipun.Gupta@amd.com,
        nikhil.agarwal@amd.com, puneet.gupta@amd.com, git@amd.com,
        michal.simek@amd.com,
        Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
Subject: Re: [PATCH 1/4] cdx: add support for bus enable and disable
Message-ID: <2023071109-saturate-purist-d854@gregkh>
References: <20230711121027.936487-1-abhijit.gangurde@amd.com>
 <20230711121027.936487-2-abhijit.gangurde@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230711121027.936487-2-abhijit.gangurde@amd.com>
Precedence: bulk

On Tue, Jul 11, 2023 at 05:40:24PM +0530, Abhijit Gangurde wrote:
> CDX bus needs to be disabled before updating/writing devices
> in the FPGA. Once the devices are written, the bus shall be
> enabled. This change provides sysfs entry to enable/disable the
> CDX bus.
> 
> Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cdx | 11 +++++
>  drivers/cdx/cdx.c                       | 26 ++++++++++++
>  drivers/cdx/controller/cdx_controller.c | 27 +++++++++++++
>  drivers/cdx/controller/mc_cdx_pcol.h    | 54 +++++++++++++++++++++++++
>  drivers/cdx/controller/mcdi_functions.c | 24 +++++++++++
>  drivers/cdx/controller/mcdi_functions.h | 16 ++++++++
>  include/linux/cdx/cdx_bus.h             |  6 +++
>  7 files changed, 164 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index 7af477f49998..0afa85b3c63b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -11,6 +11,17 @@ Description:
>  
>  		  # echo 1 > /sys/bus/cdx/rescan
>  
> +What:		/sys/bus/cdx/enable
> +Date:		July 2023
> +Contact:	nipun.gupta@amd.com
> +Description:
> +		Writing y/1/on to this file enables the CDX bus and
> +		writing n/0/off disables the bus.
> +
> +		For example to disable CDX bus::
> +
> +		  # echo 0 > /sys/bus/cdx/enable

What could go wrong!  :)

You don't say why disabling / enabling the bus is needed, this feels
like a very huge stick, why is this for all busses, and not just an
individual CDX bus?

> +
>  What:		/sys/bus/cdx/devices/.../vendor
>  Date:		March 2023
>  Contact:	nipun.gupta@amd.com
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index d2cad4c670a0..48c493a43491 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -380,6 +380,30 @@ static struct attribute *cdx_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(cdx_dev);
>  
> +static ssize_t enable_store(const struct bus_type *bus,
> +			    const char *buf, size_t count)
> +{
> +	struct cdx_controller *cdx;
> +	unsigned long index;
> +	bool enable;
> +	int ret;
> +
> +	if (kstrtobool(buf, &enable) < 0)
> +		return -EINVAL;
> +
> +	xa_for_each(&cdx_controllers, index, cdx) {
> +		if (cdx->enabled == enable)
> +			continue;
> +
> +		ret = cdx->ops->enable(cdx, enable);
> +		if (ret)
> +			dev_err(cdx->dev, "cdx bus enable/disable failed\n");

You can say if this was enabled or disabled to help figure things out.

> +	}

No locking needed at all?  What happens if controllers are added or
removed?

> +
> +	return count;
> +}
> +static BUS_ATTR_WO(enable);
> +
>  static ssize_t rescan_store(const struct bus_type *bus,
>  			    const char *buf, size_t count)
>  {
> @@ -410,6 +434,7 @@ static ssize_t rescan_store(const struct bus_type *bus,
>  static BUS_ATTR_WO(rescan);
>  
>  static struct attribute *cdx_bus_attrs[] = {
> +	&bus_attr_enable.attr,
>  	&bus_attr_rescan.attr,
>  	NULL,
>  };
> @@ -541,6 +566,7 @@ void cdx_unregister_controller(struct cdx_controller *cdx)
>  	if (cdx->id >= MAX_CDX_CONTROLLERS)
>  		return;
>  
> +	cdx->ops->enable(cdx, false);
>  	device_for_each_child(cdx->dev, NULL, cdx_unregister_device);
>  	xa_erase(&cdx_controllers, cdx->id);
>  }
> diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c
> index dc52f95f8978..ac8081f23cbe 100644
> --- a/drivers/cdx/controller/cdx_controller.c
> +++ b/drivers/cdx/controller/cdx_controller.c
> @@ -45,6 +45,21 @@ void cdx_rpmsg_pre_remove(struct cdx_controller *cdx)
>  	cdx_mcdi_wait_for_quiescence(cdx->priv, MCDI_RPC_TIMEOUT);
>  }
>  
> +static int cdx_bus_enable(struct cdx_controller *cdx, bool enable)

Why not just a disable and enable callback instead of being forced to
always rembmer that "foo_enable(foo, false)" really is "foo_disable(foo)"?

This is messy, please be explicit.

thanks,

greg k-h