Re: don't try and wakeup select/poll/kq in tun/bpf close

2022-01-15 Thread David Gwynne
On Sun, Jan 16, 2022 at 06:17:16AM +, Visa Hankala wrote:
> On Sun, Jan 16, 2022 at 02:41:46PM +1000, David Gwynne wrote:
> > if you're in bpfclose or tun/tap close, you're the last one out. this
> > means that there shouldn't be anything else in poll/select/kevent/etc
> > because you're the last one out.
> > 
> > from what i can tell, tun and bpf are the only drivers that do this, and
> > i dont think they need to.
> 
> The last one out holds when the device closing is triggered by the
> releasing of the last file reference. However, bpf/tun/tap close can
> also be called when the device is detached through VOP_REVOKE(). In
> that case any associated file descriptors remain open.

iirc, revoke can call fooclose() before it swaps the vnode ops out for
the dead ones. so yes, the fd is open but it's state becomes divorced
from the special device it was originally associated with.

> I think poll/select/kevent should wake up if the device is detached.
> kevent(2) and kqueue-based select(2) get notified as a result of
> klist_invalidate().
> 
> There is also the SIGIO case, but only bpf close raises the signal.
> 
> To be on the safer side, I would wait until poll(2) uses kqueue backed.

grumble grumble. yes.

or the detach code that calls revoke and klist_invalidate should also do
selwakeup...



Re: don't try and wakeup select/poll/kq in tun/bpf close

2022-01-15 Thread Visa Hankala
On Sun, Jan 16, 2022 at 02:41:46PM +1000, David Gwynne wrote:
> if you're in bpfclose or tun/tap close, you're the last one out. this
> means that there shouldn't be anything else in poll/select/kevent/etc
> because you're the last one out.
> 
> from what i can tell, tun and bpf are the only drivers that do this, and
> i dont think they need to.

The last one out holds when the device closing is triggered by the
releasing of the last file reference. However, bpf/tun/tap close can
also be called when the device is detached through VOP_REVOKE(). In
that case any associated file descriptors remain open.

I think poll/select/kevent should wake up if the device is detached.
kevent(2) and kqueue-based select(2) get notified as a result of
klist_invalidate().

There is also the SIGIO case, but only bpf close raises the signal.

To be on the safer side, I would wait until poll(2) uses kqueue backed.



C API Suggestion: Get Hard Link Path and Filename From File Descriptor

2022-01-15 Thread Samuel Venable
Hello, OpenBSD Developers!

Here's my code which implements this new feature, however it doesn't always 
work as intended:
- 
https://github.com/time-killer-games/bugs/blob/679d333cd947cb4cbec7c45485157b305aa0757b/apifilesystem/filesystem.cpp#L55-L62
- 
https://github.com/time-killer-games/bugs/blob/679d333cd947cb4cbec7c45485157b305aa0757b/apifilesystem/filesystem.cpp#L74-L192
- 
https://github.com/time-killer-games/bugs/blob/679d333cd947cb4cbec7c45485157b305aa0757b/apifilesystem/filesystem.cpp#L479-L492
- 
https://github.com/time-killer-games/bugs/blob/679d333cd947cb4cbec7c45485157b305aa0757b/test.cpp#L1-L15

You can run a quick test by cloning the GitHub repository and then running this 
shell script:
https://github.com/time-killer-games/bugs/blob/679d333cd947cb4cbec7c45485157b305aa0757b/test.sh

The code breaks the while loop when the first hard link matches the current 
inode associated with the file descriptor, meaning it won't return all hard 
links, but it can easily be modified to not break and return a vector of every 
hard link found, although the search will take much longer and it already has 
pretty bad performance, but it still is good enough to get the job done and the 
performance can be improved using multiple threads instead of just one, also by 
adding deeper filesystem search paths to the glob. This was written in C++ 
because I am used to that language more, but I'll gladly write a version which 
is in C to conform to a better standard worth having in the kernel, base, or 
ports as a part of a feature rich CLI + development library depending on where 
this belongs more. 

As for how it doesn't work 'as intended', for some reason if i don't delete the 
previous file i opened and closed before creating a new temp file, the filename 
returned sometimes is a duplicate of a filename associated with a former file 
descriptor i had opened. Also, before i can even get this working in the 
example I have on GitHub. There is a folder under my "/tmp" directory called 
"vi.recover", (the absolute path is "/tmp/vi.recover"), which oddly enough that 
folder is printed by the example application if that folder exists. It has 
permissions that prevent me from deleting it unless I am root. So I run 'su -l 
root -c "rm -fr /tmp/vi.recover"' to delete it and then sure enough after that 
file is gone only then does the example app ever return the correct file 
associated with the inode and file descriptor, even potentially.

