From: Frederic Barrat <fbarrat@linux.ibm.com> To: "Alastair D'Silva" <alastair@au1.ibm.com>, alastair@d-silva.org Cc: Andrew Donnellan <andrew.donnellan@au1.ibm.com>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v3 3/7] ocxl: Create a clear delineation between ocxl backend & frontend Date: Mon, 25 Mar 2019 16:11:39 +0100 [thread overview] Message-ID: <21fedf98-1b76-fc75-58d6-646e684f3e17@linux.ibm.com> (raw) In-Reply-To: <20190325054438.15022-4-alastair@au1.ibm.com> This is a huge patch, there are probably ways to split it in smaller pieces to make the review easier. However, considering how much time we've already invested discussing and reviewing it, it's with by me to keep it as is. The ref-counting and device management look good to me now. A few details below. > --- a/drivers/misc/ocxl/file.c > +++ b/drivers/misc/ocxl/file.c > @@ -17,12 +17,10 @@ static struct class *ocxl_class; > static struct mutex minors_idr_lock; > static struct idr minors_idr; > > -static struct ocxl_afu *find_and_get_afu(dev_t devno) > +static struct ocxl_file_info *find_file_info(dev_t devno) > { > - struct ocxl_afu *afu; > - int afu_minor; > + struct ocxl_file_info *info; > > - afu_minor = MINOR(devno); > /* > * We don't declare an RCU critical section here, as our AFU > * is protected by a reference counter on the device. By the time the > @@ -30,56 +28,52 @@ static struct ocxl_afu *find_and_get_afu(dev_t devno) > * the device is already at 0, so no user API will access that AFU and > * this function can't return it. > */ The comment is still true, but needs tuning. Something like: "We don't declare an RCU critical section here, as our info structure is protected by a reference counter on the device. By the time the info reference is removed from the idr, the ref count of the device is already at 0, so no user API will access the corresponding AFU and this function can't return it." > - afu = idr_find(&minors_idr, afu_minor); > - if (afu) > - ocxl_afu_get(afu); > - return afu; > + info = idr_find(&minors_idr, MINOR(devno)); > + return info; > } > > -static int allocate_afu_minor(struct ocxl_afu *afu) > +static int allocate_minor(struct ocxl_file_info *info) > { > int minor; > > mutex_lock(&minors_idr_lock); > - minor = idr_alloc(&minors_idr, afu, 0, OCXL_NUM_MINORS, GFP_KERNEL); > + minor = idr_alloc(&minors_idr, info, 0, OCXL_NUM_MINORS, GFP_KERNEL); > mutex_unlock(&minors_idr_lock); > return minor; > } > > -static void free_afu_minor(struct ocxl_afu *afu) > +static void free_minor(struct ocxl_file_info *info) > { > mutex_lock(&minors_idr_lock); > - idr_remove(&minors_idr, MINOR(afu->dev.devt)); > + idr_remove(&minors_idr, MINOR(info->dev.devt)); > mutex_unlock(&minors_idr_lock); > } > > static int afu_open(struct inode *inode, struct file *file) > { > - struct ocxl_afu *afu; > + struct ocxl_file_info *info; > struct ocxl_context *ctx; > int rc; > > pr_debug("%s for device %x\n", __func__, inode->i_rdev); > > - afu = find_and_get_afu(inode->i_rdev); > - if (!afu) > + info = find_file_info(inode->i_rdev); > + if (!info) > return -ENODEV; > > ctx = ocxl_context_alloc(); > if (!ctx) { > rc = -ENOMEM; > - goto put_afu; > + goto err; > } > > - rc = ocxl_context_init(ctx, afu, inode->i_mapping); > + rc = ocxl_context_init(ctx, info->afu, inode->i_mapping); > if (rc) > - goto put_afu; > + goto err; > file->private_data = ctx; > - ocxl_afu_put(afu); > return 0; > > -put_afu: > - ocxl_afu_put(afu); > +err: > return rc; The error path with goto is here useless. However, if ocxl_context_init() fails, the memory for the context is never released. You may consider either getting rid of ocxl_context_alloc(), which is just a simple wrapper around kzalloc(), or merging the allocation in ocxl_context_init(). It would impact the external API, but having 2 calls (alloc and init) feels like there's one too many. > +static int ocxl_file_make_visible(struct ocxl_afu *afu) > { > int rc; > + struct ocxl_file_info *info = ocxl_afu_get_private(afu); > > - cdev_init(&afu->cdev, &ocxl_afu_fops); > - rc = cdev_add(&afu->cdev, afu->dev.devt, 1); > + cdev_init(&info->cdev, &ocxl_afu_fops); > + rc = cdev_add(&info->cdev, info->dev.devt, 1); > if (rc) { > - dev_err(&afu->dev, "Unable to add afu char device: %d\n", rc); > + dev_err(&info->dev, "Unable to add afu char device: %d\n", rc); > return rc; > } > + > return 0; > } > > -void ocxl_destroy_cdev(struct ocxl_afu *afu) > +void ocxl_file_make_invisible(struct ocxl_afu *afu) This function is not called anywhere? > -void ocxl_unregister_afu(struct ocxl_afu *afu) > +void ocxl_file_unregister_afu(struct ocxl_afu *afu) > { > - free_afu_minor(afu); > + struct ocxl_file_info *info = ocxl_afu_get_private(afu); > + > + if (!info) > + return; > + So that's likely where we miss the "make invisible" call. However, is it enough to rely on the private data to be set on the AFU? > diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h > index 81086534dab5..e04e547df29e 100644 > --- a/drivers/misc/ocxl/ocxl_internal.h > +++ b/drivers/misc/ocxl/ocxl_internal.h > +/** > + * Free an AFU > + * > + * afu: The AFU to free > + */ > +void ocxl_free_afu(struct ocxl_afu *afu); This is obsolete and should go away. Fred
WARNING: multiple messages have this Message-ID (diff)
From: Frederic Barrat <fbarrat@linux.ibm.com> To: "Alastair D'Silva" <alastair@au1.ibm.com>, alastair@d-silva.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linuxppc-dev@lists.ozlabs.org, Arnd Bergmann <arnd@arndb.de>, Andrew Donnellan <andrew.donnellan@au1.ibm.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/7] ocxl: Create a clear delineation between ocxl backend & frontend Date: Mon, 25 Mar 2019 16:11:39 +0100 [thread overview] Message-ID: <21fedf98-1b76-fc75-58d6-646e684f3e17@linux.ibm.com> (raw) In-Reply-To: <20190325054438.15022-4-alastair@au1.ibm.com> This is a huge patch, there are probably ways to split it in smaller pieces to make the review easier. However, considering how much time we've already invested discussing and reviewing it, it's with by me to keep it as is. The ref-counting and device management look good to me now. A few details below. > --- a/drivers/misc/ocxl/file.c > +++ b/drivers/misc/ocxl/file.c > @@ -17,12 +17,10 @@ static struct class *ocxl_class; > static struct mutex minors_idr_lock; > static struct idr minors_idr; > > -static struct ocxl_afu *find_and_get_afu(dev_t devno) > +static struct ocxl_file_info *find_file_info(dev_t devno) > { > - struct ocxl_afu *afu; > - int afu_minor; > + struct ocxl_file_info *info; > > - afu_minor = MINOR(devno); > /* > * We don't declare an RCU critical section here, as our AFU > * is protected by a reference counter on the device. By the time the > @@ -30,56 +28,52 @@ static struct ocxl_afu *find_and_get_afu(dev_t devno) > * the device is already at 0, so no user API will access that AFU and > * this function can't return it. > */ The comment is still true, but needs tuning. Something like: "We don't declare an RCU critical section here, as our info structure is protected by a reference counter on the device. By the time the info reference is removed from the idr, the ref count of the device is already at 0, so no user API will access the corresponding AFU and this function can't return it." > - afu = idr_find(&minors_idr, afu_minor); > - if (afu) > - ocxl_afu_get(afu); > - return afu; > + info = idr_find(&minors_idr, MINOR(devno)); > + return info; > } > > -static int allocate_afu_minor(struct ocxl_afu *afu) > +static int allocate_minor(struct ocxl_file_info *info) > { > int minor; > > mutex_lock(&minors_idr_lock); > - minor = idr_alloc(&minors_idr, afu, 0, OCXL_NUM_MINORS, GFP_KERNEL); > + minor = idr_alloc(&minors_idr, info, 0, OCXL_NUM_MINORS, GFP_KERNEL); > mutex_unlock(&minors_idr_lock); > return minor; > } > > -static void free_afu_minor(struct ocxl_afu *afu) > +static void free_minor(struct ocxl_file_info *info) > { > mutex_lock(&minors_idr_lock); > - idr_remove(&minors_idr, MINOR(afu->dev.devt)); > + idr_remove(&minors_idr, MINOR(info->dev.devt)); > mutex_unlock(&minors_idr_lock); > } > > static int afu_open(struct inode *inode, struct file *file) > { > - struct ocxl_afu *afu; > + struct ocxl_file_info *info; > struct ocxl_context *ctx; > int rc; > > pr_debug("%s for device %x\n", __func__, inode->i_rdev); > > - afu = find_and_get_afu(inode->i_rdev); > - if (!afu) > + info = find_file_info(inode->i_rdev); > + if (!info) > return -ENODEV; > > ctx = ocxl_context_alloc(); > if (!ctx) { > rc = -ENOMEM; > - goto put_afu; > + goto err; > } > > - rc = ocxl_context_init(ctx, afu, inode->i_mapping); > + rc = ocxl_context_init(ctx, info->afu, inode->i_mapping); > if (rc) > - goto put_afu; > + goto err; > file->private_data = ctx; > - ocxl_afu_put(afu); > return 0; > > -put_afu: > - ocxl_afu_put(afu); > +err: > return rc; The error path with goto is here useless. However, if ocxl_context_init() fails, the memory for the context is never released. You may consider either getting rid of ocxl_context_alloc(), which is just a simple wrapper around kzalloc(), or merging the allocation in ocxl_context_init(). It would impact the external API, but having 2 calls (alloc and init) feels like there's one too many. > +static int ocxl_file_make_visible(struct ocxl_afu *afu) > { > int rc; > + struct ocxl_file_info *info = ocxl_afu_get_private(afu); > > - cdev_init(&afu->cdev, &ocxl_afu_fops); > - rc = cdev_add(&afu->cdev, afu->dev.devt, 1); > + cdev_init(&info->cdev, &ocxl_afu_fops); > + rc = cdev_add(&info->cdev, info->dev.devt, 1); > if (rc) { > - dev_err(&afu->dev, "Unable to add afu char device: %d\n", rc); > + dev_err(&info->dev, "Unable to add afu char device: %d\n", rc); > return rc; > } > + > return 0; > } > > -void ocxl_destroy_cdev(struct ocxl_afu *afu) > +void ocxl_file_make_invisible(struct ocxl_afu *afu) This function is not called anywhere? > -void ocxl_unregister_afu(struct ocxl_afu *afu) > +void ocxl_file_unregister_afu(struct ocxl_afu *afu) > { > - free_afu_minor(afu); > + struct ocxl_file_info *info = ocxl_afu_get_private(afu); > + > + if (!info) > + return; > + So that's likely where we miss the "make invisible" call. However, is it enough to rely on the private data to be set on the AFU? > diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h > index 81086534dab5..e04e547df29e 100644 > --- a/drivers/misc/ocxl/ocxl_internal.h > +++ b/drivers/misc/ocxl/ocxl_internal.h > +/** > + * Free an AFU > + * > + * afu: The AFU to free > + */ > +void ocxl_free_afu(struct ocxl_afu *afu); This is obsolete and should go away. Fred
next prev parent reply other threads:[~2019-03-25 15:11 UTC|newest] Thread overview: 156+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-27 4:57 [PATCH 0/5] ocxl: OpenCAPI Cleanup Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 4:57 ` [PATCH 1/5] ocxl: Rename struct link to ocxl_link Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 7:15 ` Andrew Donnellan 2019-02-27 7:15 ` Andrew Donnellan 2019-02-27 7:34 ` Alastair D'Silva 2019-02-27 7:34 ` Alastair D'Silva 2019-02-27 7:54 ` Andrew Donnellan 2019-02-27 7:54 ` Andrew Donnellan 2019-02-27 8:04 ` Alastair D'Silva 2019-02-27 8:04 ` Alastair D'Silva 2019-02-27 8:18 ` Andrew Donnellan 2019-02-27 8:18 ` Andrew Donnellan 2019-02-27 13:45 ` Frederic Barrat 2019-02-27 13:45 ` Frederic Barrat 2019-02-27 13:59 ` Greg Kurz 2019-02-27 13:59 ` Greg Kurz 2019-02-27 13:53 ` Greg Kurz 2019-02-27 13:53 ` Greg Kurz 2019-02-27 4:57 ` [PATCH 2/5] ocxl: Clean up printf formats Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 13:40 ` Frederic Barrat 2019-02-27 13:40 ` Frederic Barrat 2019-02-28 5:02 ` Andrew Donnellan 2019-02-28 5:02 ` Andrew Donnellan 2019-03-02 1:13 ` Joe Perches 2019-03-02 1:13 ` Joe Perches 2019-02-27 4:57 ` [PATCH 3/5] ocxl: read_pasid never returns an error, so make it void Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 13:25 ` Frederic Barrat 2019-02-27 13:25 ` Frederic Barrat 2019-02-28 5:03 ` Andrew Donnellan 2019-02-28 5:03 ` Andrew Donnellan 2019-02-27 4:57 ` [PATCH 4/5] ocxl: Remove superfluous 'extern' from headers Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 13:36 ` Frederic Barrat 2019-02-27 13:36 ` Frederic Barrat 2019-02-28 5:05 ` Andrew Donnellan 2019-02-28 5:05 ` Andrew Donnellan 2019-02-27 4:57 ` [PATCH 5/5] ocxl: Remove some unused exported symbols Alastair D'Silva 2019-02-27 4:57 ` Alastair D'Silva 2019-02-27 13:39 ` Frederic Barrat 2019-02-27 13:39 ` Frederic Barrat 2019-02-28 5:23 ` Andrew Donnellan 2019-02-28 5:23 ` Andrew Donnellan 2019-03-13 4:06 ` [PATCH v2 0/5] ocxl: OpenCAPI Cleanup Alastair D'Silva 2019-03-13 4:06 ` Alastair D'Silva 2019-03-13 4:06 ` [PATCH 1/5] ocxl: Rename struct link to ocxl_link Alastair D'Silva 2019-03-13 4:06 ` Alastair D'Silva 2019-03-15 6:58 ` Andrew Donnellan 2019-03-15 6:58 ` Andrew Donnellan 2019-03-13 4:06 ` [PATCH 2/5] ocxl: Clean up printf formats Alastair D'Silva 2019-03-13 4:06 ` Alastair D'Silva 2019-03-13 8:24 ` Greg Kurz 2019-03-14 4:58 ` Andrew Donnellan 2019-03-14 4:58 ` Andrew Donnellan 2019-03-13 4:06 ` [PATCH 3/5] ocxl: read_pasid never returns an error, so make it void Alastair D'Silva 2019-03-13 4:06 ` Alastair D'Silva 2019-03-14 4:59 ` Andrew Donnellan 2019-03-14 4:59 ` Andrew Donnellan 2019-03-13 4:07 ` [PATCH 4/5] ocxl: Remove superfluous 'extern' from headers Alastair D'Silva 2019-03-13 4:07 ` Alastair D'Silva 2019-03-13 8:28 ` Greg Kurz 2019-03-14 5:08 ` Andrew Donnellan 2019-03-14 5:08 ` Andrew Donnellan 2019-03-13 4:07 ` [PATCH 5/5] ocxl: Remove some unused exported symbols Alastair D'Silva 2019-03-13 4:07 ` Alastair D'Silva 2019-03-13 9:10 ` Greg Kurz 2019-03-14 2:23 ` Alastair D'Silva 2019-03-14 6:50 ` Greg Kurz 2019-03-15 4:49 ` Andrew Donnellan 2019-03-15 4:49 ` Andrew Donnellan 2019-03-15 5:07 ` Andrew Donnellan 2019-03-15 5:07 ` Andrew Donnellan 2019-03-20 5:34 ` [PATCH v3 0/5] ocxl: OpenCAPI Cleanup Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-20 5:34 ` [PATCH v3 1/5] ocxl: Rename struct link to ocxl_link Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-20 5:34 ` [PATCH v3 2/5] ocxl: Clean up printf formats Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-20 17:24 ` Joe Perches 2019-03-20 17:24 ` Joe Perches 2019-03-20 5:34 ` [PATCH v3 3/5] ocxl: read_pasid never returns an error, so make it void Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-20 5:34 ` [PATCH v3 4/5] ocxl: Remove superfluous 'extern' from headers Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-20 5:34 ` [PATCH v3 5/5] ocxl: Remove some unused exported symbols Alastair D'Silva 2019-03-20 5:34 ` Alastair D'Silva 2019-03-25 5:34 ` [PATCH v4 0/4] ocxl: OpenCAPI Cleanup Alastair D'Silva 2019-03-25 5:34 ` Alastair D'Silva 2019-03-25 5:34 ` [PATCH v4 1/4] ocxl: Rename struct link to ocxl_link Alastair D'Silva 2019-03-25 5:34 ` Alastair D'Silva 2019-04-03 14:18 ` Frederic Barrat 2019-04-03 14:18 ` Frederic Barrat 2019-04-05 7:05 ` Andrew Donnellan 2019-04-05 7:05 ` Andrew Donnellan 2019-05-03 6:59 ` Michael Ellerman 2019-03-25 5:34 ` [PATCH v4 2/4] ocxl: read_pasid never returns an error, so make it void Alastair D'Silva 2019-03-25 5:34 ` Alastair D'Silva 2019-04-03 14:20 ` Frederic Barrat 2019-04-03 14:20 ` Frederic Barrat 2019-04-05 7:05 ` Andrew Donnellan 2019-04-05 7:05 ` Andrew Donnellan 2019-03-25 5:34 ` [PATCH v4 3/4] ocxl: Remove superfluous 'extern' from headers Alastair D'Silva 2019-03-25 5:34 ` Alastair D'Silva 2019-03-25 16:55 ` Greg Kurz 2019-03-25 16:55 ` Greg Kurz 2019-04-03 14:20 ` Frederic Barrat 2019-04-03 14:20 ` Frederic Barrat 2019-04-05 7:09 ` Andrew Donnellan 2019-04-05 7:09 ` Andrew Donnellan 2019-03-25 5:34 ` [PATCH v4 4/4] ocxl: Remove some unused exported symbols Alastair D'Silva 2019-03-25 5:34 ` Alastair D'Silva 2019-03-25 16:57 ` Greg Kurz 2019-03-25 16:57 ` Greg Kurz 2019-04-03 14:23 ` Frederic Barrat 2019-04-03 14:23 ` Frederic Barrat 2019-04-05 7:28 ` Andrew Donnellan 2019-04-05 7:28 ` Andrew Donnellan 2019-03-25 16:49 ` [PATCH v4 0/4] ocxl: OpenCAPI Cleanup Greg Kurz 2019-03-25 16:49 ` Greg Kurz 2019-03-25 17:34 ` Frederic Barrat 2019-03-25 17:34 ` Frederic Barrat 2019-03-25 21:45 ` Alastair D'Silva 2019-03-25 21:45 ` Alastair D'Silva 2019-03-25 5:44 ` [PATCH v3 0/7] Refactor OCXL driver to allow external drivers to use it Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 5:44 ` [PATCH v3 1/7] ocxl: Split pci.c Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 10:01 ` Frederic Barrat 2019-03-25 10:01 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 2/7] ocxl: Don't pass pci_dev around Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 10:04 ` Frederic Barrat 2019-03-25 10:04 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 3/7] ocxl: Create a clear delineation between ocxl backend & frontend Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 15:11 ` Frederic Barrat [this message] 2019-03-25 15:11 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 4/7] ocxl: Allow external drivers to use OpenCAPI contexts Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 15:13 ` Frederic Barrat 2019-03-25 15:13 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 5/7] ocxl: afu_irq only deals with IRQ IDs, not offsets Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 15:24 ` Frederic Barrat 2019-03-25 15:24 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 6/7] ocxl: move event_fd handling to frontend Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 15:41 ` Frederic Barrat 2019-03-25 15:41 ` Frederic Barrat 2019-03-25 5:44 ` [PATCH v3 7/7] ocxl: Provide global MMIO accessors for external drivers Alastair D'Silva 2019-03-25 5:44 ` Alastair D'Silva 2019-03-25 15:49 ` Frederic Barrat 2019-03-25 15:49 ` Frederic Barrat
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=21fedf98-1b76-fc75-58d6-646e684f3e17@linux.ibm.com \ --to=fbarrat@linux.ibm.com \ --cc=alastair@au1.ibm.com \ --cc=alastair@d-silva.org \ --cc=andrew.donnellan@au1.ibm.com \ --cc=arnd@arndb.de \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.