RE: [PATCH 1/4] cdx: add support for bus enable and disable

[Gangurde, Abhijit]

[AMD Official Use Only - General]

> > 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@xxxxxxx>
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> vuuren@xxxxxxx>
> > Tested-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> > ---
> >  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@xxxxxxx
> > +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?
>

As said in the description of the patch, disabling/enabling is needed when FPGA is being reprogrammed.
We would enhance this interface for handling multiple buses in v2.

> > +
> >  What:              /sys/bus/cdx/devices/.../vendor
> >  Date:              March 2023
> >  Contact:   nipun.gupta@xxxxxxx
> > 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.
>

Will correct in v2.

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

As of now we had only the controller but you are correct. We would add a lock to protect the controller list.

>
> > +
> > +   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.

Will update in v2.

Thanks,
Abhijit