The only reason I made it return one hard link and not all matches is because 
of performance and because this API i originally intended to be cross-platform 
and macOS's "fnctl(fd, F_GETPATH, buffer)" API only offers returning one hard 
link from the specified file descriptor, regardless of how many there may be, 
and it seems to choose that at random on macOS, so adopted that behavior to 
remain cross-platform.

Please let me know what you think and I don't check my email often, please add 
me on Discord if you like, or open an issue/pull request on my GitHub 
repository with your suggestions:

My Discord: "Samuel Venable#5465"
Open an Issue on GitHub: https://github.com/time-killer-games/bugs/issues
Submit a pull request on GitHub: https://github.com/time-killer-games/bugs/pulls

...or reply directly to this email with your thoughts and I hope I hear back 
from you in any one of these ways.

Thank You!
Samuel Venable



don't try and wakeup select/poll/kq in tun/bpf close

2022-01-15 Thread David Gwynne
if you're in bpfclose or tun/tap close, you're the last one out. this
means that there shouldn't be anything else in poll/select/kevent/etc
because you're the last one out.

from what i can tell, tun and bpf are the only drivers that do this, and
i dont think they need to.

ok?

Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.209
diff -u -p -r1.209 bpf.c
--- bpf.c   13 Jan 2022 14:15:27 -  1.209
+++ bpf.c   16 Jan 2022 04:33:02 -
@@ -401,7 +401,6 @@ bpfclose(dev_t dev, int flag, int mode, 
d = bpfilter_lookup(minor(dev));
mtx_enter(>bd_mtx);
bpf_detachd(d);
-   bpf_wakeup(d);
LIST_REMOVE(d, bd_list);
mtx_leave(>bd_mtx);
bpf_put(d);
Index: if_tun.c
===
RCS file: /cvs/src/sys/net/if_tun.c,v
retrieving revision 1.231
diff -u -p -r1.231 if_tun.c
--- if_tun.c9 Mar 2021 20:05:14 -   1.231
+++ if_tun.c16 Jan 2022 04:33:02 -
@@ -460,7 +460,6 @@ tun_dev_close(dev_t dev, struct proc *p)
ifq_purge(>if_snd);
 
CLR(sc->sc_flags, TUN_ASYNC);
-   selwakeup(>sc_rsel);
sigio_free(>sc_sigio);
 
if (!ISSET(sc->sc_flags, TUN_DEAD)) {



Re: sbin/isakmpd: fix -Wunused-but-set-variable warnings

2022-01-15 Thread Philip Guenther
On Sat, Jan 15, 2022 at 12:36 PM Christian Weisgerber 
wrote:

> sbin/isakmpd: fix -Wunused-but-set-variable warnings
>
> The one in pf_key_v2.c could use an extra set of eyes, but I don't
> think there are any side effects.
>

The involved variables in pf_key_v2.c were added in 2000 as part of some
sort of sync-with-upstream, weren't used then, and never used after.
Deleting them seems perfectly fine to me.

ok guenther@ on the entire diff


Re: Silence vmd rtc_update_rega non-32KHz timebase spam

2022-01-15 Thread Mike Larkin
On Wed, Dec 08, 2021 at 07:45:50PM -0600, Brian Conway wrote:
> Ping with complete diff. Thanks.
>
> Brian Conway
>

Catching up on old emails. Committed. Thanks.

-ml

> diff --git usr.sbin/vmd/mc146818.c usr.sbin/vmd/mc146818.c
> index e3599c685..001c1a055 100644
> --- usr.sbin/vmd/mc146818.c
> +++ usr.sbin/vmd/mc146818.c
> @@ -34,7 +34,6 @@
>  #include "vmd.h"
>  #include "vmm.h"
>
> -#define MC_DIVIDER_MASK 0xe0
>  #define MC_RATE_MASK 0xf
>
>  #define NVRAM_CENTURY 0x32
> @@ -236,10 +235,6 @@ rtc_reschedule_per(void)
>  static void
>  rtc_update_rega(uint32_t data)
>  {
> -if ((data & MC_DIVIDER_MASK) != MC_BASE_32_KHz)
> -log_warnx("%s: set non-32KHz timebase not supported",
> -__func__);
> -
>  rtc.regs[MC_REGA] = data;
>  if ((rtc.regs[MC_REGA] ^ data) & 0x0f)
>  vm_pipe_send(_pipe, MC146818_RESCHEDULE_PER);
>
>
> On Thu, Nov 18, 2021 at 8:02 AM Brian Conway  wrote:
> >
> > Per https://marc.info/?l=openbsd-misc=159113575425726 , mlarkin@
> > suggested someone can remove it. It's still pretty spammy at the
> > current time for me.
> >
> > Brian Conway
> > Software Engineer, Owner
> > RCE Software, LLC
> >
> > diff --git usr.sbin/vmd/mc146818.c usr.sbin/vmd/mc146818.c
> > index e3599c68504..17cf21221e5 100644
> > --- usr.sbin/vmd/mc146818.c
> > +++ usr.sbin/vmd/mc146818.c
> > @@ -236,10 +236,6 @@ rtc_reschedule_per(void)
> >  static void
> >  rtc_update_rega(uint32_t data)
> >  {
> > -if ((data & MC_DIVIDER_MASK) != MC_BASE_32_KHz)
> > -log_warnx("%s: set non-32KHz timebase not supported",
> > -__func__);
> > -
> >  rtc.regs[MC_REGA] = data;
> >  if ((rtc.regs[MC_REGA] ^ data) & 0x0f)
> >  vm_pipe_send(_pipe, MC146818_RESCHEDULE_PER);
>



sbin/isakmpd: fix -Wunused-but-set-variable warnings

2022-01-15 Thread Christian Weisgerber
sbin/isakmpd: fix -Wunused-but-set-variable warnings

The one in pf_key_v2.c could use an extra set of eyes, but I don't
think there are any side effects.
 
M  sbin/isakmpd/ipsec.c
M  sbin/isakmpd/pf_key_v2.c
M  sbin/isakmpd/udp_encap.c
M  sbin/isakmpd/x509.c

diff ce1a8a9dca08dd7e01f71dfff05f1e4f4ed3bb7e 
7c5dd09ecd1ff078b868c9ab52aac9754cde7761
blob - 3954c5670aec76c08146272f2ab6e9038aa79c82
blob + 272b3657e8bdcf303f046d2641d11ad67f912fc2
--- sbin/isakmpd/ipsec.c
+++ sbin/isakmpd/ipsec.c
@@ -2090,7 +2090,6 @@ ipsec_decode_id(char *buf, size_t size, u_int8_t *id, 
 {
int id_type;
char   *addr = 0, *mask = 0;
-   u_int32_t  *idp;
 
if (id) {
if (!isakmpform) {
@@ -2102,7 +2101,6 @@ ipsec_decode_id(char *buf, size_t size, u_int8_t *id, 
id_len += ISAKMP_GEN_SZ;
}
id_type = GET_ISAKMP_ID_TYPE(id);
-   idp = (u_int32_t *) (id + ISAKMP_ID_DATA_OFF);
switch (id_type) {
case IPSEC_ID_IPV4_ADDR:
util_ntoa(, AF_INET, id + ISAKMP_ID_DATA_OFF);
blob - 04e867dedaf62dba9e3b1fc6702528432e53f243
blob + 302bcb52ee20cef9abb7ccc4fd052fcbd6486ea9
--- sbin/isakmpd/pf_key_v2.c
+++ sbin/isakmpd/pf_key_v2.c
@@ -2310,8 +2310,6 @@ pf_key_v2_acquire(struct pf_key_v2_msg *pmsg)
struct sadb_x_policy policy;
struct sadb_address *dst = 0, *src = 0;
struct sockaddr *dstaddr, *srcaddr = 0;
-   struct sadb_comb *scmb = 0;
-   struct sadb_prop *sprp = 0;
struct sadb_ident *srcident = 0, *dstident = 0;
chardstbuf[ADDRESS_MAX], srcbuf[ADDRESS_MAX], *peer = 0;
charconfname[120], *conn = 0;
@@ -2354,11 +2352,6 @@ pf_key_v2_acquire(struct pf_key_v2_msg *pmsg)
if (ext)
src = ext->seg;
 
-   ext = pf_key_v2_find_ext(pmsg, SADB_EXT_PROPOSAL);
-   if (ext) {
-   sprp = ext->seg;
-   scmb = (struct sadb_comb *) (sprp + 1);
-   }
ext = pf_key_v2_find_ext(pmsg, SADB_EXT_IDENTITY_SRC);
if (ext)
srcident = ext->seg;
blob - ae20f98fadadebfd1467ab99ba6527d3a2c236b6
blob + 1eef9e00b5c26ae4222a6c3f95b7d47ccf9d2bb8
--- sbin/isakmpd/udp_encap.c
+++ sbin/isakmpd/udp_encap.c
@@ -227,7 +227,7 @@ udp_encap_create(char *name)
 {
struct virtual_transport *v;
struct udp_transport*u;
-   struct transport*rv, *t;
+   struct transport*rv;
struct sockaddr *dst, *addr;
struct conf_list*addr_list = 0;
struct conf_list_node   *addr_node;
@@ -303,7 +303,6 @@ udp_encap_create(char *name)
rv = 0;
goto ret;
}
-   t = (struct transport *)v;
rv = udp_clone(v->encap, dst);
if (rv)
rv->vtbl = _encap_transport_vtbl;
blob - 4ccaf0728756f61db4dfb57d59d718b92e446f71
blob + a4cc6d7ca7325cfb71977a627e779d8d42e07396
--- sbin/isakmpd/x509.c
+++ sbin/isakmpd/x509.c
@@ -680,7 +680,7 @@ x509_read_crls_from_dir(X509_STORE *ctx, char *name)
struct stat sb;
charfullname[PATH_MAX];
charfile[PATH_MAX];
-   int fd, off, size;
+   int fd;
 
if (strlen(name) >= sizeof fullname - 1) {
log_print("x509_read_crls_from_dir: directory name too long");
@@ -695,8 +695,6 @@ x509_read_crls_from_dir(X509_STORE *ctx, char *name)
return 0;
}
strlcpy(fullname, name, sizeof fullname);
-   off = strlen(fullname);
-   size = sizeof fullname - off;
 
while ((fd = monitor_readdir(file, sizeof file)) != -1) {
LOG_DBG((LOG_CRYPTO, 60, "x509_read_crls_from_dir: reading "

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



sbin/pfctl: fix -Wunused-but-set-variable warning

2022-01-15 Thread Christian Weisgerber
sbin/pfctl: fix -Wunused-but-set-variable warning
 
M  sbin/pfctl/pfctl_optimize.c

diff 7c5dd09ecd1ff078b868c9ab52aac9754cde7761 
6e5c342a53c05496c18849837c67b7dc05ce3792
blob - 1ab170a832dd183a2895774549ff93896803039a
blob + 5736a0d7b0ba04afeed855daa61fc6b5ef3894e4
--- sbin/pfctl/pfctl_optimize.c
+++ sbin/pfctl/pfctl_optimize.c
@@ -789,7 +789,6 @@ block_feedback(struct pfctl *pf, struct superblock *bl
 {
TAILQ_HEAD( , pf_opt_rule) queue;
struct pf_opt_rule *por1, *por2;
-   u_int64_t total_count = 0;
struct pf_rule a, b;
 
 
@@ -799,8 +798,6 @@ block_feedback(struct pfctl *pf, struct superblock *bl
 */
TAILQ_FOREACH(por1, >sb_profiled_block->sb_rules, por_entry) {
comparable_rule(, >por_rule, DC);
-   total_count += por1->por_rule.packets[0] +
-   por1->por_rule.packets[1];
TAILQ_FOREACH(por2, >sb_rules, por_entry) {
if (por2->por_profile_count)
continue;

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



lib/libfuse: fix -Wunused-but-set-variable warning

2022-01-15 Thread Christian Weisgerber
Since the switch to LLVM 13, there are a number of compiler warnings
in base about variables that are assigned to but never used.  Let's
start picking the low-hanging fruit, ok?

lib/libfuse: fix -Wunused-but-set-variable warning
 
M  lib/libfuse/fuse_opt.c

diff 926818cffbbacfeb5685fa0f8e104986608d1a29 
ce1a8a9dca08dd7e01f71dfff05f1e4f4ed3bb7e
blob - 38bf34a7d157a543ee47f18bf350cb9183ab9803
blob + 26dcecd3e2486ad1f833bfee0827a2bd5406e068
--- lib/libfuse/fuse_opt.c
+++ lib/libfuse/fuse_opt.c
@@ -190,10 +190,9 @@ parse_opt(const struct fuse_opt *o, const char *opt, v
 fuse_opt_proc_t f, struct fuse_args *arg)
 {
const char *val;
-   int keyval, ret, found;
+   int ret, found;
size_t sep;
 
-   keyval = 0;
found = 0;
 
for(; o != NULL && o->templ; o++) {
@@ -205,11 +204,9 @@ parse_opt(const struct fuse_opt *o, const char *opt, v
val = opt;
 
/* check key=value or -p n */
-   if (o->templ[sep] == '=') {
-   keyval = 1;
+   if (o->templ[sep] == '=')
val = [sep + 1];
-   } else if (o->templ[sep] == ' ') {
-   keyval = 1;
+   else if (o->templ[sep] == ' ') {
if (sep == strlen(opt)) {
/* ask for next arg to be included */
return (IFUSE_OPT_NEED_ANOTHER_ARG);

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Visa Hankala
On Sat, Jan 15, 2022 at 01:34:35PM +0100, Mark Kettenis wrote:
> > Date: Sat, 15 Jan 2022 12:21:59 +
> > From: Visa Hankala 
> > 
> > If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> > cache-line-aligned, the function spins because len wraps around.
> > The following patch fixes this.
> > 
> > There still is a subtle detail. If len is zero but pa is non-aligned,
> > the function is not a no-op. However, shouldn't upper layers handle
> > things so that zero length does not reach this deep?
> 
> Probably.  But one could also argue that even when the length is zero,
> this should be a barrier.  And the barrier doesn't really hurt.  So I
> don't think we need to worry about this.
> 
> I noticed that len has the type paddr_t.  That is clealy wrong and
> probably should be psize_t.  Can you fix that as well?

Sure, I will fix it.

It occurred to me that my patch handles incorrectly the upper end of
the paddr_t range. At least in theory, pa + len may wrap to zero.
I have adjusted the code in an attempt to fix this. Now `end' is
rounded up and equality comparison is used to cover wrapping.

OK?

Index: arch/riscv64/dev/sfcc.c
===
RCS file: src/sys/arch/riscv64/dev/sfcc.c,v
retrieving revision 1.2
diff -u -p -r1.2 sfcc.c
--- arch/riscv64/dev/sfcc.c 22 May 2021 17:07:28 -  1.2
+++ arch/riscv64/dev/sfcc.c 15 Jan 2022 17:38:03 -
@@ -93,18 +93,19 @@ sfcc_attach(struct device *parent, struc
 }
 
 void
-sfcc_cache_wbinv_range(paddr_t pa, paddr_t len)
+sfcc_cache_wbinv_range(paddr_t pa, psize_t len)
 {
struct sfcc_softc *sc = sfcc_sc;
+   paddr_t end, mask;
 
-   len += pa & (sc->sc_line_size - 1);
-   pa &= ~((paddr_t)sc->sc_line_size - 1);
+   mask = sc->sc_line_size - 1;
+   end = (pa + len + mask) & ~mask;
+   pa &= ~mask;
 
__asm volatile ("fence iorw,iorw" ::: "memory");
-   while (len > 0) {
+   while (pa != end) {
bus_space_write_8(sc->sc_iot, sc->sc_ioh, SFCC_FLUSH64, pa);
__asm volatile ("fence iorw,iorw" ::: "memory");
pa += sc->sc_line_size;
-   len -= sc->sc_line_size;
}
 }



Re: Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Visa Hankala
On Sat, Jan 15, 2022 at 11:54:00PM +1100, Jonathan Gray wrote:
> On Sat, Jan 15, 2022 at 12:21:59PM +, Visa Hankala wrote:
> > If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> > cache-line-aligned, the function spins because len wraps around.
> > The following patch fixes this.
> > 
> > There still is a subtle detail. If len is zero but pa is non-aligned,
> > the function is not a no-op. However, shouldn't upper layers handle
> > things so that zero length does not reach this deep?
> > 
> > OK?
> 
> This driver was added to workaround the lack of coherency on
> beaglev and is unused on the unmatched (sifive,fu740-c000-ccache).
> 
> The polarfire device tree has sifive,fu540-c000-ccache this
> driver matches on but does it actually need this driver?

The Technical Reference Manual indicates that the data caches and main
interconnects are coherent. However, I am not sure about that yet.



Re: Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Mark Kettenis
> Date: Sat, 15 Jan 2022 23:54:00 +1100
> From: Jonathan Gray 
> 
> On Sat, Jan 15, 2022 at 12:21:59PM +, Visa Hankala wrote:
> > If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> > cache-line-aligned, the function spins because len wraps around.
> > The following patch fixes this.
> > 
> > There still is a subtle detail. If len is zero but pa is non-aligned,
> > the function is not a no-op. However, shouldn't upper layers handle
> > things so that zero length does not reach this deep?
> > 
> > OK?
> 
> This driver was added to workaround the lack of coherency on
> beaglev and is unused on the unmatched (sifive,fu740-c000-ccache).
> 
> The polarfire device tree has sifive,fu540-c000-ccache this
> driver matches on but does it actually need this driver?

The jury is still out on whether we need this driver on other SoCs
based on SiFive cores.  So I think we should fix the bugs regardless.



Re: Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Jonathan Gray
On Sat, Jan 15, 2022 at 12:21:59PM +, Visa Hankala wrote:
> If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> cache-line-aligned, the function spins because len wraps around.
> The following patch fixes this.
> 
> There still is a subtle detail. If len is zero but pa is non-aligned,
> the function is not a no-op. However, shouldn't upper layers handle
> things so that zero length does not reach this deep?
> 
> OK?

This driver was added to workaround the lack of coherency on
beaglev and is unused on the unmatched (sifive,fu740-c000-ccache).

The polarfire device tree has sifive,fu540-c000-ccache this
driver matches on but does it actually need this driver?



Re: Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Mark Kettenis
> Date: Sat, 15 Jan 2022 12:21:59 +
> From: Visa Hankala 
> 
> If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> cache-line-aligned, the function spins because len wraps around.
> The following patch fixes this.
> 
> There still is a subtle detail. If len is zero but pa is non-aligned,
> the function is not a no-op. However, shouldn't upper layers handle
> things so that zero length does not reach this deep?

Probably.  But one could also argue that even when the length is zero,
this should be a barrier.  And the barrier doesn't really hurt.  So I
don't think we need to worry about this.

I noticed that len has the type paddr_t.  That is clealy wrong and
probably should be psize_t.  Can you fix that as well?

> OK?

ok kettenis@


> Index: arch/riscv64/dev/sfcc.c
> ===
> RCS file: src/sys/arch/riscv64/dev/sfcc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 sfcc.c
> --- arch/riscv64/dev/sfcc.c   22 May 2021 17:07:28 -  1.2
> +++ arch/riscv64/dev/sfcc.c   15 Jan 2022 11:53:12 -
> @@ -96,15 +96,15 @@ void
>  sfcc_cache_wbinv_range(paddr_t pa, paddr_t len)
>  {
>   struct sfcc_softc *sc = sfcc_sc;
> + paddr_t end;
>  
> - len += pa & (sc->sc_line_size - 1);
> + end = pa + len;
>   pa &= ~((paddr_t)sc->sc_line_size - 1);
>  
>   __asm volatile ("fence iorw,iorw" ::: "memory");
> - while (len > 0) {
> + while (pa < end) {
>   bus_space_write_8(sc->sc_iot, sc->sc_ioh, SFCC_FLUSH64, pa);
>   __asm volatile ("fence iorw,iorw" ::: "memory");
>   pa += sc->sc_line_size;
> - len -= sc->sc_line_size;
>   }
>  }
> 
> 



Fix a bug in sfcc_cache_wbinv_range()

2022-01-15 Thread Visa Hankala
If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
cache-line-aligned, the function spins because len wraps around.
The following patch fixes this.

There still is a subtle detail. If len is zero but pa is non-aligned,
the function is not a no-op. However, shouldn't upper layers handle
things so that zero length does not reach this deep?

OK?

Index: arch/riscv64/dev/sfcc.c
===
RCS file: src/sys/arch/riscv64/dev/sfcc.c,v
retrieving revision 1.2
diff -u -p -r1.2 sfcc.c
--- arch/riscv64/dev/sfcc.c 22 May 2021 17:07:28 -  1.2
+++ arch/riscv64/dev/sfcc.c 15 Jan 2022 11:53:12 -
@@ -96,15 +96,15 @@ void
 sfcc_cache_wbinv_range(paddr_t pa, paddr_t len)
 {
struct sfcc_softc *sc = sfcc_sc;
+   paddr_t end;
 
-   len += pa & (sc->sc_line_size - 1);
+   end = pa + len;
pa &= ~((paddr_t)sc->sc_line_size - 1);
 
__asm volatile ("fence iorw,iorw" ::: "memory");
-   while (len > 0) {
+   while (pa < end) {
bus_space_write_8(sc->sc_iot, sc->sc_ioh, SFCC_FLUSH64, pa);
__asm volatile ("fence iorw,iorw" ::: "memory");
pa += sc->sc_line_size;
-   len -= sc->sc_line_size;
}
 }