Re: is anyone maintaining hifn(4)?

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Sun, 18 Jul 2021 01:33:50 -0700
> From: emily ingalls <emily@ingalls.rocks>
> 
> as far as my use case goes, i have quite a lot of old and slow
> hardware, which is one of the reasons i use and like netbsd, since
> it works on all of it. i like to test modern software with this
> hardware under netbsd, and fix issues when they crop up. much of my
> hardware is usable for many things, but consistently are slowed to a
> crawl by cryptographic operations.

Sounds reasonable.

> also, since soekris was the primary source of these cards that i am
> aware of, and they are no longer manufacturing them (and the
> remainder of their stock is presumably dwindling), i have been
> considering sourcing some hifn chips and producing new cards of my
> own. with an open source board design, for people with similar
> needs.

Cool!

> > What do you mean by `currently disabled'?  It looks like hifn(4) is
> > included in amd64/GENERIC and i386/GENERIC, at least.
> 
> i saw a commit from 2020 that mentioned disabling hifn. i haven't
> tested this or tried to build hifn on current, so if this is wrong
> then just ignore me.

If you're referring to

https://mail-index.netbsd.org/source-changes/2020/02/29/msg114571.html

that just disabled building the hifn(4) loadable module; it is still
built into the x86 GENERIC kernels by default.

> > If you have any patches, I'd be happy to take a look.  (Can't test on
> > my device right now but I'll eventually have access to it again.)
> 
> sure. a small patch follows, which fixes three things: [...]

This all sounds reasonable to me -- I don't understand the clock
business well enough to say, but if this is what the datasheet
recommends then it sounds good.  Some comments on the patch:

1. Can you provide a link to the datasheet so the next person to come
   along can easily verify it or find similar mistakes?

2. kpause is not allowed while holding a spin lock or in interrupt
   context.  This doesn't happen during hifn_attach, but it can happen
   in hifn_detach and hifn_intr, via hifn_abort ->
   hifn_init_pci_registers.

   We could use DELAY instead of kpause here, but while it would
   technically work it is an abuse of DELAY that we should avoid (like
   the annoying long pauses in wm(4)).

   What I would do is:

   (a) Add a boolean flag, say sc_aborting, to hifn_softc, to be
       accessed only while holding sc_mtx; and add a condvar, say
       sc_cv, for transition of sc_aborting from true to false.

   (b) In hifn_crypto, under sc_mtx (which, by the way, it should
       KASSERT is mutex_owned), if sc_aborting, fail with ERESTART.
       Same with anything else that writes new commands to the DMA
       buffer like hifn_compress_enter.

   (c) Defer aborts from interrupt to thread context with a workqueue:
       In hifn_intr, under sc_mtx, if sc_aborting is clear, then set
       it and schedule a worker; the worker will call hifn_abort, and
       then, under sc_mtx, clear sc_aborting, signal a condvar (say
       sc_cv), and call crypto_unblock.

   (d) Have hifn_detach wait under sc_mtx on the condvar until
       sc_aborting is false, then set sc_aborting, release sc_mtx,
       issue hifn_abort, and finally proceed to detach everything as
       before.

   Finally, hifn_abort will need to KASSERT(sc->sc_aborting) and
   ASSERT_SLEEPABLE() instead of KASSERT(mutex_owned(&sc->sc_mtx)).

3. mstohz turns out to be kind of a lousy API -- it doesn't (always?)
   round up, so you should use MIN(1, mstohz(10)) in case anyone tries
   to run this code with HZ=10 in which case mstohz(10) = 0 leads to
   no pause at all.

4. Minor style nit:

   /* single-line comments */

   or

   /*
    * multi-line
    * comments
    */

   but not

   /* mixed single-line
    * and multi-line style */