Michael Ellerman <[email protected]> writes: > Vaibhav Jain <[email protected]> writes: > >> Thanks Mpe for reviewing the patch >> >> Michael Ellerman <[email protected]> writes: >> >>>> To fix this the patch assigns the return value of assign_thread_tidr() >>>> to a temporary int and assigns it to thread.tidr iff its '> 0'. >>> >>> .. and changes the calling convention of the function. >>> >>> Now it returns -ve error values, or a +ve TIDR value when it succeeds, >>> or possibly 0 if that's returned by assign_thread_tidr(). >>> >>> Which I'm not sure you meant to do. If you did, you should at least >>> document it. >> >> Yes this is intentional and this was supposed to be the calling >> convention of set_thread_tidr() in first place. At-least that what I >> gather from subsequent cxl patch to add its support >> http://patchwork.ozlabs.org/patch/840719/ > > That's not at all what I gather from that patch. > > + /* Assign a unique TIDR (thread id) for the current thread */ > + rc = set_thread_tidr(current); > + if (!rc) > + ctx->tid = current->thread.tidr; > > That expects 0 on success, anything else is an error. > > Which is what set_thread_tidr() currently implements, and is the most > common calling convention in kernel code. > > Please don't change that as part of an unrelated fix. > > If you want to change the calling convention, send a patch to do that > and only that. > > cheers > Agreed Mpe, checked with Christophe and he too echoed similar inputs. I will update my v2 patch by not causing a change to he call convention.
-- Vaibhav Jain <[email protected]> Linux Technology Center, IBM India Pvt. Ltd.
