On Wed, 27 Feb 2019 14:45:44 +0100
Frederic Barrat <fbar...@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.donnel...@au1.ibm.com>
> >>> Sent: Wednesday, 27 February 2019 6:55 PM
> >>> To: Alastair D'Silva <alast...@d-silva.org>; 'Alastair D'Silva'
> >>> <alast...@au1.ibm.com>
> >>> Cc: 'Greg Kurz' <gr...@kaod.org>; 'Frederic Barrat'
> >>> <fbar...@linux.ibm.com>; 'Arnd Bergmann' <a...@arndb.de>; 'Greg Kroah-
> >>> Hartman' <gre...@linuxfoundation.org>; linuxppc-dev@lists.ozlabs.org;
> >>> linux-ker...@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
>
