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

[Frederic Barrat]

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@xxxxxxxxxxx>
> > > Sent: Wednesday, 27 February 2019 6:55 PM
> > > To: Alastair D'Silva <alastair@xxxxxxxxxxx>; 'Alastair D'Silva'
> > > <alastair@xxxxxxxxxxx>
> > > Cc: 'Greg Kurz' <groug@xxxxxxxx>; 'Frederic Barrat'
> > > <fbarrat@xxxxxxxxxxxxx>; 'Arnd Bergmann' <arnd@xxxxxxxx>; 'Greg Kroah-
> > > Hartman' <gregkh@xxxxxxxxxxxxxxxxxxx>; linuxppc-dev@xxxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx
> > > 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