| Message ID | 20190227045741.21412-2-alastair@au1.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ocxl: OpenCAPI Cleanup | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 8 checks, 143 lines checked |
On 27/2/19 3:57 pm, Alastair D'Silva wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > The term 'link' is ambiguous (especially when the struct is used for a > list), so rename it for clarity. > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > Reviewed-by: Greg Kurz <groug@kaod.org> > --- > drivers/misc/ocxl/file.c | 2 +- > drivers/misc/ocxl/link.c | 36 ++++++++++++++++++------------------ > 2 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c > index e6a607488f8a..16eb8a60d5c7 100644 > --- a/drivers/misc/ocxl/file.c > +++ b/drivers/misc/ocxl/file.c > @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx, > > if (status == ATTACHED) { > int rc; > - struct link *link = ctx->afu->fn->link; > + void *link = ctx->afu->fn->link; This doesn't look like a rename... > > rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr); > if (rc) > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c > index d50b861d7e57..8d2690a1a9de 100644 > --- a/drivers/misc/ocxl/link.c > +++ b/drivers/misc/ocxl/link.c > @@ -76,7 +76,7 @@ struct spa { > * limited number of opencapi slots on a system and lookup is only > * done when the device is probed > */ > -struct link { > +struct ocxl_link { > struct list_head list; > struct kref ref; > int domain; > @@ -179,7 +179,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work) > > static irqreturn_t xsl_fault_handler(int irq, void *data) > { > - struct link *link = (struct link *) data; > + struct ocxl_link *link = (struct ocxl_link *) data; > struct spa *spa = link->spa; > u64 dsisr, dar, pe_handle; > struct pe_data *pe_data; > @@ -256,7 +256,7 @@ static int map_irq_registers(struct pci_dev *dev, struct spa *spa) > &spa->reg_tfc, &spa->reg_pe_handle); > } > > -static int setup_xsl_irq(struct pci_dev *dev, struct link *link) > +static int setup_xsl_irq(struct pci_dev *dev, struct ocxl_link *link) > { > struct spa *spa = link->spa; > int rc; > @@ -311,7 +311,7 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link) > return rc; > } > > -static void release_xsl_irq(struct link *link) > +static void release_xsl_irq(struct ocxl_link *link) > { > struct spa *spa = link->spa; > > @@ -323,7 +323,7 @@ static void release_xsl_irq(struct link *link) > unmap_irq_registers(spa); > } > > -static int alloc_spa(struct pci_dev *dev, struct link *link) > +static int alloc_spa(struct pci_dev *dev, struct ocxl_link *link) > { > struct spa *spa; > > @@ -350,7 +350,7 @@ static int alloc_spa(struct pci_dev *dev, struct link *link) > return 0; > } > > -static void free_spa(struct link *link) > +static void free_spa(struct ocxl_link *link) > { > struct spa *spa = link->spa; > > @@ -364,12 +364,12 @@ static void free_spa(struct link *link) > } > } > > -static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) > +static int alloc_link(struct pci_dev *dev, int PE_mask, struct ocxl_link **out_link) > { > - struct link *link; > + struct ocxl_link *link; > int rc; > > - link = kzalloc(sizeof(struct link), GFP_KERNEL); > + link = kzalloc(sizeof(struct ocxl_link), GFP_KERNEL); > if (!link) > return -ENOMEM; > > @@ -405,7 +405,7 @@ static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) > return rc; > } > > -static void free_link(struct link *link) > +static void free_link(struct ocxl_link *link) > { > release_xsl_irq(link); > free_spa(link); > @@ -415,7 +415,7 @@ static void free_link(struct link *link) > int ocxl_link_setup(struct pci_dev *dev, int PE_mask, void **link_handle) > { > int rc = 0; > - struct link *link; > + struct ocxl_link *link; > > mutex_lock(&links_list_lock); > list_for_each_entry(link, &links_list, list) { > @@ -442,7 +442,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_setup); > > static void release_xsl(struct kref *ref) > { > - struct link *link = container_of(ref, struct link, ref); > + struct ocxl_link *link = container_of(ref, struct ocxl_link, ref); > > list_del(&link->list); > /* call platform code before releasing data */ > @@ -452,7 +452,7 @@ static void release_xsl(struct kref *ref) > > void ocxl_link_release(struct pci_dev *dev, void *link_handle) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > > mutex_lock(&links_list_lock); > kref_put(&link->ref, release_xsl); > @@ -488,7 +488,7 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr, > void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr), > void *xsl_err_data) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > int pe_handle, rc = 0; > @@ -558,7 +558,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_add_pe); > > int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > int pe_handle, rc; > @@ -594,7 +594,7 @@ int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) > > int ocxl_link_remove_pe(void *link_handle, int pasid) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > struct pe_data *pe_data; > @@ -666,7 +666,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_remove_pe); > > int ocxl_link_irq_alloc(void *link_handle, int *hw_irq, u64 *trigger_addr) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > int rc, irq; > u64 addr; > > @@ -687,7 +687,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_irq_alloc); > > void ocxl_link_free_irq(void *link_handle, int hw_irq) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > > pnv_ocxl_free_xive_irq(hw_irq); > atomic_inc(&link->irq_available); >
> -----Original Message----- > From: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Sent: Wednesday, 27 February 2019 6:16 PM > To: Alastair D'Silva <alastair@au1.ibm.com>; alastair@d-silva.org > Cc: Greg Kurz <groug@kaod.org>; Frederic Barrat <fbarrat@linux.ibm.com>; > Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link > > On 27/2/19 3:57 pm, Alastair D'Silva wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > > > The term 'link' is ambiguous (especially when the struct is used for a > > list), so rename it for clarity. > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > Reviewed-by: Greg Kurz <groug@kaod.org> > > --- > > drivers/misc/ocxl/file.c | 2 +- > > drivers/misc/ocxl/link.c | 36 ++++++++++++++++++------------------ > > 2 files changed, 19 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index > > e6a607488f8a..16eb8a60d5c7 100644 > > --- a/drivers/misc/ocxl/file.c > > +++ b/drivers/misc/ocxl/file.c > > @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct > > ocxl_context *ctx, > > > > if (status == ATTACHED) { > > int rc; > > - struct link *link = ctx->afu->fn->link; > > + void *link = ctx->afu->fn->link; > > This doesn't look like a rename... That corrects the type to what the member (and prototype for ocxl_link_update_pe) declare it as. The struct link there is bogus, it shouldn't even compile (since the intended struct link is defined in a different compilation unit), but instead picks up a different definition of 'struct link' from elsewhere.
On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index >>> e6a607488f8a..16eb8a60d5c7 100644 >>> --- a/drivers/misc/ocxl/file.c >>> +++ b/drivers/misc/ocxl/file.c >>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct >>> ocxl_context *ctx, >>> >>> if (status == ATTACHED) { >>> int rc; >>> - struct link *link = ctx->afu->fn->link; >>> + void *link = ctx->afu->fn->link; >> >> This doesn't look like a rename... > > That corrects the type to what the member (and prototype for ocxl_link_update_pe) declare it as. > > The struct link there is bogus, it shouldn't even compile (since the intended struct link is defined in a different compilation unit), but instead picks up a different definition of 'struct link' from elsewhere. > Given there's only a handful of struct links defined across the entire kernel, I'm going to guess that the definition it's picking up is in fact the ocxl one. I think the better solution here is to move struct ocxl_link into ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather than void *, and update the function signature for ocxl_link_update_pe() as well.
> -----Original Message----- > From: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Sent: Wednesday, 27 February 2019 6:55 PM > To: Alastair D'Silva <alastair@d-silva.org>; 'Alastair D'Silva' > <alastair@au1.ibm.com> > Cc: 'Greg Kurz' <groug@kaod.org>; 'Frederic Barrat' > <fbarrat@linux.ibm.com>; 'Arnd Bergmann' <arnd@arndb.de>; 'Greg Kroah- > Hartman' <gregkh@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link > > On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git > a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index > >>> e6a607488f8a..16eb8a60d5c7 100644 > >>> --- a/drivers/misc/ocxl/file.c > >>> +++ b/drivers/misc/ocxl/file.c > >>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct > >>> ocxl_context *ctx, > >>> > >>> if (status == ATTACHED) { > >>> int rc; > >>> - struct link *link = ctx->afu->fn->link; > >>> + void *link = ctx->afu->fn->link; > >> > >> This doesn't look like a rename... > > > > That corrects the type to what the member (and prototype for > ocxl_link_update_pe) declare it as. > > > > The struct link there is bogus, it shouldn't even compile (since the intended > struct link is defined in a different compilation unit), but instead picks up a > different definition of 'struct link' from elsewhere. > > > > Given there's only a handful of struct links defined across the entire kernel, > I'm going to guess that the definition it's picking up is in fact the ocxl one. > Unlikely, since that's never in a header. It wasn't caught since it was assigned to/from a void*. > I think the better solution here is to move struct ocxl_link into > ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather than void > *, and update the function signature for ocxl_link_update_pe() as well. Not move it, but we could have an opaque declaration there.
On 27/2/19 7:04 pm, Alastair D'Silva wrote: >> -----Original Message----- >> From: Andrew Donnellan <andrew.donnellan@au1.ibm.com> >> Sent: Wednesday, 27 February 2019 6:55 PM >> To: Alastair D'Silva <alastair@d-silva.org>; 'Alastair D'Silva' >> <alastair@au1.ibm.com> >> Cc: 'Greg Kurz' <groug@kaod.org>; 'Frederic Barrat' >> <fbarrat@linux.ibm.com>; 'Arnd Bergmann' <arnd@arndb.de>; 'Greg Kroah- >> Hartman' <gregkh@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link >> >> On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git >> a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index >>>>> e6a607488f8a..16eb8a60d5c7 100644 >>>>> --- a/drivers/misc/ocxl/file.c >>>>> +++ b/drivers/misc/ocxl/file.c >>>>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct >>>>> ocxl_context *ctx, >>>>> >>>>> if (status == ATTACHED) { >>>>> int rc; >>>>> - struct link *link = ctx->afu->fn->link; >>>>> + void *link = ctx->afu->fn->link; >>>> >>>> This doesn't look like a rename... >>> >>> That corrects the type to what the member (and prototype for >> ocxl_link_update_pe) declare it as. >>> >>> The struct link there is bogus, it shouldn't even compile (since the intended >> struct link is defined in a different compilation unit), but instead picks up a >> different definition of 'struct link' from elsewhere. >>> >> >> Given there's only a handful of struct links defined across the entire kernel, >> I'm going to guess that the definition it's picking up is in fact the ocxl one. >> > > Unlikely, since that's never in a header. It wasn't caught since it was assigned to/from a void*. Ah, yeah that'd explain it... and it's a pointer so it never needs to know its size. I'm clearly not very good at C. > >> I think the better solution here is to move struct ocxl_link into >> ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather than void >> *, and update the function signature for ocxl_link_update_pe() as well. > > Not move it, but we could have an opaque declaration there. > Putting it there would fit with all the other ocxl_* structs, but either way, we definitely need a declaration in there and get rid of the void*, t
Le 27/02/2019 à 09:18, Andrew Donnellan a écrit : > On 27/2/19 7:04 pm, Alastair D'Silva wrote: >>> -----Original Message----- >>> From: Andrew Donnellan <andrew.donnellan@au1.ibm.com> >>> Sent: Wednesday, 27 February 2019 6:55 PM >>> To: Alastair D'Silva <alastair@d-silva.org>; 'Alastair D'Silva' >>> <alastair@au1.ibm.com> >>> Cc: 'Greg Kurz' <groug@kaod.org>; 'Frederic Barrat' >>> <fbarrat@linux.ibm.com>; 'Arnd Bergmann' <arnd@arndb.de>; 'Greg Kroah- >>> Hartman' <gregkh@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org; >>> linux-kernel@vger.kernel.org >>> Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link >>> >>> On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git >>> a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index >>>>>> e6a607488f8a..16eb8a60d5c7 100644 >>>>>> --- a/drivers/misc/ocxl/file.c >>>>>> +++ b/drivers/misc/ocxl/file.c >>>>>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct >>>>>> ocxl_context *ctx, >>>>>> >>>>>> if (status == ATTACHED) { >>>>>> int rc; >>>>>> - struct link *link = ctx->afu->fn->link; >>>>>> + void *link = ctx->afu->fn->link; >>>>> >>>>> This doesn't look like a rename... >>>> >>>> That corrects the type to what the member (and prototype for >>> ocxl_link_update_pe) declare it as. >>>> >>>> The struct link there is bogus, it shouldn't even compile (since the >>>> intended >>> struct link is defined in a different compilation unit), but instead >>> picks up a >>> different definition of 'struct link' from elsewhere. >>>> >>> >>> Given there's only a handful of struct links defined across the >>> entire kernel, >>> I'm going to guess that the definition it's picking up is in fact the >>> ocxl one. >>> >> >> Unlikely, since that's never in a header. It wasn't caught since it >> was assigned to/from a void*. > > Ah, yeah that'd explain it... and it's a pointer so it never needs to > know its size. I'm clearly not very good at C. > >> >>> I think the better solution here is to move struct ocxl_link into >>> ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather >>> than void >>> *, and update the function signature for ocxl_link_update_pe() as well. >> Not move it, but we could have an opaque declaration there. >> > > Putting it there would fit with all the other ocxl_* structs, but either > way, we definitely need a declaration in there and get rid of the void*, t Mmm, it might turn out to be more invasive that planned... The point was only to have it as an opaque to the outside world, for APIs we'd like to deprecate at some point, so I wouldn't sweat too much over it. Fred
On Wed, 27 Feb 2019 15:57:37 +1100 "Alastair D'Silva" <alastair@au1.ibm.com> wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > The term 'link' is ambiguous (especially when the struct is used for a > list), so rename it for clarity. > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > Reviewed-by: Greg Kurz <groug@kaod.org> > --- > drivers/misc/ocxl/file.c | 2 +- > drivers/misc/ocxl/link.c | 36 ++++++++++++++++++------------------ > 2 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c > index e6a607488f8a..16eb8a60d5c7 100644 > --- a/drivers/misc/ocxl/file.c > +++ b/drivers/misc/ocxl/file.c > @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx, > > if (status == ATTACHED) { > int rc; > - struct link *link = ctx->afu->fn->link; > + void *link = ctx->afu->fn->link; > Thinking about that again, the link variable only has one user. Irrespectively of the discussion on adding a struct ocxl_link forward declaration in the private header and changing the API, link isn't needed in the first place... > rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr); ... and we could just do: rc = ocxl_link_update_pe(ctx->afu->fn->link, ctx->pasid, ctx->tidr); I guess it is acceptable to do this change "while here" since the patch is about dropping the ambiguous 'link' term. > if (rc) > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c > index d50b861d7e57..8d2690a1a9de 100644 > --- a/drivers/misc/ocxl/link.c > +++ b/drivers/misc/ocxl/link.c > @@ -76,7 +76,7 @@ struct spa { > * limited number of opencapi slots on a system and lookup is only > * done when the device is probed > */ > -struct link { > +struct ocxl_link { > struct list_head list; > struct kref ref; > int domain; > @@ -179,7 +179,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work) > > static irqreturn_t xsl_fault_handler(int irq, void *data) > { > - struct link *link = (struct link *) data; > + struct ocxl_link *link = (struct ocxl_link *) data; > struct spa *spa = link->spa; > u64 dsisr, dar, pe_handle; > struct pe_data *pe_data; > @@ -256,7 +256,7 @@ static int map_irq_registers(struct pci_dev *dev, struct spa *spa) > &spa->reg_tfc, &spa->reg_pe_handle); > } > > -static int setup_xsl_irq(struct pci_dev *dev, struct link *link) > +static int setup_xsl_irq(struct pci_dev *dev, struct ocxl_link *link) > { > struct spa *spa = link->spa; > int rc; > @@ -311,7 +311,7 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link) > return rc; > } > > -static void release_xsl_irq(struct link *link) > +static void release_xsl_irq(struct ocxl_link *link) > { > struct spa *spa = link->spa; > > @@ -323,7 +323,7 @@ static void release_xsl_irq(struct link *link) > unmap_irq_registers(spa); > } > > -static int alloc_spa(struct pci_dev *dev, struct link *link) > +static int alloc_spa(struct pci_dev *dev, struct ocxl_link *link) > { > struct spa *spa; > > @@ -350,7 +350,7 @@ static int alloc_spa(struct pci_dev *dev, struct link *link) > return 0; > } > > -static void free_spa(struct link *link) > +static void free_spa(struct ocxl_link *link) > { > struct spa *spa = link->spa; > > @@ -364,12 +364,12 @@ static void free_spa(struct link *link) > } > } > > -static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) > +static int alloc_link(struct pci_dev *dev, int PE_mask, struct ocxl_link **out_link) > { > - struct link *link; > + struct ocxl_link *link; > int rc; > > - link = kzalloc(sizeof(struct link), GFP_KERNEL); > + link = kzalloc(sizeof(struct ocxl_link), GFP_KERNEL); > if (!link) > return -ENOMEM; > > @@ -405,7 +405,7 @@ static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) > return rc; > } > > -static void free_link(struct link *link) > +static void free_link(struct ocxl_link *link) > { > release_xsl_irq(link); > free_spa(link); > @@ -415,7 +415,7 @@ static void free_link(struct link *link) > int ocxl_link_setup(struct pci_dev *dev, int PE_mask, void **link_handle) > { > int rc = 0; > - struct link *link; > + struct ocxl_link *link; > > mutex_lock(&links_list_lock); > list_for_each_entry(link, &links_list, list) { > @@ -442,7 +442,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_setup); > > static void release_xsl(struct kref *ref) > { > - struct link *link = container_of(ref, struct link, ref); > + struct ocxl_link *link = container_of(ref, struct ocxl_link, ref); > > list_del(&link->list); > /* call platform code before releasing data */ > @@ -452,7 +452,7 @@ static void release_xsl(struct kref *ref) > > void ocxl_link_release(struct pci_dev *dev, void *link_handle) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > > mutex_lock(&links_list_lock); > kref_put(&link->ref, release_xsl); > @@ -488,7 +488,7 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr, > void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr), > void *xsl_err_data) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > int pe_handle, rc = 0; > @@ -558,7 +558,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_add_pe); > > int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > int pe_handle, rc; > @@ -594,7 +594,7 @@ int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) > > int ocxl_link_remove_pe(void *link_handle, int pasid) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > struct spa *spa = link->spa; > struct ocxl_process_element *pe; > struct pe_data *pe_data; > @@ -666,7 +666,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_remove_pe); > > int ocxl_link_irq_alloc(void *link_handle, int *hw_irq, u64 *trigger_addr) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > int rc, irq; > u64 addr; > > @@ -687,7 +687,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_irq_alloc); > > void ocxl_link_free_irq(void *link_handle, int hw_irq) > { > - struct link *link = (struct link *) link_handle; > + struct ocxl_link *link = (struct ocxl_link *) link_handle; > > pnv_ocxl_free_xive_irq(hw_irq); > atomic_inc(&link->irq_available);
On Wed, 27 Feb 2019 14:45:44 +0100 Frederic Barrat <fbarrat@linux.ibm.com> wrote: > Le 27/02/2019 à 09:18, Andrew Donnellan a écrit : > > On 27/2/19 7:04 pm, Alastair D'Silva wrote: > >>> -----Original Message----- > >>> From: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > >>> Sent: Wednesday, 27 February 2019 6:55 PM > >>> To: Alastair D'Silva <alastair@d-silva.org>; 'Alastair D'Silva' > >>> <alastair@au1.ibm.com> > >>> Cc: 'Greg Kurz' <groug@kaod.org>; 'Frederic Barrat' > >>> <fbarrat@linux.ibm.com>; 'Arnd Bergmann' <arnd@arndb.de>; 'Greg Kroah- > >>> Hartman' <gregkh@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org; > >>> linux-kernel@vger.kernel.org > >>> Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link > >>> > >>> On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git > >>> a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index > >>>>>> e6a607488f8a..16eb8a60d5c7 100644 > >>>>>> --- a/drivers/misc/ocxl/file.c > >>>>>> +++ b/drivers/misc/ocxl/file.c > >>>>>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct > >>>>>> ocxl_context *ctx, > >>>>>> > >>>>>> if (status == ATTACHED) { > >>>>>> int rc; > >>>>>> - struct link *link = ctx->afu->fn->link; > >>>>>> + void *link = ctx->afu->fn->link; > >>>>> > >>>>> This doesn't look like a rename... > >>>> > >>>> That corrects the type to what the member (and prototype for > >>> ocxl_link_update_pe) declare it as. > >>>> > >>>> The struct link there is bogus, it shouldn't even compile (since the > >>>> intended > >>> struct link is defined in a different compilation unit), but instead > >>> picks up a > >>> different definition of 'struct link' from elsewhere. > >>>> > >>> > >>> Given there's only a handful of struct links defined across the > >>> entire kernel, > >>> I'm going to guess that the definition it's picking up is in fact the > >>> ocxl one. > >>> > >> > >> Unlikely, since that's never in a header. It wasn't caught since it > >> was assigned to/from a void*. > > > > Ah, yeah that'd explain it... and it's a pointer so it never needs to > > know its size. I'm clearly not very good at C. > > > >> > >>> I think the better solution here is to move struct ocxl_link into > >>> ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather > >>> than void > >>> *, and update the function signature for ocxl_link_update_pe() as well. > >> Not move it, but we could have an opaque declaration there. > >> > > > > Putting it there would fit with all the other ocxl_* structs, but either > > way, we definitely need a declaration in there and get rid of the void*, t > > > Mmm, it might turn out to be more invasive that planned... > The point was only to have it as an opaque to the outside world, for > APIs we'd like to deprecate at some point, so I wouldn't sweat too much > over it. > I concur. And even if an API change turns out to be beneficial, it clearly belongs to some other patch/series. Hence my proposal to simply drop that controversial and unneeded link variable in afu_ioctl_enable_p9_wait(). > Fred >
diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index e6a607488f8a..16eb8a60d5c7 100644 --- a/drivers/misc/ocxl/file.c +++ b/drivers/misc/ocxl/file.c @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx, if (status == ATTACHED) { int rc; - struct link *link = ctx->afu->fn->link; + void *link = ctx->afu->fn->link; rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr); if (rc) diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c index d50b861d7e57..8d2690a1a9de 100644 --- a/drivers/misc/ocxl/link.c +++ b/drivers/misc/ocxl/link.c @@ -76,7 +76,7 @@ struct spa { * limited number of opencapi slots on a system and lookup is only * done when the device is probed */ -struct link { +struct ocxl_link { struct list_head list; struct kref ref; int domain; @@ -179,7 +179,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work) static irqreturn_t xsl_fault_handler(int irq, void *data) { - struct link *link = (struct link *) data; + struct ocxl_link *link = (struct ocxl_link *) data; struct spa *spa = link->spa; u64 dsisr, dar, pe_handle; struct pe_data *pe_data; @@ -256,7 +256,7 @@ static int map_irq_registers(struct pci_dev *dev, struct spa *spa) &spa->reg_tfc, &spa->reg_pe_handle); } -static int setup_xsl_irq(struct pci_dev *dev, struct link *link) +static int setup_xsl_irq(struct pci_dev *dev, struct ocxl_link *link) { struct spa *spa = link->spa; int rc; @@ -311,7 +311,7 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link) return rc; } -static void release_xsl_irq(struct link *link) +static void release_xsl_irq(struct ocxl_link *link) { struct spa *spa = link->spa; @@ -323,7 +323,7 @@ static void release_xsl_irq(struct link *link) unmap_irq_registers(spa); } -static int alloc_spa(struct pci_dev *dev, struct link *link) +static int alloc_spa(struct pci_dev *dev, struct ocxl_link *link) { struct spa *spa; @@ -350,7 +350,7 @@ static int alloc_spa(struct pci_dev *dev, struct link *link) return 0; } -static void free_spa(struct link *link) +static void free_spa(struct ocxl_link *link) { struct spa *spa = link->spa; @@ -364,12 +364,12 @@ static void free_spa(struct link *link) } } -static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) +static int alloc_link(struct pci_dev *dev, int PE_mask, struct ocxl_link **out_link) { - struct link *link; + struct ocxl_link *link; int rc; - link = kzalloc(sizeof(struct link), GFP_KERNEL); + link = kzalloc(sizeof(struct ocxl_link), GFP_KERNEL); if (!link) return -ENOMEM; @@ -405,7 +405,7 @@ static int alloc_link(struct pci_dev *dev, int PE_mask, struct link **out_link) return rc; } -static void free_link(struct link *link) +static void free_link(struct ocxl_link *link) { release_xsl_irq(link); free_spa(link); @@ -415,7 +415,7 @@ static void free_link(struct link *link) int ocxl_link_setup(struct pci_dev *dev, int PE_mask, void **link_handle) { int rc = 0; - struct link *link; + struct ocxl_link *link; mutex_lock(&links_list_lock); list_for_each_entry(link, &links_list, list) { @@ -442,7 +442,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_setup); static void release_xsl(struct kref *ref) { - struct link *link = container_of(ref, struct link, ref); + struct ocxl_link *link = container_of(ref, struct ocxl_link, ref); list_del(&link->list); /* call platform code before releasing data */ @@ -452,7 +452,7 @@ static void release_xsl(struct kref *ref) void ocxl_link_release(struct pci_dev *dev, void *link_handle) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; mutex_lock(&links_list_lock); kref_put(&link->ref, release_xsl); @@ -488,7 +488,7 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr, void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr), void *xsl_err_data) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; struct spa *spa = link->spa; struct ocxl_process_element *pe; int pe_handle, rc = 0; @@ -558,7 +558,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_add_pe); int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; struct spa *spa = link->spa; struct ocxl_process_element *pe; int pe_handle, rc; @@ -594,7 +594,7 @@ int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) int ocxl_link_remove_pe(void *link_handle, int pasid) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; struct spa *spa = link->spa; struct ocxl_process_element *pe; struct pe_data *pe_data; @@ -666,7 +666,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_remove_pe); int ocxl_link_irq_alloc(void *link_handle, int *hw_irq, u64 *trigger_addr) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; int rc, irq; u64 addr; @@ -687,7 +687,7 @@ EXPORT_SYMBOL_GPL(ocxl_link_irq_alloc); void ocxl_link_free_irq(void *link_handle, int hw_irq) { - struct link *link = (struct link *) link_handle; + struct ocxl_link *link = (struct ocxl_link *) link_handle; pnv_ocxl_free_xive_irq(hw_irq); atomic_inc(&link->irq_available);