svn commit: r335094 - head/sys/ofed/drivers/infiniband/core

[Bruce Evans]

On Wed, 13 Jun 2018, Matthew Macy wrote:

> On Wed, Jun 13, 2018 at 4:47 PM, Ryan Libby <rlibby at freebsd.org> wrote:
>> On Wed, Jun 13, 2018 at 4:30 PM, Matt Macy <mmacy at freebsd.org> 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