linux-kernel - Re: [PATCH 1/5] ocxl: Rename struct link to ocxl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190227145911.60859bbb@bahia.lan>
Date:   Wed, 27 Feb 2019 14:59:11 +0100
From:   Greg Kurz <groug@...d.org>
To:     Frederic Barrat <fbarrat@...ux.ibm.com>
Cc:     Andrew Donnellan <andrew.donnellan@....ibm.com>,
        "Alastair D'Silva" <alastair@...ilva.org>,
        "'Alastair D'Silva'" <alastair@....ibm.com>,
        "'Arnd Bergmann'" <arnd@...db.de>,
        "'Greg Kroah-Hartman'" <gregkh@...uxfoundation.org>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link

On Wed, 27 Feb 2019 14:45:44 +0100
Frederic Barrat <fbarrat@...ux.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@....ibm.com>
> >>> Sent: Wednesday, 27 February 2019 6:55 PM
> >>> To: Alastair D'Silva <alastair@...ilva.org>; 'Alastair D'Silva'
> >>> <alastair@....ibm.com>
> >>> Cc: 'Greg Kurz' <groug@...d.org>; 'Frederic Barrat'
> >>> <fbarrat@...ux.ibm.com>; 'Arnd Bergmann' <arnd@...db.de>; 'Greg Kroah-
> >>> Hartman' <gregkh@...uxfoundation.org>; linuxppc-dev@...ts.ozlabs.org;
> >>> linux-kernel@...r.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
>