Re: Recent sysctl changes
On Thu, Mar 06, 2014 at 09:19:00AM +0200, Andreas Gustafsson wrote: > Thor Lancelot Simon wrote: > > I don't actually know of any code that hands over a "wrong-size" > > buffer and will therefore break, though. Do you? > > No, but I think we should aim for correctness, not just "works for me". Seconded. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: Recent sysctl changes
Thor Lancelot Simon wrote: > I don't actually know of any code that hands over a "wrong-size" > buffer and will therefore break, though. Do you? No, but I think we should aim for correctness, not just "works for me". -- Andreas Gustafsson, g...@netbsd.org
Re: arcmsr(4) and SAS
Thor Lancelot Simon wrote: > It would be nice to have a Marvell SAS driver and I am pretty sure there > is an open source one for FreeBSD, but I'm not exactly suggesting you > write or port one just for your own amusement... Especially since the motivation was LTFS... The following SAS models are currently being sold. Any idea if some of them would be in the path of least resistance? ARC-1320-8X ARC-1320IX-16 ARC-1320-4I4X ARC-1320-8I ARC-1212 ARC-1222 ARC-1213-4I ARC-1213-4X ARC-1223-8I ARC-1223-8X ARC-1680IX-24 ARC-1880IXL-8 ARC-1880IXL-12 ARC-1882I ARC-1882IX-12 ARC-1882IX-16 ARC-1882IX-24 ARC-1882LP ARC-1882X -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz m...@netbsd.org
Re: arcmsr(4) and SAS
On Thu, Mar 06, 2014 at 06:21:43AM +0100, Emmanuel Dreyfus wrote: > Thor Lancelot Simon wrote: > > > On Wed, Mar 05, 2014 at 03:52:33PM +, Emmanuel Dreyfus wrote: > > > > > > But no disk connected to the controller will attach. The Linux driver > > > does not seems to treat SATA and SAS models very differently. Is there > > > anything particular to NetBSD when it comes to SAS? I see in the man page > > > that some other ARCEA SAS board are supported by NetBSD. > > > > Have a look at the FreeBSD driver. It has (last I looked, anyway) > > an idea of card type: "A", "B", "C", "D", and maybe even "E". > > The ARC1380-8i is PCI productID 0x1320. It does not seems to be > supported either by FreeBSD nor by Linux as of 3.13. It is a Marvell SAS controller with Areca vendor and product IDs. For some reason Areca seem to try to hide this but the Linux driver source (the FreeBSD driver is binary-only!) makes it pretty clear. In other words -- I hate to say it, but -- not really an Areca controller. It would be nice to have a Marvell SAS driver and I am pretty sure there is an open source one for FreeBSD, but I'm not exactly suggesting you write or port one just for your own amusement... Thor
Re: arcmsr(4) and SAS
Thor Lancelot Simon wrote: > On Wed, Mar 05, 2014 at 03:52:33PM +, Emmanuel Dreyfus wrote: > > > > But no disk connected to the controller will attach. The Linux driver > > does not seems to treat SATA and SAS models very differently. Is there > > anything particular to NetBSD when it comes to SAS? I see in the man page > > that some other ARCEA SAS board are supported by NetBSD. > > Have a look at the FreeBSD driver. It has (last I looked, anyway) > an idea of card type: "A", "B", "C", "D", and maybe even "E". The ARC1380-8i is PCI productID 0x1320. It does not seems to be supported either by FreeBSD nor by Linux as of 3.13. The commercial name ARC1380-8i is misleading, as NetBSD drivers seems support the ARC1380. At least it matches it. I'm affraid this board will be a paperweight for a few months. -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz m...@netbsd.org
Re: Recent sysctl changes
On Wed, 5 Mar 2014, Andreas Gustafsson wrote: I posted this on source-changes-d earlier today, but Jeff Rizzo asked me to take it to tech-kern. On Feb 27, David Laight said: Module Name: src Committed By: dsl Date: Thu Feb 27 22:50:52 UTC 2014 Modified Files: src/sys/kern: kern_sysctl.c Log Message: Allow CTLTYPE_INT and CTLTYPE_QUAD to be read and written as either 4 or 8 byte values regardless of the type. 64bit writes to 32bit variables must be valid (signed) values. 32bit reads of large values return -1. Amongst other things this should fix libm's code that reads machdep.sse as a 32bit int, but I'd changed it to 64bit (to common up some code). To generate a diff of this commit: cvs rdiff -u -r1.246 -r1.247 src/sys/kern/kern_sysctl.c In private correspondence with David, I have objected to this commit as well as his earlier change to the type of the machdep.sse sysctl variable. Regrettably, David and I have not been able to reach an agreement, and he has requested that I ask for opinions on a public list. So... 1. I object to the earlier change of the following sysctl variables from type CTLTYPE_INT to type CTLTYPE_QUAD: machdep.fpu_present machdep.osfxsr machdep.sse machdep.sse2 machdep.biosbasemem machdep.biosextmem My understanding of David's rationale for changing them is that it facilitated sharing code with the machdep.xsave_features variable which actually needs to be of type CTLTYPE_QUAD. By my reckoning, the savings from this code sharing amount to eight lines of code. Changing the types of existing sysctl variables breaks both source and binary compatibility and should not be done lightly; that's why we have both both hw.physmem and hw.physmem64, for example. Now, the types of multiple variables have been incompatibly changed for no reason other than saving a few lines of code. I believe the harm caused by the incompatible type change far outweighs the cost of a few added lines of code, and would like the original types to be restored. I am attaching a patch to do so; it also changes the newly added variables machdep.fpu_save and machdep.fpu_save_size to CTLTYPE_INT, but if David really thinks those should be CTLTYPE_QUAD, I will not argue that point. 2. I also object to the change of kern_sysctl.c 1.247. This change attempts to work around the problems caused by the changes to the variable types by making sysctl() return different types depending on the value of the *oldlenp argument. IMO, this is a bad idea. The *oldlenp argument does *not* specify the size of the data type expected by the caller, but rather the size of a buffer. The sysctl() API allows the caller to pass a buffer larger than the variable being read, and conversely, guarantees that passing a buffer that is too small results in ENOMEM. Both of these aspects of the API are now broken: reading a 4-byte CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8, and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the buffer size happens to be 4. IMO, this behavior violates both the letter of the sysctl() man page and the principle of least astonishment. Also, the work-around is ineffective in the case of a caller that allocates the buffer dynamically using the size given by an initial sysctl() call with oldp = NULL. If the original types of the sysctl variables are restored, this work-around will no longer serve a purpose, and I'm asking for it to be removed. Opinions? I agree on both counts. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: Recent sysctl changes
On Wed, Mar 05, 2014 at 03:56:54PM -0500, Thor Lancelot Simon wrote: > On Wed, Mar 05, 2014 at 08:55:50PM +0200, Andreas Gustafsson wrote: > > > > 2. I also object to the change of kern_sysctl.c 1.247. > > > > This change attempts to work around the problems caused by the changes > > to the variable types by making sysctl() return different types > > depending on the value of the *oldlenp argument. > > As I recall, we considered this approach before creating hw.physmem64, > and decided it was just a little too cute. > > I don't actually know of any code that hands over a "wrong-size" > buffer and will therefore break, though. Do you? I agree the > possibility does exist. I actually wonder if the code should also support single byte reads for things like machdep.sse which are effectively booleans. Maybe we should also allow 1, 4 and 8 byte reads for items declared as booleans. IIRC one of the arm ABIs uses 4 byte booleans - bound to be a cause for confusion at some point. David -- David Laight: da...@l8s.co.uk
Re: Recent sysctl changes
On Wed, Mar 05, 2014 at 08:55:50PM +0200, Andreas Gustafsson wrote: > > 2. I also object to the change of kern_sysctl.c 1.247. > > This change attempts to work around the problems caused by the changes > to the variable types by making sysctl() return different types > depending on the value of the *oldlenp argument. As I recall, we considered this approach before creating hw.physmem64, and decided it was just a little too cute. I don't actually know of any code that hands over a "wrong-size" buffer and will therefore break, though. Do you? I agree the possibility does exist. Thor
Recent sysctl changes
I posted this on source-changes-d earlier today, but Jeff Rizzo asked
me to take it to tech-kern.
On Feb 27, David Laight said:
> Module Name: src
> Committed By: dsl
> Date: Thu Feb 27 22:50:52 UTC 2014
>
> Modified Files:
> src/sys/kern: kern_sysctl.c
>
> Log Message:
> Allow CTLTYPE_INT and CTLTYPE_QUAD to be read and written as either 4 or 8
> byte values regardless of the type.
> 64bit writes to 32bit variables must be valid (signed) values.
> 32bit reads of large values return -1.
> Amongst other things this should fix libm's code that reads machdep.sse
> as a 32bit int, but I'd changed it to 64bit (to common up some code).
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.246 -r1.247 src/sys/kern/kern_sysctl.c
In private correspondence with David, I have objected to this commit
as well as his earlier change to the type of the machdep.sse
sysctl variable.
Regrettably, David and I have not been able to reach an agreement, and
he has requested that I ask for opinions on a public list. So...
1. I object to the earlier change of the following sysctl variables
from type CTLTYPE_INT to type CTLTYPE_QUAD:
machdep.fpu_present
machdep.osfxsr
machdep.sse
machdep.sse2
machdep.biosbasemem
machdep.biosextmem
My understanding of David's rationale for changing them is that it
facilitated sharing code with the machdep.xsave_features variable
which actually needs to be of type CTLTYPE_QUAD. By my reckoning,
the savings from this code sharing amount to eight lines of code.
Changing the types of existing sysctl variables breaks both source
and binary compatibility and should not be done lightly; that's
why we have both both hw.physmem and hw.physmem64, for example.
Now, the types of multiple variables have been incompatibly changed
for no reason other than saving a few lines of code.
I believe the harm caused by the incompatible type change far
outweighs the cost of a few added lines of code, and would like
the original types to be restored. I am attaching a patch to do so;
it also changes the newly added variables machdep.fpu_save and
machdep.fpu_save_size to CTLTYPE_INT, but if David really thinks
those should be CTLTYPE_QUAD, I will not argue that point.
2. I also object to the change of kern_sysctl.c 1.247.
This change attempts to work around the problems caused by the changes
to the variable types by making sysctl() return different types
depending on the value of the *oldlenp argument.
IMO, this is a bad idea. The *oldlenp argument does *not* specify the
size of the data type expected by the caller, but rather the size of a
buffer. The sysctl() API allows the caller to pass a buffer larger
than the variable being read, and conversely, guarantees that passing
a buffer that is too small results in ENOMEM.
Both of these aspects of the API are now broken: reading a 4-byte
CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8,
and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer
of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the
buffer size happens to be 4. IMO, this behavior violates both the
letter of the sysctl() man page and the principle of least astonishment.
Also, the work-around is ineffective in the case of a caller that
allocates the buffer dynamically using the size given by an initial
sysctl() call with oldp = NULL.
If the original types of the sysctl variables are restored, this
work-around will no longer serve a purpose, and I'm asking for it
to be removed.
Opinions?
--
Andreas Gustafsson, g...@netbsd.org
Index: x86_machdep.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/x86_machdep.c,v
retrieving revision 1.63
diff -u -r1.63 x86_machdep.c
--- x86_machdep.c 23 Feb 2014 22:38:40 - 1.63
+++ x86_machdep.c 3 Mar 2014 18:41:06 -
@@ -1056,7 +1056,17 @@
}
static void
-const_sysctl(struct sysctllog **clog, const char *name, u_quad_t value, int
tag)
+const_sysctl_int(struct sysctllog **clog, const char *name, int value, int tag)
+{
+ sysctl_createv(clog, 0, NULL, NULL,
+ CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE,
+ CTLTYPE_INT, name, NULL,
+ NULL, (u_quad_t)value, NULL, 0,
+ CTL_MACHDEP, tag, CTL_EOL);
+}
+
+static void
+const_sysctl_quad(struct sysctllog **clog, const char *name, u_quad_t value,
int tag)
{
sysctl_createv(clog, 0, NULL, NULL,
CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE,
@@ -1115,17 +1125,17 @@
CTL_MACHDEP, CTL_CREATE, CTL_EOL);
/* None of these can ever change once the system has booted */
- const_sysctl(clog, "fpu_present", i386_fpu_present, CPU_FPU_PRESENT);
- const_sysctl(clog, "osfxsr", i386_use_fxsave, CPU_OSFXSR);
- const_sysctl(clog, "sse", i386_has_sse, CPU_SSE);
- const_sysctl(clog, "sse2", i386_has_
Re: arcmsr(4) and SAS
On Wed, Mar 05, 2014 at 03:52:33PM +, Emmanuel Dreyfus wrote: > > But no disk connected to the controller will attach. The Linux driver > does not seems to treat SATA and SAS models very differently. Is there > anything particular to NetBSD when it comes to SAS? I see in the man page > that some other ARCEA SAS board are supported by NetBSD. Have a look at the FreeBSD driver. It has (last I looked, anyway) an idea of card type: "A", "B", "C", "D", and maybe even "E". In almost every routine in the driver, right at the head there's a switch statement that calls the per-card-type routine. This is obviously a pretty awful way to do it -- the devices are clearly not that different from each other since we work fine with several of their "types" using a single driver without all those conditionals, and where they *are* that different, it would be far better to use multiple drivers that call the common code kept in a separate source file -- but by examining the code for the newer types in that driver you can probably see what the differences are, and then decide how hard it would be to fix. I don't believe there's anything particularly different about the SAS cards compared to the SATA cards; but I don't believe our driver currently works with any cards newer than the 16xx. The 18xx and 13xx are both too new, I believe. Thor
arcmsr(4) and SAS
Hi I have an ARECA ARC1380-8i board with supports 8 SAS devices. I try to get it working with arcmsr(4). Matching the device is easy, and I now have: arcmsr0 at pci1 dev 0 function 0 arcmsr0: interrupting at irq 9 But no disk connected to the controller will attach. The Linux driver does not seems to treat SATA and SAS models very differently. Is there anything particular to NetBSD when it comes to SAS? I see in the man page that some other ARCEA SAS board are supported by NetBSD. -- Emmanuel Dreyfus m...@netbsd.org
