This patch series provides sysfs interface to
- enable and disable of cdx bus
- reset_all for all the devices on cdx bus
- resource files for cdx device
- subsystem, class and revision for cdx device
Abhijit Gangurde (4):
cdx: add support for bus enable and disable
cdx: add sysfs for reset_all
cdx: create sysfs resource files
cdx: add sysfs for subsystem, class and revision
Documentation/ABI/testing/sysfs-bus-cdx | 82 +++++++++
drivers/cdx/cdx.c | 223 +++++++++++++++++++++++-
drivers/cdx/cdx.h | 8 +
drivers/cdx/controller/cdx_controller.c | 27 +++
drivers/cdx/controller/mc_cdx_pcol.h | 54 ++++++
drivers/cdx/controller/mcdi_functions.c | 31 ++++
drivers/cdx/controller/mcdi_functions.h | 16 ++
include/linux/cdx/cdx_bus.h | 43 ++++-
include/linux/mod_devicetable.h | 10 ++
scripts/mod/devicetable-offsets.c | 4 +
scripts/mod/file2alias.c | 8 +
11 files changed, 502 insertions(+), 4 deletions(-)
--
2.25.1
Resource files provides the basic MMIO regions info to the
user-space. Also, resources<x> devices can be used to mmap the
MMIO regions in the user-space.
Co-developed-by: Puneet Gupta <[email protected]>
Signed-off-by: Puneet Gupta <[email protected]>
Co-developed-by: Nipun Gupta <[email protected]>
Signed-off-by: Nipun Gupta <[email protected]>
Signed-off-by: Abhijit Gangurde <[email protected]>
Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
Tested-by: Nikhil Agarwal <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
include/linux/cdx/cdx_bus.h | 10 ++
3 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
index d9e00058471d..6ca47b6442ce 100644
--- a/Documentation/ABI/testing/sysfs-bus-cdx
+++ b/Documentation/ABI/testing/sysfs-bus-cdx
@@ -76,3 +76,18 @@ Description:
For example::
# echo 1 > /sys/bus/cdx/devices/.../remove
+
+What: /sys/bus/cdx/devices/.../resource
+Date: July 2023
+Contact: [email protected]
+Description:
+ The resource file contains host addresses of CDX device
+ resources. Each line of the resource file describes a region
+ with start, end, and flag fields.
+
+What: /sys/bus/cdx/devices/.../resource<N>
+Date: July 2023
+Contact: [email protected]
+Description:
+ The resource binary file contains the content of the memory
+ regions. These files can be m'maped from userspace.
diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index 4d20047b55bb..9d568df8e566 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -73,6 +73,8 @@
/* CDX controllers registered with the CDX bus */
static DEFINE_XARRAY_ALLOC(cdx_controllers);
+static void cdx_destroy_res_attr(struct cdx_device *cdx_dev, int num);
+
/**
* cdx_dev_reset - Reset a CDX device
* @dev: CDX device
@@ -126,6 +128,8 @@ static int cdx_unregister_device(struct device *dev,
{
struct cdx_device *cdx_dev = to_cdx_device(dev);
+ cdx_destroy_res_attr(cdx_dev, MAX_CDX_DEV_RESOURCES);
+
kfree(cdx_dev->driver_override);
cdx_dev->driver_override = NULL;
/*
@@ -375,12 +379,32 @@ static ssize_t driver_override_show(struct device *dev,
}
static DEVICE_ATTR_RW(driver_override);
+static ssize_t resource_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct cdx_device *cdx_dev = to_cdx_device(dev);
+ size_t len = 0;
+ int i;
+
+ for (i = 0; i < MAX_CDX_DEV_RESOURCES; i++) {
+ struct resource *res = &cdx_dev->res[i];
+
+ len += sysfs_emit_at(buf, len, "0x%016llx 0x%016llx 0x%016llx\n",
+ (unsigned long long)res->start,
+ (unsigned long long)res->end,
+ (unsigned long long)res->flags);
+ }
+
+ return len;
+}
+static DEVICE_ATTR_RO(resource);
+
static struct attribute *cdx_dev_attrs[] = {
&dev_attr_remove.attr,
&dev_attr_reset.attr,
&dev_attr_vendor.attr,
&dev_attr_device.attr,
&dev_attr_driver_override.attr,
+ &dev_attr_resource.attr,
NULL,
};
ATTRIBUTE_GROUPS(cdx_dev);
@@ -514,12 +538,106 @@ static void cdx_device_release(struct device *dev)
kfree(cdx_dev);
}
+static const struct vm_operations_struct cdx_phys_vm_ops = {
+#ifdef CONFIG_HAVE_IOREMAP_PROT
+ .access = generic_access_phys,
+#endif
+};
+
+/**
+ * cdx_mmap_resource - map a CDX resource into user memory space
+ * @fp: File pointer. Not used in this function, but required where
+ * this API is registered as a callback.
+ * @kobj: kobject for mapping
+ * @attr: struct bin_attribute for the file being mapped
+ * @vma: struct vm_area_struct passed into the mmap
+ *
+ * Use the regular CDX mapping routines to map a CDX resource into userspace.
+ *
+ * Return: true on success, false otherwise.
+ */
+static int cdx_mmap_resource(struct file *fp, struct kobject *kobj,
+ struct bin_attribute *attr,
+ struct vm_area_struct *vma)
+{
+ struct cdx_device *cdx_dev = to_cdx_device(kobj_to_dev(kobj));
+ int num = (unsigned long)attr->private;
+ struct resource *res;
+ unsigned long size;
+
+ res = &cdx_dev->res[num];
+ if (iomem_is_exclusive(res->start))
+ return -EINVAL;
+
+ /* Make sure the caller is mapping a valid resource for this device */
+ size = ((cdx_resource_len(cdx_dev, num) - 1) >> PAGE_SHIFT) + 1;
+ if (vma->vm_pgoff + vma_pages(vma) > size)
+ return -EINVAL;
+
+ /*
+ * Map memory region and vm->vm_pgoff is expected to be an
+ * offset within that region.
+ */
+ vma->vm_page_prot = pgprot_device(vma->vm_page_prot);
+ vma->vm_pgoff += (cdx_resource_start(cdx_dev, num) >> PAGE_SHIFT);
+ vma->vm_ops = &cdx_phys_vm_ops;
+ return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+ vma->vm_end - vma->vm_start,
+ vma->vm_page_prot);
+}
+
+static void cdx_destroy_res_attr(struct cdx_device *cdx_dev, int num)
+{
+ int i;
+
+ /* removing the bin attributes */
+ for (i = 0; i < num; i++) {
+ struct bin_attribute *res_attr;
+
+ res_attr = cdx_dev->res_attr[i];
+ if (res_attr) {
+ sysfs_remove_bin_file(&cdx_dev->dev.kobj, res_attr);
+ kfree(res_attr);
+ }
+ }
+}
+
+#define CDX_RES_ATTR_NAME_LEN 10
+static int cdx_create_res_attr(struct cdx_device *cdx_dev, int num)
+{
+ struct bin_attribute *res_attr;
+ char *res_attr_name;
+ int ret;
+
+ res_attr = kzalloc(sizeof(*res_attr) + CDX_RES_ATTR_NAME_LEN, GFP_ATOMIC);
+ if (!res_attr)
+ return -ENOMEM;
+
+ res_attr_name = (char *)(res_attr + 1);
+
+ sysfs_bin_attr_init(res_attr);
+
+ cdx_dev->res_attr[num] = res_attr;
+ sprintf(res_attr_name, "resource%d", num);
+
+ res_attr->mmap = cdx_mmap_resource;
+ res_attr->attr.name = res_attr_name;
+ res_attr->attr.mode = 0600;
+ res_attr->size = cdx_resource_len(cdx_dev, num);
+ res_attr->private = (void *)(unsigned long)num;
+ ret = sysfs_create_bin_file(&cdx_dev->dev.kobj, res_attr);
+ if (ret)
+ kfree(res_attr);
+
+ return ret;
+}
+
int cdx_device_add(struct cdx_dev_params *dev_params)
{
struct cdx_controller *cdx = dev_params->cdx;
struct device *parent = cdx->dev;
struct cdx_device *cdx_dev;
- int ret;
+ int ret, i;
cdx_dev = kzalloc(sizeof(*cdx_dev), GFP_KERNEL);
if (!cdx_dev)
@@ -558,7 +676,26 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
goto fail;
}
+ /* Create resource<N> attributes */
+ for (i = 0; i < MAX_CDX_DEV_RESOURCES; i++) {
+ if (cdx_resource_flags(cdx_dev, i) & IORESOURCE_MEM) {
+ /* skip empty resources */
+ if (!cdx_resource_len(cdx_dev, i))
+ continue;
+
+ ret = cdx_create_res_attr(cdx_dev, i);
+ if (ret != 0) {
+ dev_err(&cdx_dev->dev,
+ "cdx device resource<%d> file creation failed: %d", i, ret);
+ goto fail1;
+ }
+ }
+ }
+
return 0;
+fail1:
+ cdx_destroy_res_attr(cdx_dev, i);
+ device_del(&cdx_dev->dev);
fail:
/*
* Do not free cdx_dev here as it would be freed in
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index 5da0634ae4ee..e93f1cd8ae33 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -104,6 +104,7 @@ struct cdx_device {
u8 bus_num;
u8 dev_num;
struct resource res[MAX_CDX_DEV_RESOURCES];
+ struct bin_attribute *res_attr[MAX_CDX_DEV_RESOURCES];
u8 res_count;
u64 dma_mask;
u16 flags;
@@ -114,6 +115,15 @@ struct cdx_device {
#define to_cdx_device(_dev) \
container_of(_dev, struct cdx_device, dev)
+#define cdx_resource_start(dev, num) ((dev)->res[(num)].start)
+#define cdx_resource_end(dev, num) ((dev)->res[(num)].end)
+#define cdx_resource_flags(dev, num) ((dev)->res[(num)].flags)
+#define cdx_resource_len(dev, num) \
+ ((cdx_resource_start((dev), (num)) == 0 && \
+ cdx_resource_end((dev), (num)) == \
+ cdx_resource_start((dev), (num))) ? 0 : \
+ (cdx_resource_end((dev), (num)) - \
+ cdx_resource_start((dev), (num)) + 1))
/**
* struct cdx_driver - CDX device driver
* @driver: Generic device driver
--
2.25.1
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 <[email protected]>
Signed-off-by: Nipun Gupta <[email protected]>
Signed-off-by: Abhijit Gangurde <[email protected]>
Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
Tested-by: Nikhil Agarwal <[email protected]>
---
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: [email protected]
+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: /sys/bus/cdx/devices/.../vendor
Date: March 2023
Contact: [email protected]
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");
+ }
+
+ 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)
+{
+ int ret;
+
+ if (enable)
+ ret = cdx_mcdi_bus_enable(cdx->priv);
+ else
+ ret = cdx_mcdi_bus_disable(cdx->priv);
+
+ if (!ret)
+ cdx->enabled = enable;
+
+ return ret;
+}
+
static int cdx_configure_device(struct cdx_controller *cdx,
u8 bus_num, u8 dev_num,
struct cdx_device_config *dev_config)
@@ -80,11 +95,22 @@ static int cdx_scan_devices(struct cdx_controller *cdx)
for (bus_num = 0; bus_num < num_cdx_bus; bus_num++) {
u8 num_cdx_dev;
+ ret = cdx_mcdi_bus_enable(cdx_mcdi);
+ if (ret && ret != -EALREADY) {
+ dev_err(cdx->dev,
+ "CDX bus %d enable failed: %d\n", bus_num, ret);
+ continue;
+ }
+
/* MCDI FW Read: Fetch the number of devices present */
ret = cdx_mcdi_get_num_devs(cdx_mcdi, bus_num);
if (ret < 0) {
dev_err(cdx->dev,
"Get devices on CDX bus %d failed: %d\n", bus_num, ret);
+ ret = cdx_mcdi_bus_disable(cdx_mcdi);
+ if (ret)
+ dev_err(cdx->dev,
+ "CDX bus %d disable failed: %d\n", bus_num, ret);
continue;
}
num_cdx_dev = (u8)ret;
@@ -120,6 +146,7 @@ static int cdx_scan_devices(struct cdx_controller *cdx)
}
static struct cdx_ops cdx_ops = {
+ .enable = cdx_bus_enable,
.scan = cdx_scan_devices,
.dev_configure = cdx_configure_device,
};
diff --git a/drivers/cdx/controller/mc_cdx_pcol.h b/drivers/cdx/controller/mc_cdx_pcol.h
index 4ccb7b52951b..2de019406b57 100644
--- a/drivers/cdx/controller/mc_cdx_pcol.h
+++ b/drivers/cdx/controller/mc_cdx_pcol.h
@@ -455,6 +455,60 @@
#define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_REQUESTER_ID_OFST 84
#define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_REQUESTER_ID_LEN 4
+/***********************************/
+/*
+ * MC_CMD_CDX_BUS_DOWN
+ * Asserting reset on the CDX bus causes all devices on the bus to be quiesced.
+ * DMA bus mastering is disabled and any pending DMA request are flushed. Once
+ * the response is returned, the devices are guaranteed to no longer issue DMA
+ * requests or raise MSI interrupts. Further device MMIO accesses may have
+ * undefined results. While the bus reset is asserted, any of the enumeration
+ * or device configuration MCDIs will fail with EAGAIN. It is only legal to
+ * reload the relevant PL region containing CDX devices if the corresponding CDX
+ * bus is in reset. Depending on the implementation, the firmware may or may
+ * not enforce this restriction and it is up to the caller to make sure this
+ * requirement is satisfied.
+ */
+#define MC_CMD_CDX_BUS_DOWN 0x4
+#define MC_CMD_CDX_BUS_DOWN_MSGSET 0x4
+
+/* MC_CMD_CDX_BUS_DOWN_IN msgrequest */
+#define MC_CMD_CDX_BUS_DOWN_IN_LEN 4
+/* Bus number to put in reset, in range 0 to BUS_COUNT-1 */
+#define MC_CMD_CDX_BUS_DOWN_IN_BUS_OFST 0
+#define MC_CMD_CDX_BUS_DOWN_IN_BUS_LEN 4
+
+/*
+ * MC_CMD_CDX_BUS_DOWN_OUT msgresponse: The bus is quiesced, no further
+ * upstream traffic for devices on this bus.
+ */
+#define MC_CMD_CDX_BUS_DOWN_OUT_LEN 0
+
+/***********************************/
+/*
+ * MC_CMD_CDX_BUS_UP
+ * After bus reset is de-asserted, devices are in a state which is functionally
+ * equivalent to each device having been reset with MC_CMD_CDX_DEVICE_RESET. In
+ * other words, device logic is reset in a hardware-specific way, MMIO accesses
+ * are forwarded to the device, DMA bus mastering is disabled and needs to be
+ * re-enabled with MC_CMD_CDX_DEVICE_DMA_ENABLE once the driver is ready to
+ * start servicing DMA. If the underlying number of devices or device resources
+ * changed (e.g. if PL was reloaded) while the bus was in reset, the bus driver
+ * is expected to re-enumerate the bus. Returns EALREADY if the bus was already
+ * up before the call.
+ */
+#define MC_CMD_CDX_BUS_UP 0x5
+#define MC_CMD_CDX_BUS_UP_MSGSET 0x5
+
+/* MC_CMD_CDX_BUS_UP_IN msgrequest */
+#define MC_CMD_CDX_BUS_UP_IN_LEN 4
+/* Bus number to take out of reset, in range 0 to BUS_COUNT-1 */
+#define MC_CMD_CDX_BUS_UP_IN_BUS_OFST 0
+#define MC_CMD_CDX_BUS_UP_IN_BUS_LEN 4
+
+/* MC_CMD_CDX_BUS_UP_OUT msgresponse: The bus can now be enumerated. */
+#define MC_CMD_CDX_BUS_UP_OUT_LEN 0
+
/***********************************/
/*
* MC_CMD_CDX_DEVICE_RESET
diff --git a/drivers/cdx/controller/mcdi_functions.c b/drivers/cdx/controller/mcdi_functions.c
index 0158f26533dd..400fdc771104 100644
--- a/drivers/cdx/controller/mcdi_functions.c
+++ b/drivers/cdx/controller/mcdi_functions.c
@@ -124,6 +124,30 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
return 0;
}
+int cdx_mcdi_bus_enable(struct cdx_mcdi *cdx)
+{
+ MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_BUS_UP_IN_LEN);
+ int ret;
+
+ MCDI_SET_DWORD(inbuf, CDX_BUS_UP_IN_BUS, 0);
+ ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_BUS_UP, inbuf, sizeof(inbuf),
+ NULL, 0, NULL);
+
+ return ret;
+}
+
+int cdx_mcdi_bus_disable(struct cdx_mcdi *cdx)
+{
+ MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_BUS_DOWN_IN_LEN);
+ int ret;
+
+ MCDI_SET_DWORD(inbuf, CDX_BUS_DOWN_IN_BUS, 0);
+ ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_BUS_DOWN, inbuf, sizeof(inbuf),
+ NULL, 0, NULL);
+
+ return ret;
+}
+
int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num)
{
MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_RESET_IN_LEN);
diff --git a/drivers/cdx/controller/mcdi_functions.h b/drivers/cdx/controller/mcdi_functions.h
index 7440ace5539a..6d26b7cdc07c 100644
--- a/drivers/cdx/controller/mcdi_functions.h
+++ b/drivers/cdx/controller/mcdi_functions.h
@@ -47,6 +47,22 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
u8 bus_num, u8 dev_num,
struct cdx_dev_params *dev_params);
+/**
+ * cdx_mcdi_bus_enable - Enable CDX bus represented by bus_num
+ * @cdx: pointer to MCDI interface.
+ *
+ * Return: 0 on success, <0 on failure
+ */
+int cdx_mcdi_bus_enable(struct cdx_mcdi *cdx);
+
+/**
+ * cdx_mcdi_bus_disable - Disable CDX bus represented by bus_num
+ * @cdx: pointer to MCDI interface.
+ *
+ * Return: 0 on success, <0 on failure
+ */
+int cdx_mcdi_bus_disable(struct cdx_mcdi *cdx);
+
/**
* cdx_mcdi_reset_device - Reset cdx device represented by bus_num:dev_num
* @cdx: pointer to MCDI interface.
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index bead71b7bc73..5da0634ae4ee 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -28,6 +28,8 @@ struct cdx_device_config {
u8 type;
};
+typedef int (*cdx_bus_enable_cb)(struct cdx_controller *cdx, bool enable);
+
typedef int (*cdx_scan_cb)(struct cdx_controller *cdx);
typedef int (*cdx_dev_configure_cb)(struct cdx_controller *cdx,
@@ -49,11 +51,13 @@ typedef int (*cdx_dev_configure_cb)(struct cdx_controller *cdx,
/**
* struct cdx_ops - Callbacks supported by CDX controller.
+ * @enable: enable or disable bus on the controller
* @scan: scan the devices on the controller
* @dev_configure: configuration like reset, master_enable,
* msi_config etc for a CDX device
*/
struct cdx_ops {
+ cdx_bus_enable_cb enable;
cdx_scan_cb scan;
cdx_dev_configure_cb dev_configure;
};
@@ -63,12 +67,14 @@ struct cdx_ops {
* @dev: Linux device associated with the CDX controller.
* @priv: private data
* @id: Controller ID
+ * @enabled: state enabled/disabled
* @ops: CDX controller ops
*/
struct cdx_controller {
struct device *dev;
void *priv;
u32 id;
+ bool enabled;
struct cdx_ops *ops;
};
--
2.25.1
Add sysfs for reset_all entry which resets all the
devices on the CDX bus.
Co-developed-by: Puneet Gupta <[email protected]>
Signed-off-by: Puneet Gupta <[email protected]>
Co-developed-by: Nipun Gupta <[email protected]>
Signed-off-by: Nipun Gupta <[email protected]>
Signed-off-by: Abhijit Gangurde <[email protected]>
Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
Tested-by: Nikhil Agarwal <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-cdx | 11 ++++++++++
drivers/cdx/cdx.c | 29 +++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
index 0afa85b3c63b..d9e00058471d 100644
--- a/Documentation/ABI/testing/sysfs-bus-cdx
+++ b/Documentation/ABI/testing/sysfs-bus-cdx
@@ -22,6 +22,17 @@ Description:
# echo 0 > /sys/bus/cdx/enable
+What: /sys/bus/cdx/reset_all
+Date: July 2023
+Contact: [email protected]
+Description:
+ Writing y/1/on to this file resets all the devices
+ present on the CDX bus
+
+ For example::
+
+ # echo 1 > /sys/bus/cdx/reset_all
+
What: /sys/bus/cdx/devices/.../vendor
Date: March 2023
Contact: [email protected]
diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index 48c493a43491..4d20047b55bb 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -106,6 +106,11 @@ int cdx_dev_reset(struct device *dev)
}
EXPORT_SYMBOL_GPL(cdx_dev_reset);
+static int reset_cdx_device(struct device *dev, void *data)
+{
+ return cdx_dev_reset(dev);
+}
+
/**
* cdx_unregister_device - Unregister a CDX device
* @dev: CDX device
@@ -433,9 +438,33 @@ static ssize_t rescan_store(const struct bus_type *bus,
}
static BUS_ATTR_WO(rescan);
+static ssize_t reset_all_store(const struct bus_type *bus,
+ const char *buf, size_t count)
+{
+ bool val;
+ int ret;
+
+ if (kstrtobool(buf, &val) < 0)
+ return -EINVAL;
+
+ if (!val)
+ return -EINVAL;
+
+ /* Reset all the devices attached to cdx bus */
+ ret = bus_for_each_dev(bus, NULL, NULL, reset_cdx_device);
+ if (ret) {
+ pr_err("error in CDX bus reset\n");
+ return 0;
+ }
+
+ return count;
+}
+static BUS_ATTR_WO(reset_all);
+
static struct attribute *cdx_bus_attrs[] = {
&bus_attr_enable.attr,
&bus_attr_rescan.attr,
+ &bus_attr_reset_all.attr,
NULL,
};
ATTRIBUTE_GROUPS(cdx_bus);
--
2.25.1
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 <[email protected]>
> Signed-off-by: Nipun Gupta <[email protected]>
> Signed-off-by: Abhijit Gangurde <[email protected]>
> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> Tested-by: Nikhil Agarwal <[email protected]>
> ---
> 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: [email protected]
> +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: [email protected]
> 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
On Tue, Jul 11, 2023 at 05:40:26PM +0530, Abhijit Gangurde wrote:
> Resource files provides the basic MMIO regions info to the
> user-space. Also, resources<x> devices can be used to mmap the
> MMIO regions in the user-space.
>
> Co-developed-by: Puneet Gupta <[email protected]>
> Signed-off-by: Puneet Gupta <[email protected]>
> Co-developed-by: Nipun Gupta <[email protected]>
> Signed-off-by: Nipun Gupta <[email protected]>
> Signed-off-by: Abhijit Gangurde <[email protected]>
> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> Tested-by: Nikhil Agarwal <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> include/linux/cdx/cdx_bus.h | 10 ++
> 3 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index d9e00058471d..6ca47b6442ce 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -76,3 +76,18 @@ Description:
> For example::
>
> # echo 1 > /sys/bus/cdx/devices/.../remove
> +
> +What: /sys/bus/cdx/devices/.../resource
> +Date: July 2023
> +Contact: [email protected]
> +Description:
> + The resource file contains host addresses of CDX device
> + resources. Each line of the resource file describes a region
> + with start, end, and flag fields.
If you documented what this file looked like here, it would be obvious
that this is not an acceptable sysfs file in any sense of the word.
Please do so, and then fix the patch to not do that at all.
thanks,
greg k-h
On Tue, Jul 11, 2023 at 05:40:25PM +0530, Abhijit Gangurde wrote:
> Add sysfs for reset_all entry which resets all the
> devices on the CDX bus.
On all CDX busses, right? Why all? Why not per-bus?
>
> Co-developed-by: Puneet Gupta <[email protected]>
> Signed-off-by: Puneet Gupta <[email protected]>
> Co-developed-by: Nipun Gupta <[email protected]>
> Signed-off-by: Nipun Gupta <[email protected]>
> Signed-off-by: Abhijit Gangurde <[email protected]>
> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> Tested-by: Nikhil Agarwal <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-cdx | 11 ++++++++++
> drivers/cdx/cdx.c | 29 +++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index 0afa85b3c63b..d9e00058471d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -22,6 +22,17 @@ Description:
>
> # echo 0 > /sys/bus/cdx/enable
>
> +What: /sys/bus/cdx/reset_all
> +Date: July 2023
> +Contact: [email protected]
> +Description:
> + Writing y/1/on to this file resets all the devices
> + present on the CDX bus
> +
> + For example::
> +
> + # echo 1 > /sys/bus/cdx/reset_all
What does resetting a device mean will happen?
> +
> What: /sys/bus/cdx/devices/.../vendor
> Date: March 2023
> Contact: [email protected]
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index 48c493a43491..4d20047b55bb 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -106,6 +106,11 @@ int cdx_dev_reset(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(cdx_dev_reset);
>
> +static int reset_cdx_device(struct device *dev, void *data)
> +{
> + return cdx_dev_reset(dev);
> +}
> +
> /**
> * cdx_unregister_device - Unregister a CDX device
> * @dev: CDX device
> @@ -433,9 +438,33 @@ static ssize_t rescan_store(const struct bus_type *bus,
> }
> static BUS_ATTR_WO(rescan);
>
> +static ssize_t reset_all_store(const struct bus_type *bus,
> + const char *buf, size_t count)
> +{
> + bool val;
> + int ret;
> +
> + if (kstrtobool(buf, &val) < 0)
> + return -EINVAL;
> +
> + if (!val)
> + return -EINVAL;
> +
> + /* Reset all the devices attached to cdx bus */
> + ret = bus_for_each_dev(bus, NULL, NULL, reset_cdx_device);
No locking needed?
> + if (ret) {
> + pr_err("error in CDX bus reset\n");
What error? For what device? Put the error message in the reset
callback, not here, as you have no idea what device failed.
thanks,
greg k-h
[AMD Official Use Only - General]
> > Add sysfs for reset_all entry which resets all the
> > devices on the CDX bus.
>
> On all CDX busses, right? Why all? Why not per-bus?
>
This was on similar line to bus enable/disable. Would extend this to support per bus in v2.
>
> >
> > Co-developed-by: Puneet Gupta <[email protected]>
> > Signed-off-by: Puneet Gupta <[email protected]>
> > Co-developed-by: Nipun Gupta <[email protected]>
> > Signed-off-by: Nipun Gupta <[email protected]>
> > Signed-off-by: Abhijit Gangurde <[email protected]>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> [email protected]>
> > Tested-by: Nikhil Agarwal <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-cdx | 11 ++++++++++
> > drivers/cdx/cdx.c | 29 +++++++++++++++++++++++++
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> b/Documentation/ABI/testing/sysfs-bus-cdx
> > index 0afa85b3c63b..d9e00058471d 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > @@ -22,6 +22,17 @@ Description:
> >
> > # echo 0 > /sys/bus/cdx/enable
> >
> > +What: /sys/bus/cdx/reset_all
> > +Date: July 2023
> > +Contact: [email protected]
> > +Description:
> > + Writing y/1/on to this file resets all the devices
> > + present on the CDX bus
> > +
> > + For example::
> > +
> > + # echo 1 > /sys/bus/cdx/reset_all
>
> What does resetting a device mean will happen?
It would be same of pcie flr to the device. Would add more description in v2.
>
> > +
> > What: /sys/bus/cdx/devices/.../vendor
> > Date: March 2023
> > Contact: [email protected]
> > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > index 48c493a43491..4d20047b55bb 100644
> > --- a/drivers/cdx/cdx.c
> > +++ b/drivers/cdx/cdx.c
> > @@ -106,6 +106,11 @@ int cdx_dev_reset(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(cdx_dev_reset);
> >
> > +static int reset_cdx_device(struct device *dev, void *data)
> > +{
> > + return cdx_dev_reset(dev);
> > +}
> > +
> > /**
> > * cdx_unregister_device - Unregister a CDX device
> > * @dev: CDX device
> > @@ -433,9 +438,33 @@ static ssize_t rescan_store(const struct bus_type
> *bus,
> > }
> > static BUS_ATTR_WO(rescan);
> >
> > +static ssize_t reset_all_store(const struct bus_type *bus,
> > + const char *buf, size_t count)
> > +{
> > + bool val;
> > + int ret;
> > +
> > + if (kstrtobool(buf, &val) < 0)
> > + return -EINVAL;
> > +
> > + if (!val)
> > + return -EINVAL;
> > +
> > + /* Reset all the devices attached to cdx bus */
> > + ret = bus_for_each_dev(bus, NULL, NULL, reset_cdx_device);
>
> No locking needed?
Would protect this with a lock.
>
> > + if (ret) {
> > + pr_err("error in CDX bus reset\n");
>
> What error? For what device? Put the error message in the reset
> callback, not here, as you have no idea what device failed.
Would correct in v2.
Thanks,
Abhijit
[AMD Official Use Only - General]
> > Resource files provides the basic MMIO regions info to the
> > user-space. Also, resources<x> devices can be used to mmap the
> > MMIO regions in the user-space.
> >
> > Co-developed-by: Puneet Gupta <[email protected]>
> > Signed-off-by: Puneet Gupta <[email protected]>
> > Co-developed-by: Nipun Gupta <[email protected]>
> > Signed-off-by: Nipun Gupta <[email protected]>
> > Signed-off-by: Abhijit Gangurde <[email protected]>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> [email protected]>
> > Tested-by: Nikhil Agarwal <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > include/linux/cdx/cdx_bus.h | 10 ++
> > 3 files changed, 163 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> b/Documentation/ABI/testing/sysfs-bus-cdx
> > index d9e00058471d..6ca47b6442ce 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > @@ -76,3 +76,18 @@ Description:
> > For example::
> >
> > # echo 1 > /sys/bus/cdx/devices/.../remove
> > +
> > +What: /sys/bus/cdx/devices/.../resource
> > +Date: July 2023
> > +Contact: [email protected]
> > +Description:
> > + The resource file contains host addresses of CDX device
> > + resources. Each line of the resource file describes a region
> > + with start, end, and flag fields.
>
> If you documented what this file looked like here, it would be obvious
> that this is not an acceptable sysfs file in any sense of the word.
>
> Please do so, and then fix the patch to not do that at all.
Similar interface exist for pci and we intended to keep it same way. Could you please elaborate on this.
# cat /sys/bus/pci/devices/0000\:01\:00.0/resource
0x0000000092100000 0x00000000921fffff 0x000000000014220c
0x0000000000000000 0x0000000000000000 0x0000000000000000
Thanks,
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 <[email protected]>
> > Signed-off-by: Nipun Gupta <[email protected]>
> > Signed-off-by: Abhijit Gangurde <[email protected]>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> [email protected]>
> > Tested-by: Nikhil Agarwal <[email protected]>
> > ---
> > 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: [email protected]
> > +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: [email protected]
> > 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
On Wed, Jul 12, 2023 at 01:23:28PM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
>
> > > Resource files provides the basic MMIO regions info to the
> > > user-space. Also, resources<x> devices can be used to mmap the
> > > MMIO regions in the user-space.
> > >
> > > Co-developed-by: Puneet Gupta <[email protected]>
> > > Signed-off-by: Puneet Gupta <[email protected]>
> > > Co-developed-by: Nipun Gupta <[email protected]>
> > > Signed-off-by: Nipun Gupta <[email protected]>
> > > Signed-off-by: Abhijit Gangurde <[email protected]>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > [email protected]>
> > > Tested-by: Nikhil Agarwal <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > include/linux/cdx/cdx_bus.h | 10 ++
> > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > index d9e00058471d..6ca47b6442ce 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > @@ -76,3 +76,18 @@ Description:
> > > For example::
> > >
> > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > +
> > > +What: /sys/bus/cdx/devices/.../resource
> > > +Date: July 2023
> > > +Contact: [email protected]
> > > +Description:
> > > + The resource file contains host addresses of CDX device
> > > + resources. Each line of the resource file describes a region
> > > + with start, end, and flag fields.
> >
> > If you documented what this file looked like here, it would be obvious
> > that this is not an acceptable sysfs file in any sense of the word.
> >
> > Please do so, and then fix the patch to not do that at all.
>
> Similar interface exist for pci and we intended to keep it same way. Could you please elaborate on this.
>
> # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> 0x0000000000000000 0x0000000000000000 0x0000000000000000
Please don't propagate incorrect decisions in the past.
Why do you need all of these "resources" in userspace? What tool is
going to read and parse them and do something with them?
This really violates the "one value per file" sysfs rule, you are going
to have to have a huge reason why this is not applicable here, AND you
are going to have to document it very very well and get everyone to
agree with it.
thanks,
greg k-h
On Wed, Jul 12, 2023 at 01:21:13PM +0000, Gangurde, Abhijit wrote:
> [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 <[email protected]>
> > > Signed-off-by: Nipun Gupta <[email protected]>
> > > Signed-off-by: Abhijit Gangurde <[email protected]>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > [email protected]>
> > > Tested-by: Nikhil Agarwal <[email protected]>
> > > ---
> > > 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: [email protected]
> > > +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.
Ok, why would this not also be in the description for when you need to
look up what this file does? You will not be able to track it back to
the commit log very easily.
thanks,
greg k-h
[AMD Official Use Only - General]
> > > > Resource files provides the basic MMIO regions info to the
> > > > user-space. Also, resources<x> devices can be used to mmap the
> > > > MMIO regions in the user-space.
> > > >
> > > > Co-developed-by: Puneet Gupta <[email protected]>
> > > > Signed-off-by: Puneet Gupta <[email protected]>
> > > > Co-developed-by: Nipun Gupta <[email protected]>
> > > > Signed-off-by: Nipun Gupta <[email protected]>
> > > > Signed-off-by: Abhijit Gangurde <[email protected]>
> > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > > [email protected]>
> > > > Tested-by: Nikhil Agarwal <[email protected]>
> > > > ---
> > > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > > include/linux/cdx/cdx_bus.h | 10 ++
> > > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > index d9e00058471d..6ca47b6442ce 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > @@ -76,3 +76,18 @@ Description:
> > > > For example::
> > > >
> > > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > > +
> > > > +What: /sys/bus/cdx/devices/.../resource
> > > > +Date: July 2023
> > > > +Contact: [email protected]
> > > > +Description:
> > > > + The resource file contains host addresses of CDX device
> > > > + resources. Each line of the resource file describes a region
> > > > + with start, end, and flag fields.
> > >
> > > If you documented what this file looked like here, it would be obvious
> > > that this is not an acceptable sysfs file in any sense of the word.
> > >
> > > Please do so, and then fix the patch to not do that at all.
> >
> > Similar interface exist for pci and we intended to keep it same way. Could you
> please elaborate on this.
> >
> > # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> > 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> > 0x0000000000000000 0x0000000000000000 0x0000000000000000
>
> Please don't propagate incorrect decisions in the past.
>
> Why do you need all of these "resources" in userspace? What tool is
> going to read and parse them and do something with them?
>
> This really violates the "one value per file" sysfs rule, you are going
> to have to have a huge reason why this is not applicable here, AND you
> are going to have to document it very very well and get everyone to
> agree with it.
We don't have any strong reason apart from that this is being used by some
test and debug applications. Will drop this one now and revisit later by
complying to the specifications.
Thanks,
Abhijit
On Thu, Jul 13, 2023 at 05:36:25AM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
>
> > > > > Resource files provides the basic MMIO regions info to the
> > > > > user-space. Also, resources<x> devices can be used to mmap the
> > > > > MMIO regions in the user-space.
> > > > >
> > > > > Co-developed-by: Puneet Gupta <[email protected]>
> > > > > Signed-off-by: Puneet Gupta <[email protected]>
> > > > > Co-developed-by: Nipun Gupta <[email protected]>
> > > > > Signed-off-by: Nipun Gupta <[email protected]>
> > > > > Signed-off-by: Abhijit Gangurde <[email protected]>
> > > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > > > [email protected]>
> > > > > Tested-by: Nikhil Agarwal <[email protected]>
> > > > > ---
> > > > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > > > include/linux/cdx/cdx_bus.h | 10 ++
> > > > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > index d9e00058471d..6ca47b6442ce 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > @@ -76,3 +76,18 @@ Description:
> > > > > For example::
> > > > >
> > > > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > > > +
> > > > > +What: /sys/bus/cdx/devices/.../resource
> > > > > +Date: July 2023
> > > > > +Contact: [email protected]
> > > > > +Description:
> > > > > + The resource file contains host addresses of CDX device
> > > > > + resources. Each line of the resource file describes a region
> > > > > + with start, end, and flag fields.
> > > >
> > > > If you documented what this file looked like here, it would be obvious
> > > > that this is not an acceptable sysfs file in any sense of the word.
> > > >
> > > > Please do so, and then fix the patch to not do that at all.
> > >
> > > Similar interface exist for pci and we intended to keep it same way. Could you
> > please elaborate on this.
> > >
> > > # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> > > 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> > > 0x0000000000000000 0x0000000000000000 0x0000000000000000
> >
> > Please don't propagate incorrect decisions in the past.
> >
> > Why do you need all of these "resources" in userspace? What tool is
> > going to read and parse them and do something with them?
> >
> > This really violates the "one value per file" sysfs rule, you are going
> > to have to have a huge reason why this is not applicable here, AND you
> > are going to have to document it very very well and get everyone to
> > agree with it.
>
> We don't have any strong reason apart from that this is being used by some
> test and debug applications. Will drop this one now and revisit later by
> complying to the specifications.
If it's only for debug stuff, then use debugfs, that's what it is there
for.
thanks,
greg k-h