On Tue, Aug 15, 2017 at 01:40:22PM -0700, Don Hiatt wrote:
>
>
> On 8/14/2017 1:12 PM, Don Hiatt wrote:
> > On 8/14/2017 11:36 AM, Don Hiatt wrote:
> > >
> > >
> > > On 8/14/2017 11:17 AM, Don Hiatt wrote:
> > > > This patch series primarily increases sizes of variables that hold
> > > > lid values from 16 to 32 bits. Additionally, it adds a check in
> > > > the IB mad stack to verify a properly formatted MAD when OPA
> > > > extended LIDs are used.
> > > >
> > > > Signed-off-by: Don Hiatt <don.hiatt@xxxxxxxxx>
> > > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> > > > ---
> > > >
> > > > This is an incremental patch to move from v3 of the 'Add OPA
> > > > extended LID support' to v6 of the series.
> > > > Changes from v5:
> > > > ---------------
> > > > * Fixed typo in WARN_ON_ONCE usage in helper functions.
> > > > * Actually return be16 in ib_lid_be16() helper function.
> > >
> > > Sorry, this was meant to go to my email as a test, not to the list.
> > > My tests are still running so please
> > > hold off on this until I confirm.
> > >
> > All test completed fine. I think we're good to go.
> >
> > Leon, if I missed anything else please let me know.
> >
> > Thanks,
> >
> > don
> >
> I did not get this email respond but saw it on the mailing list so pasted it
> in to respond)
>
> >Yeah, you should fix the function below too.
> >The whole extended LID series did enormous mess with all these
> lid/slid/dlid.
> >
> > 88 static inline bool opa_is_extended_lid(u32 dlid, u32 slid)
> > 89 {
> > 90 if ((be32_to_cpu(dlid) >=
> > 91 be16_to_cpu(IB_MULTICAST_LID_BASE)) ||
> > 92 (be32_to_cpu(slid) >=
> > 93 be16_to_cpu(IB_MULTICAST_LID_BASE)))
> > 94 return true;
> > 95 else
> > 96 return false;
> > 97 }
> >
> >It will help a lot, if you break this patch to small steps:
> >1. Fix existing annotation errors.
> >2. Change (rename) the ib_lid/ib_slid functions.
> >3. Add WARN_ON.
> >
> >Right now, we have potential breakage of compatibility between
> >big-endian vs. little-endian systems.
> >
> >Please run smatch and sparse checkers before LID patches and after to
> >know what else you should fix.
> >
> > Thanks
>
> This patch series sat on the mailing list for over two months since the your
> last
> request. It then got merged in and an incremental patch was asked for.
>
> I've been trying to address your concerns but since this patch is going in
> as an
> incremental patch I do not see how breaking it up as requested is required.
>
> If Doug would like to pull the entire series then I'll break the patches up.
>
> As of now, with this patch the endian-ness issues have been resolved, or are
> you
> saying they are not?
>
My complains are not related to the fact that this patch series was
posted to the mailing list a long time ago and merged - it is completely OK.
My complains are due to the fact that the whole community work is built
on trust and we (me and I saw Doug said the same) expect from the
developers doing at least sanity checks before sending their patches.
The MINIMAL set from random developers of those sanity checks are: checkpatch,
builds without warnings, runs of smatch and sparse.
For my submissions, I'm relying on results from verification team
and personally checks boots together with simple ping-pong.
I skipped some of this checks once and we saw disaster (the patch to check
legal values for the port), but those LIDs changes did much more - broke
RoCE, RoCE multicast and port in AH in addition to big-little endianness
mess.
So the bottom-line, please don't expect from us to do your homework and
filter the warnings.
Before the LIDs patches, the RDMA tree had less than 100 smatch warnings,
after the LIDs patches, we have more than 200 such warnings.
Thanks
Attachment:
signature.asc
Description: PGP signature
