Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On 06/14/18 14:21, Bruce Evans wrote: Eventually the leak breaks uniqueness of hard-coded Linux major numbers. Like already said, the major and minor numbers are internal to the LinuxKPI and devfs is not aware about them, and nor anything in user-space. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On Thu, 14 Jun 2018, Hans Petter Selasky wrote: On 06/14/18 10:46, Bruce Evans wrote: Are these macros for conversion of host (FreeBSD) dev_t's or target (Linux) ones??? If for the host, then I don't see any reason not to use the host APIs. If for the target, then they shouldn't be used with the host dev_t.?? If for a mixture, then the translations are very confusing, especially when they are the identity, and many more macros are needed to reduce the confusion. These values are only used inside the LinuxKPI for creating character devices. They have no exposure to user-space. Basically: Z = MKDEV(X,Y) MAJOR(Z) must be equal to X MINOR(Z) must be equal to Y So they are only for the host. You should try harder to allocate the device number properly (not using hard-coded numbers which are not even #defined in a central place). compat/linux already has similar hard-coding for pts and more, but that is more central and historically established. Unique device numbers now seems to be allocated by devfs_alloc_cdp_inode(). Or call make_dev*() to create a full device with a device number allocated by this. I now see bugs in this too. Major numbers for devices are supposed to be 0. But devfs_alloc_cdp_inode() constructs a minor number which is assigned to cdp->cdp_inode. cdp_inode is treated as a device number, not as a minor number. It is assigned directly to a device number in dev2udev() and for assignment to va_rdev. So after 256 allocations, the minor bits leak into the major bits. Eventually the leak breaks uniqueness of hard-coded Linux major numbers. I saw this earlier to today but thought the problem was a kernel without the encoding fixes combined with an old userland using old major(), etc., to decode new dev_t encoding. Actually, the encodings were consistently old except in devfs_alloc_cdp_inode(). I created 514 ptys in /dev/pts/ and noticed that the numbers change from (major = N, minor = 255) to (major = N, minor = 0). With the previous kernel encoding, the minor numbers would have increased sequentially, but old userland would not have noticed the difference since it users old major(), etc., to decode.s If you create a device by name, then its name needs to be unique instead of its numbers, but this is easier to arrange. Bruce___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On 06/14/18 10:46, Bruce Evans wrote: Are these macros for conversion of host (FreeBSD) dev_t's or target (Linux) ones? If for the host, then I don't see any reason not to use the host APIs. If for the target, then they shouldn't be used with the host dev_t. If for a mixture, then the translations are very confusing, especially when they are the identity, and many more macros are needed to reduce the confusion. Hi, These values are only used inside the LinuxKPI for creating character devices. They have no exposure to user-space. Basically: Z = MKDEV(X,Y) MAJOR(Z) must be equal to X MINOR(Z) must be equal to Y --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On Thu, 14 Jun 2018, Hans Petter Selasky wrote: On 06/14/18 02:03, 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 * ... Please revert this patch and fix MKDEV() in: compat/linuxkpi/common/include/linux/kdev_t.h Instead. The value returned by MKDEV() must be passable through MAJOR() and MINOR() to restore the two arguments. Please make sure this is the case. Else the code gets broken. Are these macros for conversion of host (FreeBSD) dev_t's or target (Linux) ones? If for the host, then I don't see any reason not to use the host APIs. If for the target, then they shouldn't be used with the host dev_t. If for a mixture, then the translations are very confusing, especially when they are the identity, and many more macros are needed to reduce the confusion. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On 06/14/18 09:26, Hans Petter Selasky wrote:
On 06/14/18 02:03, 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
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?
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.
Hi,
Please revert this patch and fix MKDEV() in:
compat/linuxkpi/common/include/linux/kdev_t.h
Instead.
The value returned by MKDEV() must be passable through MAJOR() and
MINOR() to restore the two arguments. Please make sure this is the case.
Else the code gets broken.
--HPS
See r335123.
--HPS
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
On 06/14/18 02:03, 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
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?
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.
Hi,
Please revert this patch and fix MKDEV() in:
compat/linuxkpi/common/include/linux/kdev_t.h
Instead.
The value returned by MKDEV() must be passable through MAJOR() and
MINOR() to restore the two arguments. Please make sure this is the case.
Else the code gets broken.
--HPS
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
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
>>
>> 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?
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.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r335094 - head/sys/ofed/drivers/infiniband/core
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
>
> 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?
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.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
