Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link

[Greg Kurz <groug@kaod.org>]

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
>