From owner-svn-src-all@freebsd.org Thu Jun 14 08:10:04 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A0976100E997; Thu, 14 Jun 2018 08:10:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id E26EC7C39A; Thu, 14 Jun 2018 08:10:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id F3C023CCC66; Thu, 14 Jun 2018 18:09:51 +1000 (AEST) Date: Thu, 14 Jun 2018 18:09:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Matthew Macy cc: Ryan Libby , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org, bde@freebsd.org Subject: Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core In-Reply-To: Message-ID: <20180614165804.L1156@besplex.bde.org> References: <201806132330.w5DNUsrE043573@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=I9sVfJog c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=Qbu6MDAsAjz1s12MRMMA:9 a=NM1pzMuOk_UbrT9-:21 a=hHtOW9pH9XD3AjIz:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jun 2018 08:10:04 -0000 On Wed, 13 Jun 2018, Matthew Macy wrote: > On Wed, Jun 13, 2018 at 4:47 PM, Ryan Libby wrote: >> On Wed, Jun 13, 2018 at 4:30 PM, Matt Macy wrote: >>> Author: mmacy >>> Date: Wed Jun 13 23:30:54 2018 >>> New Revision: 335094 >>> URL: https://svnweb.freebsd.org/changeset/base/335094 >>> >>> Log: >>> fix OFED build after r335053 Ooops. makedev(), etc., will have to go back to being plain macros. But not so plain since they will need to use unportable statement-expressions to be safe macros. >>> Modified: >>> head/sys/ofed/drivers/infiniband/core/ib_user_mad.c >>> >>> Modified: head/sys/ofed/drivers/infiniband/core/ib_user_mad.c >>> ============================================================================== >>> --- head/sys/ofed/drivers/infiniband/core/ib_user_mad.c Wed Jun 13 23:19:54 2018 (r335093) >>> +++ head/sys/ofed/drivers/infiniband/core/ib_user_mad.c Wed Jun 13 23:30:54 2018 (r335094) >>> @@ -130,7 +130,8 @@ struct ib_umad_packet { >>> >>> static struct class *umad_class; >>> >>> -static const dev_t base_dev = MKDEV(IB_UMAD_MAJOR, IB_UMAD_MINOR_BASE); >>> +#define IBMKDEV(x, y) (((dev_t)(x) << 32) | (unsigned)(y)) >>> +static const dev_t base_dev = IBMKDEV(IB_UMAD_MAJOR, IB_UMAD_MINOR_BASE); >>> >>> static DEFINE_SPINLOCK(port_lock); >>> static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS); >>> >> >> The scheme for major/minor encoding is different as of r335053. Won't >> that matter? Using the host makedev() seems to be correct here. So is wrapping it in MKDEV() (except the definition of MKDEV() is broken -- it is missing parentheses around args). Since the major and minor number here are small (231, 0), there is no problem with the encoding and you can just use the old makedev() macro for a quick fix (shift by 8 instead of 32) until the new one is fixed. I seem to have broken the 64-bit linux l_dev_t in a different way than before. Linux device numbers are only 16 bits for linux32 (even for stat64) and also for everything in linux[64] except newstat. For 64-bit (amd64) newstat, they are 64 bits. They can actually support large major and minor numbers in this case. I don't know there encoding for this case, but it is unlikely to be the same as the FreeBSD one. This gives the following bugs: - st_rdev is still translated by old code that does (major << 8 | minor). This is supposed to give the Linux encoding. For major < 256 and minor < 256, it works to give the old common 16-bit encoding. Otherwises, it first combines the bits in a wrong way and then assignment to 16-bit ldev_t may lose bits while assignment to 64-bit ldev_t gives wrong bits instead of lost bits for the high 48 bits. - st_dev was translated in the same way. Now it truncates to 16 bits in the 64-bit case too, so it loses bits consistently. > Yes. > >> In sys/ofed/drivers/infiniband/core/{ib_ucm.c,ib_uverbs_main.c} the >> pattern is to #define the MKDEV(). Following that would in >> ib_user_mad.c would also resolve this. Or makedev could be >> re-macroized with the new scheme. > > That's probably the best course of action. > Will follow up. It is not best to churn makedev() just for ofed, but some applications probably need it to be a plain macro too. Bruce