Re: CVS commit: src/sys/dev/pci
> On 12. Jul 2020, at 21:05, Jaromir Dolecek wrote: > > Module Name: src > Committed By: jdolecek > Date: Sun Jul 12 19:05:32 UTC 2020 > > Modified Files: > src/sys/dev/pci: if_bnx.c if_bnxvar.h > > Log Message: > enable MSI/MSI-X if supported by adapter > > tested MSI-X with Broadcom NetXtreme II BCM5709 1000Base-T > > > To generate a diff of this commit: > cvs rdiff -u -r1.95 -r1.96 src/sys/dev/pci/if_bnx.c > cvs rdiff -u -r1.12 -r1.13 src/sys/dev/pci/if_bnxvar.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. With this change my Dell PowerEdge 2850 spits watchdog resets: [ 68.1828359] bnx0: Watchdog timeout -- resetting! [ 88.6042909] bnx0: Watchdog timeout -- resetting! [ 119.0265230] bnx0: Watchdog timeout -- resetting! [ 145.4484562] bnx0: Watchdog timeout -- resetting! Dmesg before is: [ 1.017306] pci4 at ppb3 bus 7 [ 1.017306] pci4: i/o space, memory space enabled, rd/line, wr/inv ok [ 1.017306] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 1000Base-T [ 1.017306] bnx0: Ethernet address 00:24:e8:67:4b:db [ 1.017306] bnx0: ASIC BCM5708 B2 (0x57081020) [ 1.017306] bnx0: PCI-X 64bit 133MHz [ 1.017306] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 1.6.0) [ 1.017306] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80) [ 1.017306] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, rev. 6 [ 1.017306] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FDX, auto [ 1.017306] bnx0: interrupting at ioapic0 pin 16 while dmesg after is: [ 1.017262] pci4 at ppb3 bus 7 [ 1.017262] pci4: i/o space, memory space enabled, rd/line, wr/inv ok [ 1.017262] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 1000Base-T [ 1.017262] bnx0: Ethernet address 00:24:e8:67:4b:db [ 1.017262] bnx0: ASIC BCM5708 B2 (0x57081020) [ 1.017262] bnx0: PCI-X 64bit 133MHz [ 1.017262] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 1.6.0) [ 1.017262] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80) [ 1.017262] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, rev. 6 [ 1.017262] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FDX, auto [ 1.017262] bnx0: interrupting at msi0 vec 0 Pcictl dump gives (in the MSI-X case): PCI Message Signaled Interrupt Message Control register: 0x0081 MSI Enabled: on Multiple Message Capable: no (1 vector) Multiple Message Enabled: off (1 vector) 64 Bit Address Capable: on Per-Vector Masking Capable: off Extended Message Data Capable: off Extended Message Data Enable: off Message Address (lower) register: 0xfee0 Message Address (upper) register: 0x Message Data register: 0x0064 Any ideas how to fix this issue? -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany) signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/dev/pci
On Tue, Jan 26, 2021 at 05:51:42PM +0900, Rin Okuyama wrote: > Hi, > This seems not correct for me. Is the attached patch OK with you? Well you spotted a bug indeed int he freeing section. I'll fix and commit it. Thanks for reporting. Reinoud signature.asc Description: PGP signature
Re: CVS commit: src/sys/dev/pci
Hi,
On 2021/01/25 0:33, Reinoud Zandijk wrote:
Module Name:src
Committed By: reinoud
Date: Sun Jan 24 15:33:02 UTC 2021
Modified Files:
src/sys/dev/pci: virtio_pci.c
Log Message:
On error unmap the pci_mapreg_map()d regions using bus_space_unmap() as
suggested by jak@
To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sys/dev/pci/virtio_pci.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
This seems not correct for me. Is the attached patch OK with you?
Thanks,
rin
Index: sys/dev/pci/virtio_pci.c
===
RCS file: /home/netbsd/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.25
diff -p -u -r1.25 virtio_pci.c
--- sys/dev/pci/virtio_pci.c24 Jan 2021 15:59:35 - 1.25
+++ sys/dev/pci/virtio_pci.c26 Jan 2021 08:47:23 -
@@ -444,7 +444,7 @@ virtio_pci_attach_10(device_t self, void
bus_size_t bars[NMAPREG] = { 0 };
int bars_idx[NMAPREG] = { 0 };
struct virtio_pci_cap *caps[] = { , , , };
- int i, j = 0, ret = 0;
+ int i, j, ret = 0;
if (virtio_pci_find_cap(psc, VIRTIO_PCI_CAP_COMMON_CFG,
, sizeof(common)))
@@ -471,7 +471,7 @@ virtio_pci_attach_10(device_t self, void
bars[bar] = len;
}
- for (i = 0; i < __arraycount(bars); i++) {
+ for (i = j = 0; i < __arraycount(bars); i++) {
int reg;
pcireg_t type;
if (bars[i] == 0)
@@ -550,11 +550,12 @@ virtio_pci_attach_10(device_t self, void
err:
/* undo our pci_mapreg_map()s */
- for (i = 0; i < __arraycount(bars); i++) {
+ for (i = j = 0; i < __arraycount(bars); i++) {
if (bars[i] == 0)
continue;
bus_space_unmap(psc->sc_bars_iot[j], psc->sc_bars_ioh[j],
psc->sc_bars_iosize[j]);
+ j++;
}
return ret;
}
Re: CVS commit: src/sys/dev/pci
> On Jan 24, 2021, at 10:20 PM, Martin Husemann wrote: > >> I kept getting a ?static after non-static declaration? error when building >> for aarch64. > > I guess that was with outdated arm/include/bus_funcs.h and > sys/bus_proto.h (or where was the previous declaration)? I did a “cvs update” just before, *shrug*. In any case, the problem was easily avoidable, and now it is avoided. -- thorpej
Re: CVS commit: src/sys/dev/pci
On Sun, Jan 24, 2021 at 09:45:14PM -0800, Jason Thorpe wrote: > > > On Jan 24, 2021, at 9:37 PM, Martin Husemann wrote: > > > > While I don't care for names, I would like to understand fallout in > > details before hiding it - what exactly did not compile correctly? > > At least the affected arm kernels worked for me in the state directly > > before your commit. > > I kept getting a ?static after non-static declaration? error when building > for aarch64. I guess that was with outdated arm/include/bus_funcs.h and sys/bus_proto.h (or where was the previous declaration)? Martin
Re: CVS commit: src/sys/dev/pci
> On Jan 24, 2021, at 9:37 PM, Martin Husemann wrote: > > While I don't care for names, I would like to understand fallout in > details before hiding it - what exactly did not compile correctly? > At least the affected arm kernels worked for me in the state directly > before your commit. I kept getting a “static after non-static declaration” error when building for aarch64. -- thorpej
Re: CVS commit: src/sys/dev/pci
On Sun, Jan 24, 2021 at 03:34:08PM +, Jason R Thorpe wrote: > Module Name: src > Committed By: thorpej > Date: Sun Jan 24 15:34:08 UTC 2021 > > Modified Files: > src/sys/dev/pci: virtio_pci.c > > Log Message: > Redefining bus_space functions in drivers is a bad idea, and we just > should't be in the habit of doing so. Besides, the previous "solutions" > still did not compile correctly, and this does, so let's be done with > this nonsense, shall we? While I don't care for names, I would like to understand fallout in details before hiding it - what exactly did not compile correctly? At least the affected arm kernels worked for me in the state directly before your commit. Martin
Re: CVS commit: src/sys/dev/pci
> On Jan 23, 2021, at 5:40 PM, Christos Zoulas wrote: > >> it's a bad example. someone might copy it into another >> driver that _doesn't_ work with this version, but may seem >> to fix a build error. >> >> that's why i wanted to wrap the usage to make it clear if >> someone were to copy it elsewhere. > > I will add a comment. Yah, I was gonna say, “big fat comment in order!" -- thorpej
Re: CVS commit: src/sys/dev/pci
In article <17141.1611452...@splode.eterna.com.au>, matthew green wrote: >Christos Zoulas writes: >> In article <20210123230600.52be160...@jupiter.mumble.net>, >> Taylor R Campbell wrote: >> > >> >Conversely, how do you know whether this hacked-up implementation >> >which tears the write into two will actually work? Maybe it works for >> >virtio but there are likely other devices out there for which it will >> >fail or have weird side effects if the architecture doesn't have >> >native 8-byte bus I/O. >> >> But it is a static function defined in virtio_pci.c. How will other >> devices use it? I must be missing something. > >it's a bad example. someone might copy it into another >driver that _doesn't_ work with this version, but may seem >to fix a build error. > >that's why i wanted to wrap the usage to make it clear if >someone were to copy it elsewhere. I will add a comment. christos
re: CVS commit: src/sys/dev/pci
Christos Zoulas writes: > In article <20210123230600.52be160...@jupiter.mumble.net>, > Taylor R Campbell wrote: > > > >Conversely, how do you know whether this hacked-up implementation > >which tears the write into two will actually work? Maybe it works for > >virtio but there are likely other devices out there for which it will > >fail or have weird side effects if the architecture doesn't have > >native 8-byte bus I/O. > > But it is a static function defined in virtio_pci.c. How will other > devices use it? I must be missing something. it's a bad example. someone might copy it into another driver that _doesn't_ work with this version, but may seem to fix a build error. that's why i wanted to wrap the usage to make it clear if someone were to copy it elsewhere. .mrg.
Re: CVS commit: src/sys/dev/pci
In article <20210123230600.52be160...@jupiter.mumble.net>, Taylor R Campbell wrote: > >Conversely, how do you know whether this hacked-up implementation >which tears the write into two will actually work? Maybe it works for >virtio but there are likely other devices out there for which it will >fail or have weird side effects if the architecture doesn't have >native 8-byte bus I/O. But it is a static function defined in virtio_pci.c. How will other devices use it? I must be missing something. christos
Re: CVS commit: src/sys/dev/pci
> Date: Sat, 23 Jan 2021 22:59:22 - (UTC)
> From: chris...@astron.com (Christos Zoulas)
>
> In article <23974.1611441...@splode.eterna.com.au>,
> matthew green wrote:
> >this seems dangerous to me. we don't define it on
> >some platforms because we can't, so having it faked
> >out here seems like someone later will be confused
> >and the wrong thing will happen.
> >
> >i would rather have something like
> >
> >virtio_write8(...)
> >{
> >#ifdef __HAVE_BUS_SPACE_8
> > just use the real thing
> >#else
> > use the dual-_4 version that is ok _for this device_
> >#endif
> >}
> >
> >and then use this wrapper in the rest of the code.
>
> This implementation is internal to virtio_pci and is guaranteed to work
> by the spec, how will someone else us it?
Conversely, how do you know whether this hacked-up implementation
which tears the write into two will actually work? Maybe it works for
virtio but there are likely other devices out there for which it will
fail or have weird side effects if the architecture doesn't have
native 8-byte bus I/O.
Re: CVS commit: src/sys/dev/pci
In article <23974.1611441...@splode.eterna.com.au>,
matthew green wrote:
>"Christos Zoulas" writes:
>> Module Name: src
>> Committed By:christos
>> Date:Sat Jan 23 20:00:19 UTC 2021
>>
>> Modified Files:
>> src/sys/dev/pci: virtio_pci.c
>>
>> Log Message:
>> Provide a generic bus_space_write_8 function that is bi-endian.
>
>this seems dangerous to me. we don't define it on
>some platforms because we can't, so having it faked
>out here seems like someone later will be confused
>and the wrong thing will happen.
>
>i would rather have something like
>
>virtio_write8(...)
>{
>#ifdef __HAVE_BUS_SPACE_8
> just use the real thing
>#else
> use the dual-_4 version that is ok _for this device_
>#endif
>}
>
>and then use this wrapper in the rest of the code.
This implementation is internal to virtio_pci and is guaranteed to work
by the spec, how will someone else us it?
christos
re: CVS commit: src/sys/dev/pci
"Christos Zoulas" writes:
> Module Name: src
> Committed By: christos
> Date: Sat Jan 23 20:00:19 UTC 2021
>
> Modified Files:
> src/sys/dev/pci: virtio_pci.c
>
> Log Message:
> Provide a generic bus_space_write_8 function that is bi-endian.
this seems dangerous to me. we don't define it on
some platforms because we can't, so having it faked
out here seems like someone later will be confused
and the wrong thing will happen.
i would rather have something like
virtio_write8(...)
{
#ifdef __HAVE_BUS_SPACE_8
just use the real thing
#else
use the dual-_4 version that is ok _for this device_
#endif
}
and then use this wrapper in the rest of the code.
.mrg.
Re: CVS commit: src/sys/dev/pci
In article , Reinoud Zandijk wrote: >On Fri, Jan 22, 2021 at 04:54:51PM +1100, matthew green wrote: >> > +#ifndef _LP64 >> >> _LP64 is a terrible way to make this choice. >> >> heaps of our 32 bit platforms implement the _8 variants. > >Can't we then just make sure they have the 8 bit variant? and set a define if >its atomic or not? > >This way drivers van use the _8 variant freely and for the few drivers that >NEED the atomicity, they can check the define and deal with it the way they >like. Perhaps. But still for the ones that don't have it should use the central implementation so that we don't duplicate code. christos
Re: CVS commit: src/sys/dev/pci
In article ,
Nick Hudson wrote:
>On 22/01/2021 04:48, Christos Zoulas wrote:
>> +#if _QUAD_HIGHWORD
>> +bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
>> +bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
>> +#else
>> +bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
>> +bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
>> +#endif
>> +}
>> +#endif /* !_LP64 */
>
>
>BUS_ADDR_{HI,LO}32 are also available for your convenience.
Will use those thanks!
christos
Re: CVS commit: src/sys/dev/pci
On Fri, Jan 22, 2021 at 04:54:51PM +1100, matthew green wrote: > > +#ifndef _LP64 > > _LP64 is a terrible way to make this choice. > > heaps of our 32 bit platforms implement the _8 variants. Can't we then just make sure they have the 8 bit variant? and set a define if its atomic or not? This way drivers van use the _8 variant freely and for the few drivers that NEED the atomicity, they can check the define and deal with it the way they like. Reinoud
Re: CVS commit: src/sys/dev/pci
On 22/01/2021 04:48, Christos Zoulas wrote:
+#if _QUAD_HIGHWORD
+ bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
+ bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
+#else
+ bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
+ bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
+#endif
+}
+#endif /* !_LP64 */
BUS_ADDR_{HI,LO}32 are also available for your convenience.
Nick
Re: CVS commit: src/sys/dev/pci
In article <27744.1611294...@splode.eterna.com.au>, matthew green wrote: >> +#ifndef _LP64 > >_LP64 is a terrible way to make this choice. > >heaps of our 32 bit platforms implement the _8 variants. Let's add a _HAVE_ variable then? christos
re: CVS commit: src/sys/dev/pci
> +#ifndef _LP64 _LP64 is a terrible way to make this choice. heaps of our 32 bit platforms implement the _8 variants. .mrg.
Re: CVS commit: src/sys/dev/pci
In article <20210121204833.9ebcff...@cvs.netbsd.org>,
Reinoud Zandijk wrote:
>-=-=-=-=-=-
>
>Module Name: src
>Committed By: reinoud
>Date: Thu Jan 21 20:48:33 UTC 2021
>
>Modified Files:
> src/sys/dev/pci: virtio_pci.c
>
>Log Message:
>Remove dependency on bus_space_write_8() for i386 and instead implement it as
>two bus_space_write_4()'s as allowed in the spec.
Isn't it better to do it this way so it always works (not just for little
endian)? We could even provide this in the MI bus.h if others need it
and don't care about the non-atomic transactions. I have not even compile
tested it.
christos
Index: virtio_pci.c
===
RCS file: /cvsroot/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 virtio_pci.c
--- virtio_pci.c21 Jan 2021 20:48:33 - 1.18
+++ virtio_pci.c22 Jan 2021 04:46:24 -
@@ -34,6 +34,7 @@ __KERNEL_RCSID(0, "$NetBSD: virtio_pci.c
#include
#include
#include
+#include
#include
#include
@@ -731,9 +732,20 @@ virtio_pci_read_queue_size_10(struct vir
* By definition little endian only in v1.0 and 8 byters are allowed to be
* written as two 4 byters
*/
-#define bus_space_write_le_8(iot, ioh, reg, val) \
- bus_space_write_4(iot, ioh, reg, ((uint64_t) (val)) & 0x); \
- bus_space_write_4(iot, ioh, reg + 4, ((uint64_t) (val)) >> 32);
+#ifndef _LP64
+static __inline void
+bus_space_write_8(bus_space_tag_t iot, bus_space_handle_t ioh,
+ bus_size_t offset, uint64_t value)
+{
+#if _QUAD_HIGHWORD
+ bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
+ bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
+#else
+ bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
+ bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
+#endif
+}
+#endif /* !_LP64 */
static void
virtio_pci_setup_queue_10(struct virtio_softc *sc, uint16_t idx, uint64_t addr)
@@ -747,15 +759,15 @@ virtio_pci_setup_queue_10(struct virtio_
bus_space_write_2(iot, ioh, VIRTIO_CONFIG1_QUEUE_SELECT, vq->vq_index);
if (addr == 0) {
bus_space_write_2(iot, ioh, VIRTIO_CONFIG1_QUEUE_ENABLE, 0);
- bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_DESC, 0);
- bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_AVAIL, 0);
- bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_USED, 0);
+ bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_DESC, 0);
+ bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_AVAIL, 0);
+ bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_USED, 0);
} else {
- bus_space_write_le_8(iot, ioh,
+ bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_DESC, addr);
- bus_space_write_le_8(iot, ioh,
+ bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_AVAIL, addr + vq->vq_availoffset);
- bus_space_write_le_8(iot, ioh,
+ bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_USED, addr + vq->vq_usedoffset);
bus_space_write_2(iot, ioh,
VIRTIO_CONFIG1_QUEUE_ENABLE, 1);
@@ -771,7 +783,6 @@ virtio_pci_setup_queue_10(struct virtio_
VIRTIO_CONFIG1_QUEUE_MSIX_VECTOR, vec);
}
}
-#undef bus_space_write_le_8
static void
virtio_pci_set_status_10(struct virtio_softc *sc, int status)
Re: CVS commit: src/sys/dev
> On Jan 21, 2021, at 12:00 AM, Martin Husemann > wrote: > > On Wed, Jan 20, 2021 at 07:46:48PM +, Reinoud Zandijk wrote: >> Module Name: src >> Committed By:reinoud >> Date:Wed Jan 20 19:46:48 UTC 2021 >> > [..] >> Log Message: >> Add VirtIO PCI v1.0 attachments and fix the drivers affected. >> >> * virtio on sparc64 attaches but is it not functioning though not a >> regression. > > While not a regression by this commit, it *did* work in netbsd-8, > so overall a bad regression that we should fix. This could be a bug in Qemu … it does not work on Alpha, either, and Jonathan Kollasch tracked down to Qemu 5’s Virtio subsystem not considering the DMA window on the Alpha platform. -- thorpej
Re: CVS commit: src/sys/dev
On Wed, Jan 20, 2021 at 07:46:48PM +, Reinoud Zandijk wrote: > Module Name: src > Committed By: reinoud > Date: Wed Jan 20 19:46:48 UTC 2021 > [..] > Log Message: > Add VirtIO PCI v1.0 attachments and fix the drivers affected. > > * virtio on sparc64 attaches but is it not functioning though not a > regression. While not a regression by this commit, it *did* work in netbsd-8, so overall a bad regression that we should fix. Martin
Re: CVS commit: src/sys/dev/wscons
The change isn't wrong, but I booted the wrong kernel and it does not fix the aarch64 issue I am seeing. On Sun, 17 Jan 2021, Jared D. McNeill wrote: Module Name:src Committed By: jmcneill Date: Sun Jan 17 15:13:15 UTC 2021 Modified Files: src/sys/dev/wscons: wsdisplay_vconsvar.h Log Message: Add appropriate memory barriers around sc_busy accesses. Fixes an issue where character cells are sometimes not erased properly on aarch64 at boot. To generate a diff of this commit: cvs rdiff -u -r1.28 -r1.29 src/sys/dev/wscons/wsdisplay_vconsvar.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
re: CVS commit: src/sys/dev/acpi
"Jason R Thorpe" writes: > Module Name: src > Committed By: thorpej > Date: Sat Jan 16 01:23:04 UTC 2021 > > Modified Files: > src/sys/dev/acpi: tpm_acpi.c > > Log Message: > Match PNP0C31 as a TPM 1.2 device. Works on my ThinkPad X260, and > eliminates the last of the devices that attach to "isa". cool. now try to remove all the "at isa" devices in your config :-) (it explodes last i tried.)
Re: CVS commit: src/sys/dev/pci/ixgbe
On 2020/12/11 14:01, SAITOH Masanobu wrote: Module Name:src Committed By: msaitoh Date: Fri Dec 11 05:01:19 UTC 2020 Modified Files: src/sys/dev/pci/ixgbe: ixgbe.c ixgbe_type.h Log Message: Don't use EIMC_OTHER bit because it's read only other than 82598. Documents say: 82598: All of bit 31(OTHER bit) of EIxx are reserved. In reality, at least EIMS_OTHER and EIMC_OTHER exist and the OTHER interrupt doesn't work without EIMS_OTHER. Other than 82598: + EICR's bit 31 is defined and other EIXX's bit 31 are reserved. + In reality, EIMS_OTHER is read only and EIMC_OTHER doesn't exist. If one of bit 29..16 is set, EIMS_OTHER is set to 1 (Note that bit 30(TCP timer isn't included)). Even if write bit 31 of EIMC to 1, it's ignored (EIMS_OTHER doesn't set). We introduced new spin mutex in ixgbe.c rev. 1.260, so it's OK to remove EIMC_OTHER stuff. We already set EIMS_OTHER in if_init(), so keep it for 82598. No functional change other than 82598. Another solution is to control bit 30..16 directly to mask/unmask interrupt instead of the mutex. TODO: Some MSI-X interrupt(LSC, module insertion/removal etc.)'s mask/unmask code between ixgbe_msix_admin() and ixgbe_handle_admin() may be wrong. It'll be fixed later. To generate a diff of this commit: cvs rdiff -u -r1.261 -r1.262 src/sys/dev/pci/ixgbe/ixgbe.c cvs rdiff -u -r1.45 -r1.46 src/sys/dev/pci/ixgbe/ixgbe_type.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/wsfb
Hi, On 2020/10/21 15:42, Michael wrote: Hello, On Sun, 18 Oct 2020 11:54:21 + "Rin Okuyama" wrote: Module Name:src Committed By: rin Date: Sun Oct 18 11:54:21 UTC 2020 Modified Files: src/sys/dev/wsfb: genfb.c Log Message: For WSDISPLAYIO_GET_FBINFO ioctl, set WSFB_VRAM_IS_RAM to fbi_flags when shadow FB is used. This flag is a hint for X, telling it that the memory used as framebuffer is regular RAM ( where using a shadowfb in X would be a waste of time and RAM ), not for example PCI RAM ( where a shadowfb would be faster ). In other words, it's supposed to tell X not to shadow the framebuffer. This is why it's not set by genfb itself, which doesn't have such knowledge about the hardware, but by drivers which do. Shadow fb use in wsdisplay is completely private and mapping the framebuffer through genfb will always give you whatever genfb thinks is the actual device framebuffer. Thank you for detailed explanation! I already reverted this commit as jmcneill@ advised me: http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/wsfb/genfb.c#rev1.77 Thanks, rin
Re: CVS commit: src/sys/dev/wsfb
Hello, On Sun, 18 Oct 2020 11:54:21 + "Rin Okuyama" wrote: > Module Name: src > Committed By: rin > Date: Sun Oct 18 11:54:21 UTC 2020 > > Modified Files: > src/sys/dev/wsfb: genfb.c > > Log Message: > For WSDISPLAYIO_GET_FBINFO ioctl, set WSFB_VRAM_IS_RAM to fbi_flags > when shadow FB is used. This flag is a hint for X, telling it that the memory used as framebuffer is regular RAM ( where using a shadowfb in X would be a waste of time and RAM ), not for example PCI RAM ( where a shadowfb would be faster ). In other words, it's supposed to tell X not to shadow the framebuffer. This is why it's not set by genfb itself, which doesn't have such knowledge about the hardware, but by drivers which do. Shadow fb use in wsdisplay is completely private and mapping the framebuffer through genfb will always give you whatever genfb thinks is the actual device framebuffer. have fun Michael
Re: CVS commit: src/sys/dev/wsfb
On 2020/10/18 21:18, Jared McNeill wrote: I think WSFB_VRAM_IS_RAM is meant to be a hint for what kind of memory you get when you mmap the device. When shadow FB is used, that is generally only used for rasops and mmap bypasses the shadow and uses device memory directly. So I think this could cause performance regressions on such hardware where shadow FB is enabled and reading VRAM is slow. What problem are you attempting to solve with this change? Ah, I misunderstood its intention. I will revert this commit. Thank you for pointing out! PS I came across this flag bit when I was examining byte-order problems of framebuffer on aarch64eb. I'm just going to send a message to you. I will soon! Thanks, rin
Re: CVS commit: src/sys/dev/wsfb
I think WSFB_VRAM_IS_RAM is meant to be a hint for what kind of memory you get when you mmap the device. When shadow FB is used, that is generally only used for rasops and mmap bypasses the shadow and uses device memory directly. So I think this could cause performance regressions on such hardware where shadow FB is enabled and reading VRAM is slow. What problem are you attempting to solve with this change? On Sun, 18 Oct 2020, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Sun Oct 18 11:54:21 UTC 2020 Modified Files: src/sys/dev/wsfb: genfb.c Log Message: For WSDISPLAYIO_GET_FBINFO ioctl, set WSFB_VRAM_IS_RAM to fbi_flags when shadow FB is used. To generate a diff of this commit: cvs rdiff -u -r1.74 -r1.75 src/sys/dev/wsfb/genfb.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Re: CVS commit: src/sys/dev/pci
On 2020/10/16 14:53, SAITOH Masanobu wrote: Module Name:src Committed By: msaitoh Date: Fri Oct 16 05:53:40 UTC 2020 Modified Files: src/sys/dev/pci: if_wm.c Log Message: Fixes a problem that the attach function reported "wm_gmii_setup_phytype: Unknown PHY model. OUI=00, model=" and "PHY type is still unknown." This was dmesg only problem. The SGMII read/write functions were correctly set even though error message was printed. This problem was added in if_wm.c rev. 1.656 which added SFP support. Don't call wm_gmii_setup_phytype() three times if the interface uses SGMII with internal MDIO. Tested with I354(Rangeley(SGMII(MDIO))) and I350(SERDES(SFP), SGMII(SFP)). To generate a diff of this commit: cvs rdiff -u -r1.690 -r1.691 src/sys/dev/pci/if_wm.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/pci
Hello, On Sun, 11 Oct 2020 21:41:57 + "Julian Coleman" wrote: > Module Name: src > Committed By: jdc > Date: Sun Oct 11 21:41:57 UTC 2020 > > Modified Files: > src/sys/dev/pci: radeonfb.c > > Log Message: ... > don't ignore the bottom 200 lines of the display (for no apparent reason)) The reason I have that in most of my drivers is so I can see a good part of the glyph cache, which starts right below the VRAM area used by wsdisplay. Most drivers use it only for anti-aliased fonts so you probably didn't see anything there... have fun Michael
Re: CVS commit: src/sys/dev/fdt
On 2020/10/02 22:42, Jonathan A. Kollasch wrote: On Sun, Jul 21, 2019 at 03:57:24PM +, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Sun Jul 21 15:57:24 UTC 2019 Modified Files: src/sys/dev/fdt: dw_apb_uart.c Log Message: The device cannot recognize break signal. Use + (five plus signs) as cnmagic in the same manner with bcm2835_com.c. To generate a diff of this commit: cvs rdiff -u -r1.4 -r1.5 src/sys/dev/fdt/dw_apb_uart.c This does not appear to be needed on at least one SoC (Allwinner H5). Which SoC did you have this problem on? Oops, you are right. It seems that my device (Allwinner A20) did not accept break signal because of WSDISPLAY_MULTICONS option. At that time, kernel just ignored break signal, but today it panics instead. Anyway, by disabling wsdisplay, I can enter DDB from console by break signal. I will revert this commit, and request pullup to netbsd-9. Thank you for finding it out, and I'm sorry for bothering you. rin
Re: CVS commit: src/sys/dev/fdt
On Sun, Jul 21, 2019 at 03:57:24PM +, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Sun Jul 21 15:57:24 UTC 2019 > > Modified Files: > src/sys/dev/fdt: dw_apb_uart.c > > Log Message: > The device cannot recognize break signal. Use + (five plus signs) as > cnmagic in the same manner with bcm2835_com.c. > > > To generate a diff of this commit: > cvs rdiff -u -r1.4 -r1.5 src/sys/dev/fdt/dw_apb_uart.c > This does not appear to be needed on at least one SoC (Allwinner H5). Which SoC did you have this problem on? Jonathan
Re: CVS commit: src/sys/dev/scsipi
I don't think this should be reverted, because LUN 0 must exist, but if there is no device on it, it will report "NOT PRESENT". We do not want the scan to stop in this case, but it should continue with other LUNs (such as those found through REPORT LUNS in the future). Kind regards, + Kimmo On Fri, Sep 18, 2020 at 03:04:25PM +, Jonathan A. Kollasch wrote: > Module Name: src > Committed By: jakllsch > Date: Fri Sep 18 15:04:25 UTC 2020 > > Modified Files: > src/sys/dev/scsipi: scsiconf.c > > Log Message: > Revert scsiconf.c 1.288, it only worked for LUN 1. > > vioscsi(4) now sets PQUIRK_FORCELUNS, which fixes the original issue for > all LUNs. > > To-do: should issue REPORT LUNS and use the information it returns to > probe LUNs in an optimized way. > > > To generate a diff of this commit: > cvs rdiff -u -r1.289 -r1.290 src/sys/dev/scsipi/scsiconf.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/dev/mii (PR/kern 55538)
Hi Martin ! That is strange - I didn't expect that, especially as the previous code was wrong with respect to state tracking. Can you check whether the addresses do not have the DEtACHED flag? You could try the dtrace script from the PR - it shows a little bit what is going on. There was also some mii fixing just before my patch, but I assume you testes with reverting just this commit. Sorry for the issue - I have yet to find an explaination for that behavior. Maybe comparing the dtrace outputs for both varainst can shed a light on what happens. Frank On 08/27/20 10:20, Martin Husemann wrote: On Mon, Aug 24, 2020 at 12:46:04PM +, Frank Kardel wrote: Module Name:src Committed By: kardel Date: Mon Aug 24 12:46:04 UTC 2020 Modified Files: src/sys/dev/mii: mii_physubr.c Log Message: Keep the change check invariant intact. The previous code could miss status updates by picking up a new status different from the tested status. This left addresses in the DETACHED state although the link status is already UP again. addresses PR/kern 55538 To generate a diff of this commit: cvs rdiff -u -r1.92 -r1.93 src/sys/dev/mii/mii_physubr.c Hi Frank, this change breaks the network on my macppc, with r1.93 it only seems to be able to send packets, but never receives answers (ARP does not complete, but other hosts see the ARP requests). gem0 at pci2 dev 15 function 0: Apple Computer GMAC Ethernet (rev. 0x01) gem0: interrupting at irq 41 brgphy0 at gem0 phy 0: BCM5411 1000BASE-T media interface, rev. 1 brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FDX, auto gem0: Ethernet address 00:03:93:71:ff:cc, 10KB RX fifo, 4KB TX fifo It is connected to a gige switch: media: Ethernet autoselect (1000baseT full-duplex,master) status: active (which looks the same in the non-working kernel). Any ideas how to debug? Martin
Re: CVS commit: src/sys/dev/mii (PR/kern 55538)
On Mon, Aug 24, 2020 at 12:46:04PM +, Frank Kardel wrote: > Module Name: src > Committed By: kardel > Date: Mon Aug 24 12:46:04 UTC 2020 > > Modified Files: > src/sys/dev/mii: mii_physubr.c > > Log Message: > Keep the change check invariant intact. The previous code could miss > status updates by picking up a new status different from the tested > status. This left addresses in the DETACHED state although the > link status is already UP again. > > addresses PR/kern 55538 > > > To generate a diff of this commit: > cvs rdiff -u -r1.92 -r1.93 src/sys/dev/mii/mii_physubr.c Hi Frank, this change breaks the network on my macppc, with r1.93 it only seems to be able to send packets, but never receives answers (ARP does not complete, but other hosts see the ARP requests). gem0 at pci2 dev 15 function 0: Apple Computer GMAC Ethernet (rev. 0x01) gem0: interrupting at irq 41 brgphy0 at gem0 phy 0: BCM5411 1000BASE-T media interface, rev. 1 brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FDX, auto gem0: Ethernet address 00:03:93:71:ff:cc, 10KB RX fifo, 4KB TX fifo It is connected to a gige switch: media: Ethernet autoselect (1000baseT full-duplex,master) status: active (which looks the same in the non-working kernel). Any ideas how to debug? Martin
Re: CVS commit: src/sys/dev/mii
On Tue, Aug 04, 2020 at 07:12:54 +0300, Valery Ushakov wrote: > On Tue, Aug 04, 2020 at 12:50:11 +0900, SAITOH Masanobu wrote: > > > On 2020/08/03 23:00, Valeriy E. Ushakov wrote: > > > Module Name: src > > > Committed By: uwe > > > Date: Mon Aug 3 14:00:41 UTC 2020 > > > > > > Modified Files: > > > src/sys/dev/mii: miidevs_data.h > > > > > > Log Message: > > > mii_knowndevs[] is de facto const, define it as such. > > > > This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs > > deletes this change. If the change is required, modify Makefile.miidevs. > > Oh, thank you for the heads up. I was really working on something > else and didn't pay attention to the comment that was out off view. I have fixed the devlist2h.awk script that generates them to emit that const. As the generated files come out exactly the same modulo the rcs id (script emits unexpanded one) I think we can pretend I have committed the script change first and then regenerated the miidevs_data.h header :) -uwe
Re: CVS commit: src/sys/dev/mii
On 2020/08/04 12:50, SAITOH Masanobu wrote: Hi. On 2020/08/03 23:00, Valeriy E. Ushakov wrote: Module Name: src Committed By: uwe Date: Mon Aug 3 14:00:41 UTC 2020 Modified Files: src/sys/dev/mii: miidevs_data.h Log Message: mii_knowndevs[] is de facto const, define it as such. To generate a diff of this commit: cvs rdiff -u -r1.153 -r1.154 src/sys/dev/mii/miidevs_data.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs deletes this change. If the change is required, modify Makefile.miidevs. s/modify Makefile.miidevs/modify devlist2h.awk/ Thanks. -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/mii
On Tue, Aug 04, 2020 at 12:50:11 +0900, SAITOH Masanobu wrote: > On 2020/08/03 23:00, Valeriy E. Ushakov wrote: > > Module Name:src > > Committed By: uwe > > Date: Mon Aug 3 14:00:41 UTC 2020 > > > > Modified Files: > > src/sys/dev/mii: miidevs_data.h > > > > Log Message: > > mii_knowndevs[] is de facto const, define it as such. > > This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs > deletes this change. If the change is required, modify Makefile.miidevs. Oh, thank you for the heads up. I was really working on something else and didn't pay attention to the comment that was out off view. -uwe
Re: CVS commit: src/sys/dev/mii
Hi. On 2020/08/03 23:00, Valeriy E. Ushakov wrote: Module Name:src Committed By: uwe Date: Mon Aug 3 14:00:41 UTC 2020 Modified Files: src/sys/dev/mii: miidevs_data.h Log Message: mii_knowndevs[] is de facto const, define it as such. To generate a diff of this commit: cvs rdiff -u -r1.153 -r1.154 src/sys/dev/mii/miidevs_data.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs deletes this change. If the change is required, modify Makefile.miidevs. Thanks. -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
re: CVS commit: src/sys/dev
"Jaromir Dolecek" writes: > Module Name: src > Committed By: jdolecek > Date: Tue Jul 28 09:36:05 UTC 2020 > > Modified Files: > src/sys/dev/ic: nvmevar.h > src/sys/dev/pci: nvme_pci.c > > Log Message: > add a quirk to disable MSI, and enable it for Intel SSD DC P4500 > > this device seems to cause serious system responsiveness issues when > configured > to use MSI, while it works fine when configured for either INTx or MSI-X > > this is important so this works well under Xen Dom0, which doesn't > support MSI-X yet > > fixes another issue reported as feedback for PR port-xen/55285 by Frank Kardel i wonder if the label "force_intx" should be renamed now that it can be active _after_ msi-x. can't think of a good name, "skip_to_intx" maybe useful? thanks. .mrg.
Re: CVS commit: src/sys/dev/pci
One of the things which need to be done is calling the if_ioctl always with the IFNET_LOCK() held. Right now it sometimes is, and other times it is not, so it's not possible to rely on it and KASSERT(). As for bnx(4) I did just some basic fixes around making it work with MSI(-X), since I don't really have easy access to the hw for testing. This might change soon, then I might look into making it NET_MPSAFE, after some other bug fixes. Jaromir Le sam. 18 juil. 2020 à 00:54, Jason Thorpe a écrit : > > > > > On Jul 17, 2020, at 3:50 PM, matthew green wrote: > > > > any chance you can look at NET_MPSAFE here etc? :) > > I have a bunch of local changes for this in one of my trees, and I hope to > get back to it after netbsd-10 branches. > > -- thorpej >
Re: CVS commit: src/sys/dev/pci
> On Jul 17, 2020, at 3:50 PM, matthew green wrote: > > any chance you can look at NET_MPSAFE here etc? :) I have a bunch of local changes for this in one of my trees, and I hope to get back to it after netbsd-10 branches. -- thorpej
re: CVS commit: src/sys/dev/pci
"Jaromir Dolecek" writes: > Module Name: src > Committed By: jdolecek > Date: Fri Jul 17 09:48:21 UTC 2020 > > Modified Files: > src/sys/dev/pci: if_bnx.c > > Log Message: > re-enable MSI/MSI-X, the TX timeouts were caused by the IFF_OACTIVE handling, > which was fixed in previous revision "fixed IFF_OACTIVE" in modern netbsd means "removed IFF_OACTIVE and handled it in the driver", as the flag is not SMP friendly. any chance you can look at NET_MPSAFE here etc? :) thanks. .mrg.
Re: CVS commit: src/sys/dev/i2c
Hi Michael,
Perhaps your commit missed some changes? The code no longer compiles.
Cheers,
+ Kimmo
/p/netbsd/cvs/src/sys/dev/i2c/dbcool.c: In function 'dbcool_attach':
/p/netbsd/cvs/src/sys/dev/i2c/dbcool.c:778:4: error: 'struct dbcool_softc' has
no member named 'sc_prop'
sc->sc_prop = args->ia_prop;
^~
/p/netbsd/cvs/src/sys/dev/i2c/dbcool.c: In function 'dbcool_attach_sensor':
/p/netbsd/cvs/src/sys/dev/i2c/dbcool.c:1698:43: error: 'struct dbcool_softc'
has no member named 'sc_prop'
if (prop_dictionary_get_cstring_nocopy(sc->sc_prop, name, )) {
On Sun, Jul 12, 2020 at 06:42:33AM +, Michael Lorenz wrote:
> Module Name: src
> Committed By: macallan
> Date: Sun Jul 12 06:42:33 UTC 2020
>
> Modified Files:
> src/sys/dev/i2c: dbcool.c
>
> Log Message:
> in sysctl_dbcool_behavior() - actually use the array index when translating
> text from sysctl -w *.behavior=
> now this actually works on my sb2500's two adm1030s
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.55 -r1.56 src/sys/dev/i2c/dbcool.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
Re: CVS commit: src/sys/dev/scsipi
On Sun, Jul 12, 2020 at 12:05:37AM +0700, Robert Elz wrote: > Just to make things clear here, the LUN you're talking about is not > the scsi unit number (which is what I think Martin was referring to) > but a sub-device number within a single scsi ID. Right? Correct. I should have written "SCSI target" instead of "SCSI bus" in my commit message to avoid confusion. In both of the use cases I highlighted (Proxmox and Linode), the disks are always on SCSI ID 0. However, the "problematic" disks are on LUN 1 instead of LUN 0. Out of curiosity, I just added a "scsi2" disk to the Proxmox VM. It has been placed on LUN 2, and NetBSD does not pick it up even with this patch... FreeBSD does (although the earlier LUNs are clearly causing unexpected output from the "pass" attachments), so I guess I might be looking at this a bit more. It would be nice to have (at least debug) output about the LUN that terminates the scan loop. Will probably open a ticket with Proxmox, too, in an attempt to put a stop to this unnecessary wasteful skipping of perfectly good LUN numbers. + Kimmo
Re: CVS commit: src/sys/dev/scsipi
Date:Sat, 11 Jul 2020 18:24:51 +0300 From:Kimmo Suominen Message-ID: <20200711152451.ga1...@homeworld.netbsd.org> | On Sat, Jul 11, 2020 at 05:00:02PM +0200, Martin Husemann wrote: | > I don't understand the change. When was this broken? This has always worked | > for me e.g. with the sd0 at LUN 3 and the controller at 6 or 7. | | I think all real SCSI hardware I've had has always just only had LUN 0, | and each disk has been on its own SCSI ID (target). Just to make things clear here, the LUN you're talking about is not the scsi unit number (which is what I think Martin was referring to) but a sub-device number within a single scsi ID. Right? In real scsi hardware, the only place I think I've ever seen other than LUN 0 is in a raid array device, where there is a single scsi bus attachment, with a single ID, and then each raid volume created gets a different LUN (not all scsi raids work that way I think, but some do). kre
Re: CVS commit: src/sys/dev/scsipi
On Sat, Jul 11, 2020 at 06:24:51PM +0300, Kimmo Suominen wrote: > I think all real SCSI hardware I've had has always just only had LUN 0, > and each disk has been on its own SCSI ID (target). Yes, I confused ID and LUN here - just ignore me. Martin
Re: CVS commit: src/sys/dev/scsipi
On Sat, Jul 11, 2020 at 05:00:02PM +0200, Martin Husemann wrote: > I don't understand the change. When was this broken? This has always worked > for me e.g. with the sd0 at LUN 3 and the controller at 6 or 7. I think all real SCSI hardware I've had has always just only had LUN 0, and each disk has been on its own SCSI ID (target). > Or is there something special in your setup? Well, this is how Proxmox and Linode do it, and with this change it is possible to install and use NetBSD on those platforms. Of course, anyone running their own Proxmox could just avoid the "Virtio SCSI single" controller type (or SCSI disks altogether), but there is no user or admin control for specifying the LUN ID of a disk. With a platform like Linode, you are stuck with the configuration it creates for you from a boot profile (or other UI object). Although Linode is usually responsive to bug reports, here it is not clear if the bug is in how they do things or how NetBSD behaves. With this change it is possible to run the NetBSD installer just like the FreeBSD one (or any other "custom distro") on Linode. Kind regards, + Kimmo
Re: CVS commit: src/sys/dev/scsipi
On Sat, Jul 11, 2020 at 05:57:46PM +0300, Kimmo Suominen wrote: > On Sat, Jul 11, 2020 at 05:47:34PM +0300, Jukka Ruohonen wrote: > > I'd reckon a pullup to NetBSD 9 would be in order? > > Yes, I was just waiting to be able to link to mail-index. I had > already checked that the patch applies cleanly to both netbsd-9 > and netbsd-8. I don't understand the change. When was this broken? This has always worked for me e.g. with the sd0 at LUN 3 and the controller at 6 or 7. Or is there something special in your setup? Martin
Re: CVS commit: src/sys/dev/scsipi
On Sat, Jul 11, 2020 at 05:47:34PM +0300, Jukka Ruohonen wrote: > I'd reckon a pullup to NetBSD 9 would be in order? Yes, I was just waiting to be able to link to mail-index. I had already checked that the patch applies cleanly to both netbsd-9 and netbsd-8. http://releng.netbsd.org/cgi-bin/req-9.cgi?show=1000 http://releng.netbsd.org/cgi-bin/req-8.cgi?show=1571 Cheers, + Kimmo
Re: CVS commit: src/sys/dev/scsipi
On Sat, Jul 11, 2020 at 02:31:46PM +, Kimmo Suominen wrote:
> Use case 2: A Linode boot profile with multiple disks results in
> the first disk ("sda") on LUN 1, while the second disk ("sdb") is
> on LUN 0, each on their own bus.
As Linode is quite popular, and supposedly uses a rather similar setup to
its competitors (e.g., Vultr), I'd reckon a pullup to NetBSD 9 would be in
order? Some of these (e.g., netcup.eu in Europe) even offer off-the-shelf
NetBSD 9 images.
- Jukka
re: CVS commit: src/sys/dev/ofw
"Martin Husemann" writes: > Module Name: src > Committed By: martin > Date: Fri Jun 26 10:14:32 UTC 2020 > > Modified Files: > src/sys/dev/ofw: ofw_subr.c > > Log Message: > Remove !cold KASSERT - it does not compile on all kernels, and it is not the > right thing to test for anyway. XXX should we panic instead? Are "compatible" > strings this long happening in real devices? IMO, best to panic with a clear message about what is wrong. .mrg.
Re: CVS commit: src/sys/dev/ic
In article <18083.1593053...@splode.eterna.com.au>, matthew green wrote: >"Jaromir Dolecek" writes: >> Module Name: src >> Committed By:jdolecek >> Date:Wed Jun 24 19:55:25 UTC 2020 >> >> Modified Files: >> src/sys/dev/ic: ibm561.c >> >> Log Message: >> avoid allocating almost 5k struct ibm561data on stack in ibm561_cninit(); >> it's too early for kmem_alloc(), so use static variable in BSS > >this seems particularly wasteful for a driver that won't >be useful for most systems. > >seems like a candidate for allow-listing instead, and as >it seems to only be relevant for alpha systems, that have >a fairly large stack (16K), and this will be called with >a fairly short call stack. I agree; the BSS kludge is ugly in general and should only be used sparingly. christos
re: CVS commit: src/sys/dev/ic
"Jaromir Dolecek" writes: > Module Name: src > Committed By: jdolecek > Date: Wed Jun 24 19:55:25 UTC 2020 > > Modified Files: > src/sys/dev/ic: ibm561.c > > Log Message: > avoid allocating almost 5k struct ibm561data on stack in ibm561_cninit(); > it's too early for kmem_alloc(), so use static variable in BSS this seems particularly wasteful for a driver that won't be useful for most systems. seems like a candidate for allow-listing instead, and as it seems to only be relevant for alpha systems, that have a fairly large stack (16K), and this will be called with a fairly short call stack. .mrg.
Re: CVS commit: src/sys/dev/acpi
On Mon, Jun 22, 2020 at 04:14:18PM +, Maxime Villard wrote: > Module Name: src > Committed By: maxv > Date: Mon Jun 22 16:14:18 UTC 2020 > > Modified Files: > src/sys/dev/acpi: acpi.c > > Log Message: > Fix memory leak. Found by kLSan. > + > + default: > + ACPI_FREE(devinfo); > + break; > } > > return AE_OK; I think it leaks more, i.e. AcpiGetObjectInfo() always allocates a buffer. - Jukka
Re: CVS commit: src/sys/dev/acpi
On 10/08/2018 18:11, Taylor R Campbell wrote: Module Name:src Committed By: riastradh Date: Fri Aug 10 17:11:56 UTC 2018 Modified Files: src/sys/dev/acpi: acpi_bat.c Log Message: Don't hold up boot: defer acpibat(4) inquiry until threads are running. ok jmcneill@ This makes an old hp510 laptop reset without warning around about dhcpcd time. I don't if it matters but the battery doesn't charge anymore. Nick
Re: [virtio] Re: CVS commit: src/sys/dev/pci
Le 01/06/2020 à 03:23, Shoichi Yamaguchi a écrit : Hi, On Wed, May 27, 2020 at 8:47 PM Shoichi Yamaguchi wrote: I modified virtio(4) not to allocate unused memory. I guess it fixes the issue. Could you check this? I confirmed your closing the report on syzbot. https://syzkaller.appspot.com/bug?id=73f690e8a3d8e304407808555f1dacfa004685d1 Thank you for your response. Sorry for my lack of response -- I was waiting for the kMSan instance to sync, but it is currently down, and has been down for four days and a half now. The kMSan instance got the time to run 24h before it broke for unrelated reasons. 24h before your patch, it triggered the bug 6 times. 24h after your patch, it triggered the bug 0 times. So indeed, we can call it fixed, thanks for the fix
Re: [virtio] Re: CVS commit: src/sys/dev/pci
Hi, On Wed, May 27, 2020 at 8:47 PM Shoichi Yamaguchi wrote: > > I modified virtio(4) not to allocate unused memory. > I guess it fixes the issue. > > Could you check this? I confirmed your closing the report on syzbot. https://syzkaller.appspot.com/bug?id=73f690e8a3d8e304407808555f1dacfa004685d1 Thank you for your response. Regards, yamaguchi
Re: CVS commit: src/sys/dev
Jaromir Dolecek wrote:
> Index: src/sys/dev/ic/bwfmvar.h
> diff -u src/sys/dev/ic/bwfmvar.h:1.9 src/sys/dev/ic/bwfmvar.h:1.10
> --- src/sys/dev/ic/bwfmvar.h:1.9 Sat May 30 13:41:58 2020
> +++ src/sys/dev/ic/bwfmvar.h Sat May 30 15:55:47 2020
> @@ -1,4 +1,4 @@
> -/* $NetBSD: bwfmvar.h,v 1.9 2020/05/30 13:41:58 jdolecek Exp $ */
> +/* $NetBSD: bwfmvar.h,v 1.10 2020/05/30 15:55:47 jdolecek Exp $ */
> /* $OpenBSD: bwfmvar.h,v 1.1 2017/10/11 17:19:50 patrick Exp $ */
> /*
> * Copyright (c) 2010-2016 Broadcom Corporation
> @@ -214,6 +214,11 @@ struct bwfm_softc {
> enum ieee80211_state, int);
>
> int sc_bcdc_reqid;
> +
> + union {
> + struct bwfm_bss_info bss_info;
> + uint8_t padding[BWFM_BSS_INFO_BUFLEN];
> + } sc_bss_buf;
> };
I think you miss
#include
where BWFM_BSS_INFO_BUFLEN is defined.
--
Alex
Re: CVS commit: src/sys/dev/audio
On Sat, May 30, 2020 at 09:48:36PM +0900, Tetsuya Isaki wrote: > I will do it on next weekend. > > Thanks, > --- > Tetsuya Isaki Thank you.
Re: CVS commit: src/sys/dev/audio
At Fri, 29 May 2020 12:32:39 +, nia wrote: > OK... Can you request a pullup to ensure resuming with a stream > playing doesn't panic on 9.1? I will do it on next weekend. Thanks, --- Tetsuya Isaki
Re: CVS commit: src/sys/dev/audio
OK... Can you request a pullup to ensure resuming with a stream playing doesn't panic on 9.1? Playing audio is very distorted on resume, but that can be resolved by killing the streams...
Re: CVS commit: src/sys/dev/audio
At Wed, 27 May 2020 13:19:22 +, nia wrote: > I think this is because audio_rmixer_start is used unguarded > in audio_open (it doesn't check for the sc_rbusy flag). > This isn't the case for pmixer. > > So, if the audio device is opened for recording for the > first time after system resumption, a panic will occur > due to an assertion failure (the recording mixer would > already be busy). It's because your change didn't restore [pr]mixer's running state correctly. I have fixed it. Thanks, --- Tetsuya Isaki
Re: CVS commit: src/sys/dev/usb
On Wed, 27 May 2020, Taylor R Campbell wrote: Not really, because we just need to know whether usb_once_init has been run. OK, great! Now, should we use something other than RUN_ONCE, which can both set up and tear down? Sure, that might be better in principle, but there probably aren't that many systems that have hotpluggable USB in which you might unplug _all_ of the USBs and where you really want to save the cost of a couple kernel threads. So not likely worth much effort. I was thinking more in terms of someone using drvctl(8) to cause the detach. But yeah, it's not a very common use-case, so as long as we don't _need_ the decrement, it's not worth losing any sleep. :) Thanks for the reply. ++--+---+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | ++--+---+
Re: CVS commit: src/sys/dev/usb
> Date: Wed, 27 May 2020 05:28:41 -0700 (PDT) > From: Paul Goyette > > Do you also need to decrement the number of busses when one is > detached? Not really, because we just need to know whether usb_once_init has been run. Now, should we use something other than RUN_ONCE, which can both set up and tear down? Sure, that might be better in principle, but there probably aren't that many systems that have hotpluggable USB in which you might unplug _all_ of the USBs and where you really want to save the cost of a couple kernel threads. So not likely worth much effort.
Re: CVS commit: src/sys/dev/audio
On Wed, May 27, 2020 at 09:46:04PM +0900, Tetsuya Isaki wrote: > Why are playback and recording asymmetric? > > Thanks, I think this is because audio_rmixer_start is used unguarded in audio_open (it doesn't check for the sc_rbusy flag). This isn't the case for pmixer. So, if the audio device is opened for recording for the first time after system resumption, a panic will occur due to an assertion failure (the recording mixer would already be busy). If there's an advantage to not starting the playback mixer on resume if no devices were previously opened we can do that too?
Re: CVS commit: src/sys/dev/audio
nia, At Tue, 26 May 2020 15:20:16 +, Nia Alarie wrote: > Module Name: src > Committed By: nia > Date: Tue May 26 15:20:16 UTC 2020 > > Modified Files: > src/sys/dev/audio: audio.c > > Log Message: > audio: Only restart recording mixer on resume if it's already been started > > > To generate a diff of this commit: > cvs rdiff -u -r1.73 -r1.74 src/sys/dev/audio/audio.c Why are playback and recording asymmetric? Thanks, --- Tetsuya Isaki
Re: CVS commit: src/sys/dev/usb
Do you also need to decrement the number of busses when one is detached? On Wed, 27 May 2020, Nick Hudson wrote: Module Name:src Committed By: skrll Date: Wed May 27 07:17:45 UTC 2020 Modified Files: src/sys/dev/usb: usb.c Log Message: Don't allow open of /dev/usb if there are no attached busses. PR kern/55303 mutex_vector_enter,512: uninitialized lock To generate a diff of this commit: cvs rdiff -u -r1.186 -r1.187 src/sys/dev/usb/usb.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:5ece145a266021866921056! ++--+---+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | ++--+---+
Re: [virtio] Re: CVS commit: src/sys/dev/pci
Hi, On Wed, May 27, 2020 at 2:20 AM Maxime Villard wrote: > > Hi, > I don't know if this is related to your changes, but kMSan detected one uninit > variable in virtio 3h ago: > > https://syzkaller.appspot.com/text?tag=CrashReport=12084ef610 > > [ 153.4370851] panic: MSan: Uninitialized Kmem Memory From > virtio_pci_setup_interrupts() > [ 153.4448669] cpu0: Begin traceback... > [ 153.4448669] vpanic() at netbsd:vpanic+0x7c1 sys/kern/subr_prf.c:288 > [ 153.4632004] panic() at netbsd:panic+0x1ad sys/kern/subr_prf.c:209 > [ 153.4734357] __msan_warning() at netbsd:__msan_warning+0xe7 > kmsan_report_inline sys/kern/subr_msan.c:239 [inline] > [ 153.4734357] __msan_warning() at netbsd:__msan_warning+0xe7 > sys/kern/subr_msan.c:612 > [ 153.4931985] virtio_pci_free_interrupts() at > netbsd:virtio_pci_free_interrupts+0x1b4 sys/dev/pci/virtio_pci.c:740 > [ 153.5132006] virtio_child_detach() at > netbsd:virtio_child_detach+0x116 sys/dev/pci/virtio.c:924 > [ 153.5331982] vioscsi_detach() at netbsd:vioscsi_detach+0x40d > sys/dev/pci/vioscsi.c:244 > [ 153.5532009] config_detach() at netbsd:config_detach+0x7e3 > sys/kern/subr_autoconf.c:1760 > [ 153.5732017] config_detach_all() at netbsd:config_detach_all+0x29a > sys/kern/subr_autoconf.c:1906 > [ 153.5831984] cpu_reboot() at netbsd:cpu_reboot+0x290 > sys/arch/amd64/amd64/machdep.c:700 > [ 153.6031986] kern_reboot() at netbsd:kern_reboot+0x18f > sys/kern/kern_reboot.c:73 > [ 153.6231980] sys_reboot() at netbsd:sys_reboot+0x28d > > This means that some memory allocated by virtio_pci_setup_interrupts() on > the kmem allocator was not initialized, and later one access to it was made > by virtio_pci_free_interrupts() at l.740 of the file. Thank you for your pointed out. I modified virtio(4) not to allocate unused memory. I guess it fixes the issue. Could you check this? Thanks, yamaguchi
[virtio] Re: CVS commit: src/sys/dev/pci
Hi, I don't know if this is related to your changes, but kMSan detected one uninit variable in virtio 3h ago: https://syzkaller.appspot.com/text?tag=CrashReport=12084ef610 [ 153.4370851] panic: MSan: Uninitialized Kmem Memory From virtio_pci_setup_interrupts() [ 153.4448669] cpu0: Begin traceback... [ 153.4448669] vpanic() at netbsd:vpanic+0x7c1 sys/kern/subr_prf.c:288 [ 153.4632004] panic() at netbsd:panic+0x1ad sys/kern/subr_prf.c:209 [ 153.4734357] __msan_warning() at netbsd:__msan_warning+0xe7 kmsan_report_inline sys/kern/subr_msan.c:239 [inline] [ 153.4734357] __msan_warning() at netbsd:__msan_warning+0xe7 sys/kern/subr_msan.c:612 [ 153.4931985] virtio_pci_free_interrupts() at netbsd:virtio_pci_free_interrupts+0x1b4 sys/dev/pci/virtio_pci.c:740 [ 153.5132006] virtio_child_detach() at netbsd:virtio_child_detach+0x116 sys/dev/pci/virtio.c:924 [ 153.5331982] vioscsi_detach() at netbsd:vioscsi_detach+0x40d sys/dev/pci/vioscsi.c:244 [ 153.5532009] config_detach() at netbsd:config_detach+0x7e3 sys/kern/subr_autoconf.c:1760 [ 153.5732017] config_detach_all() at netbsd:config_detach_all+0x29a sys/kern/subr_autoconf.c:1906 [ 153.5831984] cpu_reboot() at netbsd:cpu_reboot+0x290 sys/arch/amd64/amd64/machdep.c:700 [ 153.6031986] kern_reboot() at netbsd:kern_reboot+0x18f sys/kern/kern_reboot.c:73 [ 153.6231980] sys_reboot() at netbsd:sys_reboot+0x28d This means that some memory allocated by virtio_pci_setup_interrupts() on the kmem allocator was not initialized, and later one access to it was made by virtio_pci_free_interrupts() at l.740 of the file. Can you have a look? Thanks, Maxime
Re: CVS commit: src/sys/dev/pci
On 20/05/2020 21:18, Sevan Janiyan wrote: > Bump rcs tag which was missed in r1.9 That should've been r1.10 Sevan
Re: Freeze or panic during boot was: Re: CVS commit: src/sys/dev/ata
On Sat, 2 May 2020, 20:10 Jason Thorpe, wrote: > > > On May 1, 2020, at 1:07 PM, Ryo ONODERA wrote: > > > > Hi, > > > > Have you missed this thread? > > > > If the problem requires more time to investigate, > > could you consider to revert ata change for a while? > > > > Thank you. > > I backed it out, but would appreciate some help tracking down the issue > because no other problems were reported other than on these specific > machines. > The T480 is my main machine but I'm happy to boot any test kernels (to confirm, - current as of the 30th with just that one commit reverted runs fine) Thanks David >
Re: Freeze or panic during boot was: Re: CVS commit: src/sys/dev/ata
> On May 1, 2020, at 1:07 PM, Ryo ONODERA wrote: > > Hi, > > Have you missed this thread? > > If the problem requires more time to investigate, > could you consider to revert ata change for a while? > > Thank you. I backed it out, but would appreciate some help tracking down the issue because no other problems were reported other than on these specific machines. -- thorpej
Freeze or panic during boot was: Re: CVS commit: src/sys/dev/ata
Hi, Have you missed this thread? If the problem requires more time to investigate, could you consider to revert ata change for a while? Thank you. Alexander Nasonov writes: > David Brownlee wrote: >> Just another data point - seeing this same panic on a T480 with the >> latest kernel from nyftp > > Same problem on T470. > > -- > Alex -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev/ata
Plus to confirm reverting just this commit from today's github copy of current (d5b32e03eac8b05d38a143ee0ec615efb2233201) boots fine on the T480 Thanks On Thu, 30 Apr 2020 at 00:12, Alexander Nasonov wrote: > > David Brownlee wrote: > > Just another data point - seeing this same panic on a T480 with the > > latest kernel from nyftp > > Same problem on T470. > > -- > Alex
Re: CVS commit: src/sys/dev/ata
David Brownlee wrote: > Just another data point - seeing this same panic on a T480 with the > latest kernel from nyftp Same problem on T470. -- Alex
Re: CVS commit: src/sys/dev/ata
Just another data point - seeing this same panic on a T480 with the latest kernel from nyftp
Re: CVS commit: src/sys/dev/ata
Ryo ONODERA writes: > Hi, > > After this commit, NetBSD/amd64-current on my HP Spectre x360 > freezes after acpiacad0 detection (before ld0 detection). > Reverting this commit in latest tree fixes my freeze problem. > > Could you take a look at my problem? > > Thank you. > > === === === > cpu7: CPU max freq 40 Hz > cpu7: TSC freq 199200 Hz > timecounter: Timecounter "TSC" frequency 199200 Hz quality 3000 > uhub0 at usb0: NetBSD (0x) xHCI root hub (0x), class 9/0, rev > 3.00/1.00, > addr 0 > uhub0: 6 ports with 6 removable, self powered > uhub1 at usb1: NetBSD (0x) xHCI root hub (0x), class 9/0, rev > 2.00/1.00, > addr 0 > uhub1: 12 ports with 12 removable, self powered > acpiacad0: AC adapter online. > > (With this commit, freeze here) > > ld0: GPT GUID: 3fda58df-424f-4b48-9fb9-b4c5c037379e > dk0 at ld0: "EFI", 262144 blocks at 2048, type: ntfs > dk1 at ld0: "ROOT", 66955885 blocks at 266240, type: ffs > === === === With LOCKDEBUG option, I have gotten the following panic messages: panic: TAILQ_INSERT_TAIL 0xd305ab82ae0 /usr/src/sys/dev/pckbport/pckbport.c:531 cpu0: Begin traceback... vpanic() at netbsd:vpanic+0x178 snprintf() at netbsd:snprintf pckbport_enqueue_cmd() at netbsd:pckbport_enqueue_cmd+0x3c0 pms_reset_thread() at netbsd:pms_reset_thread+0x94 cpu0: End traceback... Thank you. -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev/ata
Hi, After this commit, NetBSD/amd64-current on my HP Spectre x360 freezes after acpiacad0 detection (before ld0 detection). Reverting this commit in latest tree fixes my freeze problem. Could you take a look at my problem? Thank you. === === === cpu7: CPU max freq 40 Hz cpu7: TSC freq 199200 Hz timecounter: Timecounter "TSC" frequency 199200 Hz quality 3000 uhub0 at usb0: NetBSD (0x) xHCI root hub (0x), class 9/0, rev 3.00/1.00, addr 0 uhub0: 6 ports with 6 removable, self powered uhub1 at usb1: NetBSD (0x) xHCI root hub (0x), class 9/0, rev 2.00/1.00, addr 0 uhub1: 12 ports with 12 removable, self powered acpiacad0: AC adapter online. (With this commit, freeze here) ld0: GPT GUID: 3fda58df-424f-4b48-9fb9-b4c5c037379e dk0 at ld0: "EFI", 262144 blocks at 2048, type: ntfs dk1 at ld0: "ROOT", 66955885 blocks at 266240, type: ffs === === === "Jason R Thorpe" writes: > Module Name: src > Committed By: thorpej > Date: Sat Apr 25 00:07:27 UTC 2020 > > Modified Files: > src/sys/dev/ata: ata.c ata_subr.c atavar.h > > Log Message: > Rather than creating a kthread-per-channel, use a threadpool and a > threadpool-job-per-channel for the in-thread-context work that needs > to be done (which is rare). > > On one of my test systems, this results in the total number of LWPs > after multi-user boot dropping from 116 to 78. > > > To generate a diff of this commit: > cvs rdiff -u -r1.155 -r1.156 src/sys/dev/ata/ata.c > cvs rdiff -u -r1.9 -r1.10 src/sys/dev/ata/ata_subr.c > cvs rdiff -u -r1.105 -r1.106 src/sys/dev/ata/atavar.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
[disk changes] CVS commit: src/sys/dev/dkwedge
> Module Name:src > Committed By: jdolecek > Date: Sat Apr 11 16:00:34 UTC 2020 > > Modified Files: > src/sys/dev/dkwedge: dkwedge_apple.c dkwedge_bsdlabel.c dkwedge_gpt.c > dkwedge_mbr.c dkwedge_rdb.c It appears that since your recent changes, there is a systematic use-after-free: panic: ASan: Unauthorized Access in 0x...: Addr 0x... [2 bytes, read, PoolUseAfterFree] wdc_ata_bio() wdstart1() wd_diskstart() dk_start() bdev_strategy() spec_strategy() VOP_STRATEGY() genfs_getpages() VOP_GETPAGES() ubc_fault() uvm_fault_internal() trap() --- trap (number 6) --- copyout() uiomove() ubc_uiomove() ffs_read() VOP_READ() vn_read() dofileread() sys_read() syscall() This is reliably reproductible by just booting KASAN on amd64. Can you give a look? Thanks, Maxime
Re: CVS commit: src/sys/dev/usb
On Thu, Apr 2, 2020 at 8:37 PM Nick Hudson wrote: > > Module Name:src > Committed By: skrll > Date: Thu Apr 2 11:37:23 UTC 2020 > > Modified Files: > src/sys/dev/usb: xhci.c xhcivar.h > > Log Message: > Reduce the memory footprint by allocating a ring per endpoint/pipe on > pipe open. > > From sc.dying on tech-kern Thank you for applying the patch.
Re: CVS commit: src/sys/dev/ic
On 2020/03/24 12:45, SAITOH Masanobu wrote: > Module Name: src > Committed By: msaitoh > Date: Tue Mar 24 03:45:26 UTC 2020 > > Modified Files: > src/sys/dev/ic: spdmem.c spdmemvar.h > > Log Message: > - Define some new parameters of DDR3 SPD ROM. > - Use fine timebase parameters for time calculation on DDR3. This change > makes PC3- value + and tAA-tRCD-tRP value > more correctly on newer DD3.> > > To generate a diff of this commit: > cvs rdiff -u -r1.33 -r1.34 src/sys/dev/ic/spdmem.c > cvs rdiff -u -r1.14 -r1.15 src/sys/dev/ic/spdmemvar.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/usb
Le 23/03/2020 à 04:07, Roy Marples a écrit : > On 22/03/2020 08:30, Maxime Villard wrote: >> Overall "From OpenBSD" is a redflag for buggy and vulnerable code.. > > We should be above this, no software is perfect, not even ours. > > Roy You seem to be confusing one-off defects and structural deficiencies. That a plane crashes because of one slightly malformed screw, is a one-off defect. Yes, sh*t happens, that's statistical, and in the order of things. That a plane crashes because pilots have trained on a faulty simulator, are faced with incomplete emergency manuals, that don't document the faulty flight computer about to bring the plane down, itself installed to work around the plane's faulty airframe, is a big redflag for structural deficiencies. In that you could as well fix the simulator, fix the manuals, fix the computer, fix the airframe, that there would still be a consistent way for the plane to crash, because it is just so structurally deficient, that no one could honestly put any kind of trust in it. Damn, I love this analogy. Anyway, to come back to the point, I have come to notice that several organizations (very big ones sometimes...) produce code that is very close to structurally deficient, and that's a source of concern for our QA when that code gets imported. In the case of OpenBSD I don't know if it is recent or if it has always been like this, I would tend to think the latter. So yeah big redflag when I see a "from ...", that's an indication that the area needs attention. In all cases, these specific issues with if_umb are not urgent, because the driver is disabled by default in NetBSD. Interesting technical challenge though, if someone is interested! Maxime
Re: CVS commit: src/sys/dev/usb
On 22/03/2020 08:30, Maxime Villard wrote: Overall "From OpenBSD" is a redflag for buggy and vulnerable code.. We should be above this, no software is perfect, not even ours. Roy
Re: CVS commit: src/sys/dev/usb
Le 19/03/2020 à 08:49, Pierre Pronchery a écrit : > Module Name: src > Committed By: khorben > Date: Thu Mar 19 07:49:29 UTC 2020 > > Modified Files: > src/sys/dev/usb: if_umb.c > > Log Message: > When there is no network around the state timeout fires over and over again. > Change the printf into a log and only under IFF_DEBUG to reduce dmesg spam. > Loudly requested by beck@ OK deraadt@ FWIW, there is a number of potentially exploitable bugs in this driver, and they have been in my todo list for three months. Eg, follow umb_decode_response(), there are integer overflows that can trigger actual buffer overflows. Would you be interested in fixing the vulns? > From OpenBSD. Overall "From OpenBSD" is a redflag for buggy and vulnerable code.. Maxime
Re: CVS commit: src/sys/dev/usb
On 18/03/2020 11:33, Robert Elz wrote: > Module Name: src > Committed By: kre > Date: Wed Mar 18 11:33:32 UTC 2020 > > Modified Files: > src/sys/dev/usb: if_aue.c > > Log Message: > This was still not correct,. USB_DEBUG is what mattered, not AUE_DEBUG, > the two are orthogonal. They're not orthogonal... http://src.illumos.org/source/xref/netbsd-src/sys/dev/usb/files.usb#25 25 defflag opt_usb.h AUE_DEBUG: USB_DEBUG Just saying. Nick
Re: CVS commit: src/sys/dev/usb
Date:Tue, 17 Mar 2020 22:58:24 -0400 From:"Christos Zoulas" Message-ID: <20200318025824.93b28f...@cvs.netbsd.org> | Log Message: | define un (pointed out by kre@) The reason I didn't suggest that change, is that now un is unused when USB_DEBUG is not defined. At the very least it would need a __debugused or whatever that #define for the relevant attribute is. But for just a single use, it seemed simpler just to use the value used to init the var that is (now) only used the once. kre
Re: CVS commit: src/sys/dev/usb
In article <20200314143238.gr5...@pony.stderr.spb.ru>, Valery Ushakov wrote: >How is is affected by the decision to change (or not) 0x%x to %#x? > This was in response to the statement: ... with a bit of patience might have been less drastic and as effective. christos
Re: CVS commit: src/sys/dev/usb
On Sat, Mar 14, 2020 at 10:27:27 -0400, Christos Zoulas wrote:
> > I don't belive that "if". It's like claiming you got rid of a stain
> > on a wallpaper after you demolish a wall (not load-bearing,
> > fortunately) and have to put it back and put new wallpaper. :) Get rid
> > of the stain, sure; but may be looking closely with a bit of patience
> > might have been less drastic and as effective.
>
> To fix the kernhist ones I looked with a lot of patience and even then,
> I missed quite a few ones (the ones in the final commit). It is really
> difficult to find them, specially because the DPRINTF macros are
> used sometimes for regular debugging and other times for kernhist.
> In the end I had to add a fake printf function in kernhist.h like below,
> and then filter out the error messages about too many arguments for
> format.
>
> christos
>
> --- kernhist.h 2020-03-13 23:03:13.973939910 -0400
> +++ kernhist.h.orig 2020-03-13 22:59:37.237495925 -0400
> @@ -207,6 +207,11 @@
> #define KERNHIST_PRINTNOW(E) /* nothing */
> #endif
>
> +// Just for format checking
> +static __inline __printflike(1, 2) void
> +__kernhist_printf(const char *fmt __unused, ...) {
> +}
> +
> #define KERNHIST_LOG(NAME,FMT,A,B,C,D) \
> do { \
> unsigned int _i_, _j_; \
> @@ -227,6 +232,7 @@
> _e_->v[1] = (uintmax_t)(B); \
> _e_->v[2] = (uintmax_t)(C); \
> _e_->v[3] = (uintmax_t)(D); \
> + __kernhist_printf(FMT, _e_->v[0], _e_->v[1], _e_->v[2], _e_->v[3]); \
> KERNHIST_PRINTNOW(_e_); \
> } while (/*CONSTCOND*/ 0)
How is is affected by the decision to change (or not) 0x%x to %#x?
-uwe
Re: CVS commit: src/sys/dev/usb
> I don't belive that "if". It's like claiming you got rid of a stain
> on a wallpaper after you demolish a wall (not load-bearing,
> fortunately) and have to put it back and put new wallpaper. :) Get rid
> of the stain, sure; but may be looking closely with a bit of patience
> might have been less drastic and as effective.
To fix the kernhist ones I looked with a lot of patience and even then,
I missed quite a few ones (the ones in the final commit). It is really
difficult to find them, specially because the DPRINTF macros are
used sometimes for regular debugging and other times for kernhist.
In the end I had to add a fake printf function in kernhist.h like below,
and then filter out the error messages about too many arguments for
format.
christos
--- kernhist.h 2020-03-13 23:03:13.973939910 -0400
+++ kernhist.h.orig 2020-03-13 22:59:37.237495925 -0400
@@ -207,6 +207,11 @@
#define KERNHIST_PRINTNOW(E) /* nothing */
#endif
+// Just for format checking
+static __inline __printflike(1, 2) void
+__kernhist_printf(const char *fmt __unused, ...) {
+}
+
#define KERNHIST_LOG(NAME,FMT,A,B,C,D) \
do { \
unsigned int _i_, _j_; \
@@ -227,6 +232,7 @@
_e_->v[1] = (uintmax_t)(B); \
_e_->v[2] = (uintmax_t)(C); \
_e_->v[3] = (uintmax_t)(D); \
+ __kernhist_printf(FMT, _e_->v[0], _e_->v[1], _e_->v[2], _e_->v[3]); \
KERNHIST_PRINTNOW(_e_); \
} while (/*CONSTCOND*/ 0)
signature.asc
Description: Message signed with OpenPGP
Re: CVS commit: src/sys/dev/usb
On Sat, Mar 14, 2020 at 09:57:36 -0400, Christos Zoulas wrote: > > Even for the ones without the widths specified. E.g. I personally > > prefer zero printed as 0x0, not as 0, so I assume that when people > > choose either one that reflects their preference. Why mess with it? > > It's all so unnecessary. > > Yes, now we are discussing cosmetics (if 0 should be printed as 0 or > 0x0 mostly in debugging messages), since this is the only change > remaining. > > In retrospect, perhaps I should have left it alone, It would have been better to just leave them the hell alone as they are to begin with. This is, I think, the third time in recent memory when people try to "fix" 0x%x -> %#x and each time it goes wrong. We should have learned from that. > but now aside the cosmetics part, we are strictly better off because > all the formats have been fixed Which is true but non sequitur. > (including the 2 ones which we would not have found if I did not > make the %# change). I don't belive that "if". It's like claiming you got rid of a stain on a wallpaper after you demolish a wall (not load-bearing, fortunately) and have to put it back and put new wallpaper. :) Get rid of the stain, sure; but may be looking closely with a bit of patience might have been less drastic and as effective. -uwe
Re: CVS commit: src/sys/dev/usb
> Even for the ones without the widths specified. E.g. I personally > prefer zero printed as 0x0, not as 0, so I assume that when people > choose either one that reflects their preference. Why mess with it? > It's all so unnecessary. Yes, now we are discussing cosmetics (if 0 should be printed as 0 or 0x0 mostly in debugging messages), since this is the only change remaining. In retrospect, perhaps I should have left it alone, but now aside the cosmetics part, we are strictly better off because all the formats have been fixed (including the 2 ones which we would not have found if I did not make the %# change). christos signature.asc Description: Message signed with OpenPGP
re: CVS commit: src/sys/dev/usb
> As I wrote in a follow up email, it changes formatting b/c you didn't > change field widths and IMO using %# with a field width is mostly > trouble to begin with. It's not the first time someone tries to do > this without actually understanding the consequences of the change. > Please, can we assume that when people write either 0x%x or %#x they > most likely actaully mean it for whatever reason and that they want > that specific output format, and it's just rude to change that, > especially when you do so incorrectly. i've come to agree that %# is dangerous in general to save one character. not only does it have the width issue you've mentioned, but it also emits "0" instead of "0x0" for the zero case, which i find surprising. christos, thanks for the backout. .mrg.
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 22:26:05 -0400, Christos Zoulas wrote: > > On Mar 13, 2020, at 10:25 PM, Valery Ushakov wrote: > > > > As I wrote in a follow up email, it changes formatting b/c you didn't > > change field widths and IMO using %# with a field width is mostly > > trouble to begin with. It's not the first time someone tries to do > > this without actually understanding the consequences of the change. > > Please, can we assume that when people write either 0x%x or %#x they > > most likely actaully mean it for whatever reason and that they want > > that specific output format, and it's just rude to change that, > > especially when you do so incorrectly. > > I am reverting the fixed width ones, hold on. Even for the ones without the widths specified. E.g. I personally prefer zero printed as 0x0, not as 0, so I assume that when people choose either one that reflects their preference. Why mess with it? It's all so unnecessary. -uwe
Re: CVS commit: src/sys/dev/usb
> On Mar 13, 2020, at 10:25 PM, Valery Ushakov wrote: > > As I wrote in a follow up email, it changes formatting b/c you didn't > change field widths and IMO using %# with a field width is mostly > trouble to begin with. It's not the first time someone tries to do > this without actually understanding the consequences of the change. > Please, can we assume that when people write either 0x%x or %#x they > most likely actaully mean it for whatever reason and that they want > that specific output format, and it's just rude to change that, > especially when you do so incorrectly. I am reverting the fixed width ones, hold on. christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 22:15:31 -0400, Christos Zoulas wrote: > > This was not a part of the PR and is completely cosmetic (surely it > > supports plain %x if it does support %#x). Why was this necessary? > > (I know I would be quite miffed if someone made a change like that to > > my code). > > Yes, that %x formatting change was not part of the PR, but I only > changed 0x%x not plain %x. I did it because as I was fixing the > 0x%x in the log, I started changing them to %#jx so I did it > globally in that directory for consistency. It found two formats > that were 0x%hu... > > So one can view it as a format consistency checker(not just cosmetic). As I wrote in a follow up email, it changes formatting b/c you didn't change field widths and IMO using %# with a field width is mostly trouble to begin with. It's not the first time someone tries to do this without actually understanding the consequences of the change. Please, can we assume that when people write either 0x%x or %#x they most likely actaully mean it for whatever reason and that they want that specific output format, and it's just rude to change that, especially when you do so incorrectly. -uwe
Re: CVS commit: src/sys/dev/usb
> This was not a part of the PR and is completely cosmetic (surely it > supports plain %x if it does support %#x). Why was this necessary? > (I know I would be quite miffed if someone made a change like that to > my code). Yes, that %x formatting change was not part of the PR, but I only changed 0x%x not plain %x. I did it because as I was fixing the 0x%x in the log, I started changing them to %#jx so I did it globally in that directory for consistency. It found two formats that were 0x%hu... So one can view it as a format consistency checker(not just cosmetic). christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 17:09:14 -0700, Paul Goyette wrote:
> On Sat, 14 Mar 2020, Valery Ushakov wrote:
>
> > On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote:
> >
> > > Log Message:
> > > PR/55068: sc.dying: Fix printf formats:
> > [...]
> > > - 0x% -> %#
> >
> > This was not a part of the PR and is completely cosmetic (surely it
> > supports plain %x if it does support %#x). Why was this necessary?
> > (I know I would be quite miffed if someone made a change like that to
> > my code).
>
> Plain %x - no :(
>
> In order to enable sysctl-transport to userland, all the args need to
> be promoted to %jx, and the format strings need to ensure that they
> consume that size.
Random sample from the diff:
- printf("%s: expect 0xe6!! (0x%x)\n", device_xname(sc->sc_dev),
+ printf("%s: expect 0xe6!! (%#x)\n", device_xname(sc->sc_dev),
Actually, looking close I see diffs like
- DPRINTFN(MD_ROOT, "wValue=0x%04jx", value, 0, 0, 0);
+ DPRINTFN(MD_ROOT, "wValue=%#04jx", value, 0, 0, 0);
that are plain wrong as %#x counts the 0x prefix towards the field
width.
$ printf '0x%02x %#02x\n' 1 1
0x01 0x1
$ printf '0x%08x 0x%08x\n%#08x %#08x\n' 0 1 0 1
0x 0x0001
0x01
So, as far as I can tell, these %# changes don't fix all the argument
size issues, break some output formatting and violate preference of
the original author. Did I miss something else?
-uwe
