Re: [PATCH v2 2/3] edac: Add L3/SoC support to the APM X-Gene SoC EDAC driver

Hi Borislav,

quoted

+
+static void xgene_edac_l3_hw_init(struct edac_device_ctl_info *edac_dev,
+                               bool enable)

arg alignment. It should be

function name(arg1, arg2,
              arg3, arg4,
              arg5...

Check your other functions too.

I will check and fix other functions if any. But this function is just
fine. It may be off just by the + extra character with patch
generation only.

quoted

+{
+     struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+     struct dentry *edac_debugfs;
+     char name[30];
+
+     if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
+             return;
+
+     /*
+      * Todo: Switch to common EDAC debug file system for edac device
+      *       when available.
+      */

What is that?

Debug folder node shows up at /sys/kernel/debug/<file here> while the
MC debug node shows up at /sys/kernel/debug/edac where <file here> is
the driver node name. It would be better if everything shows up at
/sys/kernel/debug/edac. For this to happen, I need EDAC core change to
provide an parent node.

quoted

+     if (!ctx->edac->dfs) {
+             ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
+                                                 NULL);
+             if (!ctx->edac->dfs)
+                     return;
+     }
+     sprintf(name, "l3c%d", ctx->edac_idx);
+     edac_debugfs = debugfs_create_dir(name, ctx->edac->dfs);
+     if (!edac_debugfs)
+             return;
+
+     debugfs_create_file("l3_inject_ctrl", S_IWUSR, edac_debugfs, edac_dev,
+                         &xgene_edac_l3_debug_inject_fops);
+}
+
+static int xgene_edac_l3_add(struct xgene_edac *edac, struct device_node *np,
+                          int version)
+{
+     struct edac_device_ctl_info *edac_dev;
+     struct xgene_edac_dev_ctx *ctx;
+     struct resource res;
+     void __iomem *dev_csr;
+     int edac_idx;
+     int rc = 0;
+
+     if (!devres_open_group(edac->dev, xgene_edac_l3_add, GFP_KERNEL))
+             return -ENOMEM;
+
+     rc = of_address_to_resource(np, 0, &res);
+     if (rc < 0) {
+             dev_err(edac->dev, "no L3 resource address\n");
+             goto err;
+     }
+     dev_csr = devm_ioremap_resource(edac->dev, &res);
+     if (IS_ERR(dev_csr)) {
+             dev_err(edac->dev,
+                     "devm_ioremap_resource failed for L3 resource address\n");
+             rc = PTR_ERR(dev_csr);
+             goto err;
+     }

+     edac_idx = edac_device_alloc_index();
+     edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
+                                           "l3c", 1, "l3c", 1, 0, NULL, 0,
+                                           edac_idx);
+     if (!edac_dev) {
+             rc = -ENOMEM;
+             goto err;
+     }
+
+     ctx = edac_dev->pvt_info;
+     ctx->dev_csr = dev_csr;
+     ctx->name = "xgene_l3_err";
+     ctx->edac_idx = edac_idx;
+     ctx->edac = edac;
+     ctx->edac_dev = edac_dev;
+     ctx->ddev = *edac->dev;
+     ctx->version = version;
+     edac_dev->dev = &ctx->ddev;
+     edac_dev->ctl_name = ctx->name;
+     edac_dev->dev_name = ctx->name;
+     edac_dev->mod_name = EDAC_MOD_STR;
+
+     if (edac_op_state == EDAC_OPSTATE_POLL)
+             edac_dev->edac_check = xgene_edac_l3_check;

So depending on what SoC elements you detect, the last one's check
function will win?

If xgene_edac loads on multiple SoC elements, you need a global xgene
check function for which l3, soc, mc, etc register...

No... each EDAC instance has its own context as we allocated via
function edac_device_alloc_ctl_info. Per type of instance will use the
same type function.

quoted

+
+static const char * const *xgene_edac_soc_mem_data(
+     struct xgene_edac_dev_ctx *ctx)
+{
+     switch (ctx->version) {
+     case 1:
+             return soc_mem_err_v1;
+     default:
+             return NULL;
+     }
+}

That's one badly formatted function used only once. Just move the logic
there and kill it.

For format, code looks fine. In any case, I will roll this into the code.

quoted

+static int xgene_edac_soc_add(struct xgene_edac *edac, struct device_node *np,
+                           int version)
+{
+     struct edac_device_ctl_info *edac_dev;
+     struct xgene_edac_dev_ctx *ctx;
+     void __iomem *dev_csr;
+     struct resource res;
+     int edac_idx;
+     int rc = 0;
+
+     if (!devres_open_group(edac->dev, xgene_edac_soc_add, GFP_KERNEL))
+             return -ENOMEM;
+
+     rc = of_address_to_resource(np, 0, &res);
+     if (rc < 0) {
+             dev_err(edac->dev, "no SoC resource address\n");
+             goto err;
+     }
+     dev_csr = devm_ioremap_resource(edac->dev, &res);
+     if (IS_ERR(dev_csr)) {
+             dev_err(edac->dev,
+                     "devm_ioremap_resource failed for soc resource address\n");
+             rc = PTR_ERR(dev_csr);
+             goto err;
+     }
+
+     edac_idx = edac_device_alloc_index();
+     edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
+                                           "SOC", 1, "SOC", 1, 2, NULL, 0,
+                                           edac_idx);
+     if (!edac_dev) {
+             rc = -ENOMEM;
+             goto err;
+     }
+
+     ctx = edac_dev->pvt_info;
+     ctx->dev_csr = dev_csr;
+     ctx->name = "xgene_soc_err";
+     ctx->edac_idx = edac_idx;
+     ctx->edac = edac;
+     ctx->edac_dev = edac_dev;
+     ctx->ddev = *edac->dev;
+     ctx->version = version;
+     edac_dev->dev = &ctx->ddev;
+     edac_dev->ctl_name = ctx->name;
+     edac_dev->dev_name = ctx->name;
+     edac_dev->mod_name = EDAC_MOD_STR;
+
+     if (edac_op_state == EDAC_OPSTATE_POLL)
+             edac_dev->edac_check = xgene_edac_soc_check;

Same here about the ->edac_check function.

Same comment as above.

-Loc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html