>From 73a937ce583964213f945a60454a3d3666ce098f Mon Sep 17 00:00:00 2001 From: Frederic Barrat Date: Fri, 22 Mar 2019 18:29:34 +0100 Subject: [PATCH] Review comments --- 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