[PATCH] Review comments

From: Frederic Barrat
Date: Fri Mar 22 2019 - 13:29:34 EST

---
drivers/misc/ocxl/core.c | 8 +++--
drivers/misc/ocxl/file.c | 60 ++++++++++++++++++++++---------
drivers/misc/ocxl/ocxl_internal.h | 1 +
drivers/misc/ocxl/pci.c | 20 +++++++++++
4 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c
index c632ec372342..87dedaaddd4f 100644
--- a/drivers/misc/ocxl/core.c
+++ b/drivers/misc/ocxl/core.c
@@ -30,7 +30,11 @@ static struct ocxl_afu *alloc_afu(struct ocxl_fn *fn)
return afu;
}

-static void afu_release(struct kref *kref)
+static void free_afu(struct kref *kref)
+// fxb: I'm trying to keep some kind of naming logic:
+// alloc_afu/free_afu
+// we also have
+// alloc_function/free_function
{
struct ocxl_afu *afu = container_of(kref, struct ocxl_afu, kref);

@@ -47,7 +51,7 @@ EXPORT_SYMBOL_GPL(ocxl_afu_get);

void ocxl_afu_put(struct ocxl_afu *afu)
{
- kref_put(&afu->kref, afu_release);
+ kref_put(&afu->kref, free_afu);
}
EXPORT_SYMBOL_GPL(ocxl_afu_put);

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index 42214b0c956a..b17c2bebc127 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c
@@ -490,21 +490,18 @@ static const struct file_operations ocxl_afu_fops = {
.release = afu_release,
};

-// Called when there are no more consumers of the AFU
-static void ocxl_file_release(void *private)
-{
- struct ocxl_file_info *info = private;
-
- ocxl_sysfs_unregister_afu(info->afu);
- device_unregister(&info->dev);
-
- free_minor(info);
-}
-
// Free the info struct
static void info_release(struct device *dev)
{
struct ocxl_file_info *info = container_of(dev, struct ocxl_file_info, dev);
+// fxb: we don't need 2 functions to release/free the info
+// structure. Only one, called when the info device is released. So
+// I'm merging the content of ocxl_file_release. When info is
+// released, the afu ref count is decremented, which will free the afu
+// structure, which in turn, will end up freeing the function
+// structure
+ ocxl_afu_put(info->afu);
+ free_minor(info);
kfree(info);
}

@@ -520,8 +517,6 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)
if (info == NULL)
return -ENOMEM;

- info->afu = afu;
-
minor = allocate_minor(info);
if (minor < 0) {
kfree(info);
@@ -533,18 +528,44 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)
info->dev.class = ocxl_class;
info->dev.release = info_release;

- ocxl_afu_set_private(afu, info, ocxl_file_release);
+ info->afu = afu;
+ ocxl_afu_get(afu);
+// fxb: we store a reference to the AFU in the info structure, so we
+// increment the ref count of the AFU to make sure it's not going away
+
+ ocxl_afu_set_private(afu, info, NULL); // fxb: release callback still needed?

rc = dev_set_name(&info->dev, "%s.%s.%hhu",
afu->config.name, dev_name(&pci_dev->dev), afu->config.idx);
if (rc)
- return rc;
+ goto err_put;

rc = device_register(&info->dev);
- if (rc)
- return rc;
+ if (rc) {
+ put_device(&info->dev);
+ goto err_put;
+ }

return ocxl_sysfs_register_afu(afu);
+
+err_put:
+ ocxl_afu_put(afu);
+ free_minor(info);
+ kfree(info);
+ return rc;
+}
+
+void ocxl_file_unregister_afu(struct ocxl_afu *afu)
+{
+ struct ocxl_file_info *info = ocxl_afu_get_private(afu);
+
+// fxb: ocxl_sysfs_unregister_afu() cannot be called from the info
+// device release callback, as it still needs the struct device. Same
+// reason we had to call ocxl_sysfs_register_afu() after the device is
+// registered.
+
+ ocxl_sysfs_unregister_afu(afu);
+ device_unregister(&info->dev);
}

int ocxl_file_make_visible(struct ocxl_afu *afu)
@@ -552,6 +573,11 @@ int ocxl_file_make_visible(struct ocxl_afu *afu)
int rc;
struct ocxl_file_info *info = ocxl_afu_get_private(afu);

+ /*
+ * fxb: couldn't it be merged with ocxl_file_register_afu()? The
+ * requirement is that it must be done last, but it could still
+ * be achieved in ocxl_file_register_afu().
+ */
cdev_init(&info->cdev, &ocxl_afu_fops);
rc = cdev_add(&info->cdev, info->dev.devt, 1);
if (rc) {
diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
index f3775223f4b1..71780e86def8 100644
--- a/drivers/misc/ocxl/ocxl_internal.h
+++ b/drivers/misc/ocxl/ocxl_internal.h
@@ -99,6 +99,7 @@ struct ocxl_process_element {
int ocxl_create_cdev(struct ocxl_afu *afu);
void ocxl_destroy_cdev(struct ocxl_afu *afu);
int ocxl_file_register_afu(struct ocxl_afu *afu);
+void ocxl_file_unregister_afu(struct ocxl_afu *afu);
int ocxl_file_make_visible(struct ocxl_afu *afu);
void ocxl_file_make_invisible(struct ocxl_afu *afu);

diff --git a/drivers/misc/ocxl/pci.c b/drivers/misc/ocxl/pci.c
index a1ed2bb02e4b..329253b5c54a 100644
--- a/drivers/misc/ocxl/pci.c
+++ b/drivers/misc/ocxl/pci.c
@@ -39,6 +39,13 @@ static int ocxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
continue;

// Defer error cleanup until the device is removed
+ // fxb: this is actually dangerous. It means that in
+ // ocxl_file_unregister_afu() and
+ // ocxl_file_make_invisible(), we'll try to undo
+ // things which were not done int the first place, if
+ // something failed during init. It will likely
+ // generate a bunch of errors in ocxl_remove and even
+ // lead to kernel oops with some bad luck
}

return 0;
@@ -54,6 +61,19 @@ void ocxl_remove(struct pci_dev *dev)
afu_list = ocxl_function_afu_list(fn);

list_for_each_entry(afu, afu_list, list) {
+// fxb: the release of the info structure was convoluted. It's simpler
+// to keep things here symmetric with what we do in probe, so
+// ocxl_file_register_afu() needs a matching
+// ocxl_file_unregister_afu(). It also gets rid of the callback on the
+// private data. Though you may still need that for the external
+// driver, I don't know.
+
+// fxb: see comment in probe about calling those functions if we had
+// hit an error patch during probe, there's more work needed (and the
+// problem is not due to the new ocxl_file_unregister_afu(), it was
+// also there before when calling the callback in
+// ocxl_function_close()
+ ocxl_file_unregister_afu(afu);
ocxl_file_make_invisible(afu);
}

--
2.19.1


--------------71BD02764C9DB762A469E090--