Re: CVS commit: src/sys/dev/pci

2021-02-01 Thread J. Hannken-Illjes
> 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

2021-01-26 Thread Reinoud Zandijk
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

2021-01-26 Thread Rin Okuyama

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

2021-01-25 Thread Jason Thorpe


> 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

2021-01-24 Thread Martin Husemann
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

2021-01-24 Thread Jason Thorpe


> 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

2021-01-24 Thread Martin Husemann
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

2021-01-23 Thread Jason Thorpe


> 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

2021-01-23 Thread Christos Zoulas
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

2021-01-23 Thread matthew green
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

2021-01-23 Thread Christos Zoulas
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

2021-01-23 Thread Taylor R Campbell
> 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

2021-01-23 Thread Christos Zoulas
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

2021-01-23 Thread matthew green
"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

2021-01-23 Thread Christos Zoulas
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

2021-01-23 Thread Christos Zoulas
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

2021-01-22 Thread Reinoud Zandijk
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

2021-01-22 Thread Nick Hudson

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

2021-01-21 Thread Christos Zoulas
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

2021-01-21 Thread matthew green
> +#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

2021-01-21 Thread Christos Zoulas
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

2021-01-21 Thread Jason Thorpe


> 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

2021-01-21 Thread Martin Husemann
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

2021-01-17 Thread Jared McNeill
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

2021-01-15 Thread matthew green
"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

2020-12-10 Thread SAITOH Masanobu

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

2020-10-21 Thread Rin Okuyama

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

2020-10-21 Thread Michael
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

2020-10-18 Thread Rin Okuyama

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

2020-10-18 Thread Jared McNeill
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

2020-10-16 Thread SAITOH Masanobu

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

2020-10-12 Thread Michael
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

2020-10-02 Thread Rin Okuyama

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

2020-10-02 Thread Jonathan A. Kollasch
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

2020-09-19 Thread Kimmo Suominen
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)

2020-08-27 Thread Frank Kardel

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)

2020-08-27 Thread Martin Husemann
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

2020-08-03 Thread Valery Ushakov
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

2020-08-03 Thread SAITOH Masanobu

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

2020-08-03 Thread Valery Ushakov
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

2020-08-03 Thread SAITOH Masanobu

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

2020-07-28 Thread matthew green
"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

2020-07-17 Thread Jaromír Doleček
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

2020-07-17 Thread Jason Thorpe



> 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

2020-07-17 Thread matthew green
"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

2020-07-12 Thread Kimmo Suominen
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

2020-07-11 Thread Kimmo Suominen
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

2020-07-11 Thread Robert Elz
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

2020-07-11 Thread Martin Husemann
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

2020-07-11 Thread Kimmo Suominen
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

2020-07-11 Thread Martin Husemann
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

2020-07-11 Thread Kimmo Suominen
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

2020-07-11 Thread Jukka Ruohonen
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

2020-06-27 Thread matthew green
"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

2020-06-25 Thread Christos Zoulas
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

2020-06-24 Thread matthew green
"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

2020-06-23 Thread Jukka Ruohonen
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

2020-06-07 Thread Nick Hudson

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

2020-05-31 Thread Maxime Villard

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

2020-05-31 Thread Shoichi Yamaguchi
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

2020-05-30 Thread Alexander Nasonov
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

2020-05-30 Thread nia
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

2020-05-30 Thread Tetsuya Isaki
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

2020-05-29 Thread nia
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

2020-05-28 Thread Tetsuya Isaki
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

2020-05-27 Thread Paul Goyette

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

2020-05-27 Thread Taylor R Campbell
> 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

2020-05-27 Thread nia
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

2020-05-27 Thread Tetsuya Isaki
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

2020-05-27 Thread Paul Goyette

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

2020-05-27 Thread Shoichi Yamaguchi
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

2020-05-26 Thread Maxime Villard

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

2020-05-20 Thread Sevan Janiyan



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

2020-05-02 Thread David Brownlee
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

2020-05-02 Thread Jason Thorpe


> 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

2020-05-01 Thread Ryo ONODERA
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

2020-05-01 Thread David Brownlee
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

2020-04-29 Thread Alexander Nasonov
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

2020-04-29 Thread David Brownlee
Just another data point - seeing this same panic on a T480 with the
latest kernel from nyftp


Re: CVS commit: src/sys/dev/ata

2020-04-27 Thread Ryo ONODERA
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

2020-04-27 Thread Ryo ONODERA
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

2020-04-13 Thread Maxime Villard
> 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

2020-04-02 Thread sc dying
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

2020-03-23 Thread SAITOH Masanobu
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

2020-03-23 Thread Maxime Villard
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

2020-03-22 Thread Roy Marples

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

2020-03-22 Thread Maxime Villard
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

2020-03-18 Thread Nick Hudson
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

2020-03-17 Thread Robert Elz
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

2020-03-14 Thread Christos Zoulas
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

2020-03-14 Thread Valery Ushakov
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

2020-03-14 Thread Christos Zoulas

> 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

2020-03-14 Thread Valery Ushakov
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

2020-03-14 Thread Christos Zoulas

> 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

2020-03-14 Thread matthew green
> 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

2020-03-13 Thread Valery Ushakov
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

2020-03-13 Thread Christos Zoulas


> 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

2020-03-13 Thread Valery Ushakov
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

2020-03-13 Thread Christos Zoulas

> 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

2020-03-13 Thread Valery Ushakov
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


  1   2   3   4   5   6   7   8   9   10   >