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

[Andrew Donnellan]

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

--
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@xxxxxxxxxxx  IBM Australia Limited