uhidev: report sizes
Hi,
Instead of letting uhidev drivers get the report sizes, do it once in
uhidev and pass the same sizes as part of the attach arguments. Makes
the uhidev drivers a bit less repetitive. Note that each call to
uhidev_get_report_size() requires one malloc() and a full traversal of
the HID report. Not that this change will cause any noticeable
difference in terms of time but we can at least avoid doing the same
operation over and over.
It might look tempting to let uhidev assign the sizes after a driver has
attached, removing the need to repeat this logic in each driver. This
does however not work since the input size (sc_isize) must be known
while calling uhidev_open() in order to open the interrupt pipe; and
uhidev_open() is called from several attach routines.
Note that this change only works and applies to when attaching to a
single report ID.
While here, get rid of the unused repsize array.
Comments? OK?
Index: dev/usb/ucc.c
===
RCS file: /cvs/src/sys/dev/usb/ucc.c,v
retrieving revision 1.24
diff -u -p -r1.24 ucc.c
--- dev/usb/ucc.c 2 Sep 2021 15:15:12 - 1.24
+++ dev/usb/ucc.c 9 Sep 2021 05:26:35 -
@@ -633,9 +633,9 @@ ucc_match(struct device *parent, void *m
void *desc;
int size;
- uhidev_get_report_desc(uha->parent, , );
- if (hid_report_size(desc, size, hid_input, uha->reportid) == 0)
+ if (uha->isize == 0)
return UMATCH_NONE;
+ uhidev_get_report_desc(uha->parent, , );
if (!ucc_hid_match(desc, size, uha->reportid))
return UMATCH_NONE;
@@ -648,7 +648,7 @@ ucc_attach(struct device *parent, struct
struct ucc_softc *sc = (struct ucc_softc *)self;
struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux;
void *desc;
- int error, repid, size;
+ int error, size;
sc->sc_mode = WSKBD_TRANSLATED;
sc->sc_last_translate = -1;
@@ -659,10 +659,9 @@ ucc_attach(struct device *parent, struct
sc->sc_hdev.sc_report_id = uha->reportid;
uhidev_get_report_desc(uha->parent, , );
- repid = uha->reportid;
- sc->sc_hdev.sc_isize = hid_report_size(desc, size, hid_input, repid);
- sc->sc_hdev.sc_osize = hid_report_size(desc, size, hid_output, repid);
- sc->sc_hdev.sc_fsize = hid_report_size(desc, size, hid_feature, repid);
+ sc->sc_hdev.sc_isize = uha->isize;
+ sc->sc_hdev.sc_osize = uha->osize;
+ sc->sc_hdev.sc_fsize = uha->fsize;
error = ucc_hid_parse(sc, desc, size);
if (error) {
Index: dev/usb/ugold.c
===
RCS file: /cvs/src/sys/dev/usb/ugold.c,v
retrieving revision 1.17
diff -u -p -r1.17 ugold.c
--- dev/usb/ugold.c 5 Apr 2021 16:26:06 - 1.17
+++ dev/usb/ugold.c 9 Sep 2021 05:26:35 -
@@ -139,7 +139,7 @@ ugold_attach(struct device *parent, stru
{
struct ugold_softc *sc = (struct ugold_softc *)self;
struct uhidev_attach_arg *uha = aux;
- int size, repid;
+ int size;
void *desc;
sc->sc_udev = uha->parent->sc_udev;
@@ -159,10 +159,9 @@ ugold_attach(struct device *parent, stru
}
uhidev_get_report_desc(uha->parent, , );
- repid = uha->reportid;
- sc->sc_hdev.sc_isize = hid_report_size(desc, size, hid_input, repid);
- sc->sc_hdev.sc_osize = hid_report_size(desc, size, hid_output, repid);
- sc->sc_hdev.sc_fsize = hid_report_size(desc, size, hid_feature, repid);
+ sc->sc_hdev.sc_isize = uha->isize;
+ sc->sc_hdev.sc_osize = uha->osize;
+ sc->sc_hdev.sc_fsize = uha->fsize;
if (uhidev_open(>sc_hdev)) {
printf(", unable to open interrupt pipe\n");
Index: dev/usb/uhid.c
===
RCS file: /cvs/src/sys/dev/usb/uhid.c,v
retrieving revision 1.84
diff -u -p -r1.84 uhid.c
--- dev/usb/uhid.c 8 Mar 2021 14:35:57 - 1.84
+++ dev/usb/uhid.c 9 Sep 2021 05:26:35 -
@@ -126,7 +126,7 @@ uhid_attach(struct device *parent, struc
{
struct uhid_softc *sc = (struct uhid_softc *)self;
struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux;
- int size, repid;
+ int size;
void *desc;
sc->sc_hdev.sc_intr = uhid_intr;
@@ -135,10 +135,9 @@ uhid_attach(struct device *parent, struc
sc->sc_hdev.sc_report_id = uha->reportid;
uhidev_get_report_desc(uha->parent, , );
- repid = uha->reportid;
- sc->sc_hdev.sc_isize = hid_report_size(desc, size, hid_input, repid);
- sc->sc_hdev.sc_osize = hid_report_size(desc, size, hid_output, repid);
- sc->sc_hdev.sc_fsize = hid_report_size(desc, size, hid_feature, repid);
+ sc->sc_hdev.sc_isize = uha->isize;
+ sc->sc_hdev.sc_osize = uha->osize;
+ sc->sc_hdev.sc_fsize = uha->fsize;
printf(": input=%d,
Re: do less recallocarray calls in rpki-client
On Wed, Sep 08, 2021 at 06:08:53PM +0200, Claudio Jeker wrote: > On Wed, Sep 08, 2021 at 05:40:31PM +0200, Theo Buehler wrote: > > On Wed, Sep 08, 2021 at 03:05:41PM +0200, Claudio Jeker wrote: > > > Looking at profiling information and the code made me realize that these > > > recallocarray calls growing the array by one every time are unnecessary. > > > The size of the array is known in advance so use that information and > > > build it up ahead of time. > > > > > > In the roa case the IP list is double nested and so one still needs to > > > resize the array but for mft the file-and-hash list is only defined once > > > per MFT and so calloc can be used there. > > > > > > Works for me, the speed-up is not really noticable but I think this change > > > is still worth it. > > > > I'm ok with this. > > > > Should we assert after the for loops that we added as many filehashes or > > addresses as allocated? This is currently more obvious than after this > > change. > > Hmm. The code is correct even if it loads less elements. It will > overallocate but with NULL. Also currently the code fails hard if an > element does not parse. So not sure about that. I agree with all of that, it was just a suggestion. I'm ok with your diff as it is.
Re: do less recallocarray calls in rpki-client
On Wed, Sep 08, 2021 at 05:40:31PM +0200, Theo Buehler wrote:
> On Wed, Sep 08, 2021 at 03:05:41PM +0200, Claudio Jeker wrote:
> > Looking at profiling information and the code made me realize that these
> > recallocarray calls growing the array by one every time are unnecessary.
> > The size of the array is known in advance so use that information and
> > build it up ahead of time.
> >
> > In the roa case the IP list is double nested and so one still needs to
> > resize the array but for mft the file-and-hash list is only defined once
> > per MFT and so calloc can be used there.
> >
> > Works for me, the speed-up is not really noticable but I think this change
> > is still worth it.
>
> I'm ok with this.
>
> Should we assert after the for loops that we added as many filehashes or
> addresses as allocated? This is currently more obvious than after this
> change.
Hmm. The code is correct even if it loads less elements. It will
overallocate but with NULL. Also currently the code fails hard if an
element does not parse. So not sure about that.
> > --
> > :wq Claudio
> >
> > Index: roa.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> > retrieving revision 1.23
> > diff -u -p -r1.23 roa.c
> > --- roa.c 1 Aug 2021 22:29:49 - 1.23
> > +++ roa.c 7 Sep 2021 13:19:19 -
> > @@ -109,10 +109,6 @@ roa_parse_addr(const ASN1_OCTET_STRING *
> > }
> > }
> >
> > - p->res->ips = recallocarray(p->res->ips, p->res->ipsz, p->res->ipsz + 1,
> > - sizeof(struct roa_ip));
> > - if (p->res->ips == NULL)
> > - err(1, NULL);
> > res = >res->ips[p->res->ipsz++];
> >
> > res->addr = addr;
> > @@ -180,6 +176,12 @@ roa_parse_ipfam(const ASN1_OCTET_STRING
> > "failed ASN.1 sequence parse", p->fn);
> > goto out;
> > }
> > +
> > + /* will be called multiple times so use recallocarray */
> > + p->res->ips = recallocarray(p->res->ips, p->res->ipsz,
> > + p->res->ipsz + sk_ASN1_TYPE_num(sseq), sizeof(struct roa_ip));
> > + if (p->res->ips == NULL)
> > + err(1, NULL);
> >
> > for (i = 0; i < sk_ASN1_TYPE_num(sseq); i++) {
> > t = sk_ASN1_TYPE_value(sseq, i);
> > Index: mft.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> > retrieving revision 1.36
> > diff -u -p -r1.36 mft.c
> > --- mft.c 13 Jul 2021 18:39:39 - 1.36
> > +++ mft.c 7 Sep 2021 12:59:57 -
> > @@ -194,12 +194,6 @@ mft_parse_filehash(struct parse *p, cons
> > }
> >
> > /* Insert the filename and hash value. */
> > -
> > - p->res->files = recallocarray(p->res->files, p->res->filesz,
> > - p->res->filesz + 1, sizeof(struct mftfile));
> > - if (p->res->files == NULL)
> > - err(1, NULL);
> > -
> > fent = >res->files[p->res->filesz++];
> >
> > fent->file = fn;
> > @@ -231,6 +225,10 @@ mft_parse_flist(struct parse *p, const A
> > "failed ASN.1 sequence parse", p->fn);
> > goto out;
> > }
> > +
> > + p->res->files = calloc(sk_ASN1_TYPE_num(seq), sizeof(struct mftfile));
> > + if (p->res->files == NULL)
> > + err(1, NULL);
> >
> > for (i = 0; i < sk_ASN1_TYPE_num(seq); i++) {
> > t = sk_ASN1_TYPE_value(seq, i);
> >
>
>
--
:wq Claudio
Re: do less recallocarray calls in rpki-client
On Wed, Sep 08, 2021 at 03:05:41PM +0200, Claudio Jeker wrote:
> Looking at profiling information and the code made me realize that these
> recallocarray calls growing the array by one every time are unnecessary.
> The size of the array is known in advance so use that information and
> build it up ahead of time.
>
> In the roa case the IP list is double nested and so one still needs to
> resize the array but for mft the file-and-hash list is only defined once
> per MFT and so calloc can be used there.
>
> Works for me, the speed-up is not really noticable but I think this change
> is still worth it.
I'm ok with this.
Should we assert after the for loops that we added as many filehashes or
addresses as allocated? This is currently more obvious than after this
change.
> --
> :wq Claudio
>
> Index: roa.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 roa.c
> --- roa.c 1 Aug 2021 22:29:49 - 1.23
> +++ roa.c 7 Sep 2021 13:19:19 -
> @@ -109,10 +109,6 @@ roa_parse_addr(const ASN1_OCTET_STRING *
> }
> }
>
> - p->res->ips = recallocarray(p->res->ips, p->res->ipsz, p->res->ipsz + 1,
> - sizeof(struct roa_ip));
> - if (p->res->ips == NULL)
> - err(1, NULL);
> res = >res->ips[p->res->ipsz++];
>
> res->addr = addr;
> @@ -180,6 +176,12 @@ roa_parse_ipfam(const ASN1_OCTET_STRING
> "failed ASN.1 sequence parse", p->fn);
> goto out;
> }
> +
> + /* will be called multiple times so use recallocarray */
> + p->res->ips = recallocarray(p->res->ips, p->res->ipsz,
> + p->res->ipsz + sk_ASN1_TYPE_num(sseq), sizeof(struct roa_ip));
> + if (p->res->ips == NULL)
> + err(1, NULL);
>
> for (i = 0; i < sk_ASN1_TYPE_num(sseq); i++) {
> t = sk_ASN1_TYPE_value(sseq, i);
> Index: mft.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 mft.c
> --- mft.c 13 Jul 2021 18:39:39 - 1.36
> +++ mft.c 7 Sep 2021 12:59:57 -
> @@ -194,12 +194,6 @@ mft_parse_filehash(struct parse *p, cons
> }
>
> /* Insert the filename and hash value. */
> -
> - p->res->files = recallocarray(p->res->files, p->res->filesz,
> - p->res->filesz + 1, sizeof(struct mftfile));
> - if (p->res->files == NULL)
> - err(1, NULL);
> -
> fent = >res->files[p->res->filesz++];
>
> fent->file = fn;
> @@ -231,6 +225,10 @@ mft_parse_flist(struct parse *p, const A
> "failed ASN.1 sequence parse", p->fn);
> goto out;
> }
> +
> + p->res->files = calloc(sk_ASN1_TYPE_num(seq), sizeof(struct mftfile));
> + if (p->res->files == NULL)
> + err(1, NULL);
>
> for (i = 0; i < sk_ASN1_TYPE_num(seq); i++) {
> t = sk_ASN1_TYPE_value(seq, i);
>
Re: Change vm_dsize to vsize_t
On Tue, 07 Sep 2021 21:38:27 +0200, Mark Kettenis wrote: > I'm not convinced the original diff is right: > > * We have several places in the kernel where we store numbers of pages > in a (32-bit) int. Changing just one of these places is dangerous. > > * Changing the type of just vm_dsize makes no sense. We should change > them all (but see the point above). > > * Does ASAN really need to reserve that much VA space? The oddity here is that p_vm_dsize in kinfo_proc actually corresponds to vm_dused, not vm_dsize. So it is not actually the size of the data segment alone. Since uvmspace_dused() returns vsize_t it does seem like vm_dused should be sized similarly. As things stand, vm_dused could wrap. - todd
Re: let iwx(4) resume in the acpi thread
On Wed, Sep 08, 2021 at 02:19:00PM +0200, Stefan Sperling wrote:
> This patch applies on top of all the other iwx(4) diffs I've sent today.
> It makes iwx(4) initialize the device completely in the acpi thread.
>
> We now prepare the device for loading firmware during DVACT_RESUME,
> and load firmware from host memory into the device during DVACT_WAKEUP.
>
> Previously, DVACT_WAKEUP would schedule the init_task which resets the
> device, undoing work done during DVACT_RESUME, and starts all over again.
>
> ok?
The previous version had a bug: It resumed the device even while the
interface was marked down. Fixed patch below.
diff 055f053850bb0f3af81ea3aa7c4f705a85cfcb76
c734175f035f120197d6be7df1987cb81e535d3e
blob - 51063c862bfc0cf2dc9fbe3f41628bbdbdf3486e
blob + 26f8a7fa85aa48a054d79e7a175e35bfe96a447b
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -490,6 +490,7 @@ voidiwx_attach(struct device *, struct device *,
void
void iwx_init_task(void *);
intiwx_activate(struct device *, int);
intiwx_resume(struct iwx_softc *);
+intiwx_wakeup(struct iwx_softc *);
#if NBPFILTER > 0
void iwx_radiotap_attach(struct iwx_softc *);
@@ -7822,16 +7823,6 @@ iwx_init_hw(struct iwx_softc *sc)
struct ieee80211com *ic = >sc_ic;
int err, i;
- err = iwx_preinit(sc);
- if (err)
- return err;
-
- err = iwx_start_hw(sc);
- if (err) {
- printf("%s: could not initialize hardware\n", DEVNAME(sc));
- return err;
- }
-
err = iwx_run_init_mvm_ucode(sc, 0);
if (err)
return err;
@@ -7984,6 +7975,16 @@ iwx_init(struct ifnet *ifp)
KASSERT(sc->task_refs.refs == 0);
refcnt_init(>task_refs);
+ err = iwx_preinit(sc);
+ if (err)
+ return err;
+
+ err = iwx_start_hw(sc);
+ if (err) {
+ printf("%s: could not initialize hardware\n", DEVNAME(sc));
+ return err;
+ }
+
err = iwx_init_hw(sc);
if (err) {
if (generation == sc->sc_generation)
@@ -9593,6 +9594,30 @@ iwx_resume(struct iwx_softc *sc)
}
int
+iwx_wakeup(struct iwx_softc *sc)
+{
+ struct ieee80211com *ic = >sc_ic;
+ struct ifnet *ifp = >sc_ic.ic_if;
+ int err;
+
+ refcnt_init(>task_refs);
+
+ err = iwx_init_hw(sc);
+ if (err)
+ return err;
+
+ ifq_clr_oactive(>if_snd);
+ ifp->if_flags |= IFF_RUNNING;
+
+ if (ic->ic_opmode == IEEE80211_M_MONITOR)
+ ieee80211_new_state(ic, IEEE80211_S_RUN, -1);
+ else
+ ieee80211_begin_scan(ifp);
+
+ return 0;
+}
+
+int
iwx_activate(struct device *self, int act)
{
struct iwx_softc *sc = (struct iwx_softc *)self;
@@ -9608,15 +9633,27 @@ iwx_activate(struct device *self, int act)
}
break;
case DVACT_RESUME:
+ if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) != IFF_UP)
+ break;
+ sc->sc_flags &= ~IWX_FLAG_SHUTDOWN;
err = iwx_resume(sc);
- if (err)
+ if (err) {
printf("%s: could not initialize hardware\n",
DEVNAME(sc));
+ sc->sc_flags |= IWX_FLAG_SHUTDOWN;
+ }
break;
case DVACT_WAKEUP:
- /* Hardware should be up at this point. */
- if (iwx_set_hw_ready(sc))
- task_add(systq, >init_task);
+ if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) != IFF_UP)
+ break;
+ if (sc->sc_flags & IWX_FLAG_SHUTDOWN)
+ sc->sc_flags &= ~IWX_FLAG_SHUTDOWN;
+ else {
+ err = iwx_wakeup(sc);
+ if (err)
+ printf("%s: could not initialize hardware\n",
+ DEVNAME(sc));
+ }
break;
}
do less recallocarray calls in rpki-client
Looking at profiling information and the code made me realize that these
recallocarray calls growing the array by one every time are unnecessary.
The size of the array is known in advance so use that information and
build it up ahead of time.
In the roa case the IP list is double nested and so one still needs to
resize the array but for mft the file-and-hash list is only defined once
per MFT and so calloc can be used there.
Works for me, the speed-up is not really noticable but I think this change
is still worth it.
--
:wq Claudio
Index: roa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.23
diff -u -p -r1.23 roa.c
--- roa.c 1 Aug 2021 22:29:49 - 1.23
+++ roa.c 7 Sep 2021 13:19:19 -
@@ -109,10 +109,6 @@ roa_parse_addr(const ASN1_OCTET_STRING *
}
}
- p->res->ips = recallocarray(p->res->ips, p->res->ipsz, p->res->ipsz + 1,
- sizeof(struct roa_ip));
- if (p->res->ips == NULL)
- err(1, NULL);
res = >res->ips[p->res->ipsz++];
res->addr = addr;
@@ -180,6 +176,12 @@ roa_parse_ipfam(const ASN1_OCTET_STRING
"failed ASN.1 sequence parse", p->fn);
goto out;
}
+
+ /* will be called multiple times so use recallocarray */
+ p->res->ips = recallocarray(p->res->ips, p->res->ipsz,
+ p->res->ipsz + sk_ASN1_TYPE_num(sseq), sizeof(struct roa_ip));
+ if (p->res->ips == NULL)
+ err(1, NULL);
for (i = 0; i < sk_ASN1_TYPE_num(sseq); i++) {
t = sk_ASN1_TYPE_value(sseq, i);
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.36
diff -u -p -r1.36 mft.c
--- mft.c 13 Jul 2021 18:39:39 - 1.36
+++ mft.c 7 Sep 2021 12:59:57 -
@@ -194,12 +194,6 @@ mft_parse_filehash(struct parse *p, cons
}
/* Insert the filename and hash value. */
-
- p->res->files = recallocarray(p->res->files, p->res->filesz,
- p->res->filesz + 1, sizeof(struct mftfile));
- if (p->res->files == NULL)
- err(1, NULL);
-
fent = >res->files[p->res->filesz++];
fent->file = fn;
@@ -231,6 +225,10 @@ mft_parse_flist(struct parse *p, const A
"failed ASN.1 sequence parse", p->fn);
goto out;
}
+
+ p->res->files = calloc(sk_ASN1_TYPE_num(seq), sizeof(struct mftfile));
+ if (p->res->files == NULL)
+ err(1, NULL);
for (i = 0; i < sk_ASN1_TYPE_num(seq); i++) {
t = sk_ASN1_TYPE_value(seq, i);
let iwx(4) resume in the acpi thread
This patch applies on top of all the other iwx(4) diffs I've sent today.
It makes iwx(4) initialize the device completely in the acpi thread.
We now prepare the device for loading firmware during DVACT_RESUME,
and load firmware from host memory into the device during DVACT_WAKEUP.
Previously, DVACT_WAKEUP would schedule the init_task which resets the
device, undoing work done during DVACT_RESUME, and starts all over again.
ok?
I plan to write a corresponding iwm(4) patch as well.
Tested on a "wise tiger" ax200 in an x250 thinkpad.
diff 055f053850bb0f3af81ea3aa7c4f705a85cfcb76
ef7889d0664ce439e16df65b58a8d19ea0abefd2
blob - 51063c862bfc0cf2dc9fbe3f41628bbdbdf3486e
blob + a1ba4333d1417d9370344115be6abfb8e1378044
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -490,6 +490,7 @@ voidiwx_attach(struct device *, struct device *,
void
void iwx_init_task(void *);
intiwx_activate(struct device *, int);
intiwx_resume(struct iwx_softc *);
+intiwx_wakeup(struct iwx_softc *);
#if NBPFILTER > 0
void iwx_radiotap_attach(struct iwx_softc *);
@@ -7822,16 +7823,6 @@ iwx_init_hw(struct iwx_softc *sc)
struct ieee80211com *ic = >sc_ic;
int err, i;
- err = iwx_preinit(sc);
- if (err)
- return err;
-
- err = iwx_start_hw(sc);
- if (err) {
- printf("%s: could not initialize hardware\n", DEVNAME(sc));
- return err;
- }
-
err = iwx_run_init_mvm_ucode(sc, 0);
if (err)
return err;
@@ -7984,6 +7975,16 @@ iwx_init(struct ifnet *ifp)
KASSERT(sc->task_refs.refs == 0);
refcnt_init(>task_refs);
+ err = iwx_preinit(sc);
+ if (err)
+ return err;
+
+ err = iwx_start_hw(sc);
+ if (err) {
+ printf("%s: could not initialize hardware\n", DEVNAME(sc));
+ return err;
+ }
+
err = iwx_init_hw(sc);
if (err) {
if (generation == sc->sc_generation)
@@ -9593,6 +9594,30 @@ iwx_resume(struct iwx_softc *sc)
}
int
+iwx_wakeup(struct iwx_softc *sc)
+{
+ struct ieee80211com *ic = >sc_ic;
+ struct ifnet *ifp = >sc_ic.ic_if;
+ int err;
+
+ refcnt_init(>task_refs);
+
+ err = iwx_init_hw(sc);
+ if (err)
+ return err;
+
+ ifq_clr_oactive(>if_snd);
+ ifp->if_flags |= IFF_RUNNING;
+
+ if (ic->ic_opmode == IEEE80211_M_MONITOR)
+ ieee80211_new_state(ic, IEEE80211_S_RUN, -1);
+ else
+ ieee80211_begin_scan(ifp);
+
+ return 0;
+}
+
+int
iwx_activate(struct device *self, int act)
{
struct iwx_softc *sc = (struct iwx_softc *)self;
@@ -9608,15 +9633,23 @@ iwx_activate(struct device *self, int act)
}
break;
case DVACT_RESUME:
+ sc->sc_flags &= ~IWX_FLAG_SHUTDOWN;
err = iwx_resume(sc);
- if (err)
+ if (err) {
printf("%s: could not initialize hardware\n",
DEVNAME(sc));
+ sc->sc_flags |= IWX_FLAG_SHUTDOWN;
+ }
break;
case DVACT_WAKEUP:
- /* Hardware should be up at this point. */
- if (iwx_set_hw_ready(sc))
- task_add(systq, >init_task);
+ if (sc->sc_flags & IWX_FLAG_SHUTDOWN)
+ sc->sc_flags &= ~IWX_FLAG_SHUTDOWN;
+ else {
+ err = iwx_wakeup(sc);
+ if (err)
+ printf("%s: could not initialize hardware\n",
+ DEVNAME(sc));
+ }
break;
}
iwx(4) firmware memory fixes
Add a missing call to iwx_ctxt_info_free_fw_img() in an error path
of iwx_ctxt_info_init() which should always free on error.
Also, free firmware paging DMA memory in case loading firmware has failed.
If we don't free paging on error we hit KASSERT(dram->paging == NULL)
in iwx_init_fw_sec() once we try to load firmware again. I have hit
this while debugging firmware load failures during suspend/resume.
(Ideally, we would re-allocate firmware image and paging memory only
after re-loading a potentially different fw image, but this can be
fixed later.)
ok?
diff 50816b19557cd9c29c50f92eebbe32098a494bd3
055f053850bb0f3af81ea3aa7c4f705a85cfcb76
blob - f7d69707ed0a98dfcd7717c9c82faac3af4f39d7
blob + 51063c862bfc0cf2dc9fbe3f41628bbdbdf3486e
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -914,8 +914,10 @@ iwx_ctxt_info_init(struct iwx_softc *sc, const struct
IWX_WRITE(sc, IWX_CSR_CTXT_INFO_BA + 4, paddr >> 32);
/* kick FW self load */
- if (!iwx_nic_lock(sc))
+ if (!iwx_nic_lock(sc)) {
+ iwx_ctxt_info_free_fw_img(sc);
return EBUSY;
+ }
iwx_write_prph(sc, IWX_UREG_CPU_INIT_RUN, 1);
iwx_nic_unlock(sc);
@@ -3364,8 +3366,10 @@ iwx_load_firmware(struct iwx_softc *sc)
/* wait for the firmware to load */
err = tsleep_nsec(>sc_uc, 0, "iwxuc", SEC_TO_NSEC(1));
- if (err || !sc->sc_uc.uc_ok)
+ if (err || !sc->sc_uc.uc_ok) {
printf("%s: could not load firmware, %d\n", DEVNAME(sc), err);
+ iwx_ctxt_info_free_paging(sc);
+ }
iwx_ctxt_info_free_fw_img(sc);
Re: fix iwx(4) firmware loading during resume
On Wed, Sep 08, 2021 at 11:45:25AM +0200, Stefan Sperling wrote:
> I will fix the splnet() issue separately later.
And here is the patch for missing splnet() while loading firmware.
ok?
diff 5ebc9004f07f27c14bbae5eb2e8cab3d8cf3446d
a34aaba3c6977adc3012688a6d24ef6d0499df07
blob - cb1fcf19f26a512274ac04a4e42a6389a203d08a
blob + 9fcdcaf46bc65084bd0ffa109017005665d22692
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -4147,6 +4147,8 @@ iwm_load_firmware(struct iwm_softc *sc, enum iwm_ucode
{
int err;
+ splassert(IPL_NET);
+
sc->sc_uc.uc_intr = 0;
sc->sc_uc.uc_ok = 0;
@@ -4292,7 +4294,7 @@ int
iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justnvm)
{
const int wait_flags = (IWM_INIT_COMPLETE | IWM_CALIB_COMPLETE);
- int err;
+ int err, s;
if ((sc->sc_flags & IWM_FLAG_RFKILL) && !justnvm) {
printf("%s: radio is disabled by hardware switch\n",
@@ -4300,10 +4302,12 @@ iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justn
return EPERM;
}
+ s = splnet();
sc->sc_init_complete = 0;
err = iwm_load_ucode_wait_alive(sc, IWM_UCODE_TYPE_INIT);
if (err) {
printf("%s: failed to load init firmware\n", DEVNAME(sc));
+ splx(s);
return err;
}
@@ -4312,6 +4316,7 @@ iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justn
if (err) {
printf("%s: could not init bt coex (error %d)\n",
DEVNAME(sc), err);
+ splx(s);
return err;
}
}
@@ -4320,6 +4325,7 @@ iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justn
err = iwm_nvm_init(sc);
if (err) {
printf("%s: failed to read nvm\n", DEVNAME(sc));
+ splx(s);
return err;
}
@@ -4327,25 +4333,32 @@ iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justn
IEEE80211_ADDR_COPY(sc->sc_ic.ic_myaddr,
sc->sc_nvm.hw_addr);
+ splx(s);
return 0;
}
err = iwm_sf_config(sc, IWM_SF_INIT_OFF);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
/* Send TX valid antennas before triggering calibrations */
err = iwm_send_tx_ant_cfg(sc, iwm_fw_valid_tx_ant(sc));
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
/*
* Send phy configurations command to init uCode
* to start the 16.0 uCode init image internal calibrations.
*/
err = iwm_send_phy_cfg_cmd(sc);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
/*
* Nothing to do but wait for the init complete and phy DB
@@ -4358,6 +4371,7 @@ iwm_run_init_mvm_ucode(struct iwm_softc *sc, int justn
break;
}
+ splx(s);
return err;
}
blob - 39e5ecf5f6435687d6c998f65d69416ee592217d
blob + f7d69707ed0a98dfcd7717c9c82faac3af4f39d7
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -3350,6 +3350,8 @@ iwx_load_firmware(struct iwx_softc *sc)
struct iwx_fw_sects *fws;
int err;
+ splassert(IPL_NET);
+
sc->sc_uc.uc_intr = 0;
sc->sc_uc.uc_ok = 0;
@@ -3465,7 +3467,7 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
struct iwx_init_extended_cfg_cmd init_cfg = {
.init_flags = htole32(IWX_INIT_NVM),
};
- int err;
+ int err, s;
if ((sc->sc_flags & IWX_FLAG_RFKILL) && !readnvm) {
printf("%s: radio is disabled by hardware switch\n",
@@ -3473,10 +3475,12 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
return EPERM;
}
+ s = splnet();
sc->sc_init_complete = 0;
err = iwx_load_ucode_wait_alive(sc);
if (err) {
printf("%s: failed to load init firmware\n", DEVNAME(sc));
+ splx(s);
return err;
}
@@ -3486,22 +3490,28 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
*/
err = iwx_send_cmd_pdu(sc, IWX_WIDE_ID(IWX_SYSTEM_GROUP,
IWX_INIT_EXTENDED_CFG_CMD), 0, sizeof(init_cfg), _cfg);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
err = iwx_send_cmd_pdu(sc, IWX_WIDE_ID(IWX_REGULATORY_AND_NVM_GROUP,
IWX_NVM_ACCESS_COMPLETE), 0, sizeof(nvm_complete), _complete);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
/* Wait for the init complete notification from the firmware. */
while ((sc->sc_init_complete & wait_flags) != wait_flags) {
err =
net80211: show action frame names in debug output
With 'ifconfig debug' and 11n block ack sessions we see messages such as:
iwm0: sending action to xx:xx:xx:xx:xx:xx
Which is not very informative.
It would be more useful to show the type of action frame being sent.
The patch below implements this for the types we currently care about.
ok?
diff b7449c5d2782f7ea2a807bff8551957655898b20 /usr/src
blob - 8de2361c95de86ae9d0644ecd9c24f3bf3e6a332
file + sys/net80211/ieee80211_output.c
--- sys/net80211/ieee80211_output.c
+++ sys/net80211/ieee80211_output.c
@@ -154,6 +154,18 @@ ieee80211_output(struct ifnet *ifp, struct mbuf *m, st
return (error);
}
+const char *
+ieee80211_action_name(struct ieee80211_frame *wh)
+{
+ const u_int8_t *frm = (const uint8_t *)[1];
+ const char *categ_ba_name[3] = { "addba_req", "addba_resp", "delba" };
+
+ if (frm[0] == IEEE80211_CATEG_BA && frm[1] < nitems(categ_ba_name))
+ return categ_ba_name[frm[1]];
+
+ return "action";
+}
+
/*
* Send a management frame to the specified node. The node pointer
* must have a reference as the pointer will be passed to the driver
@@ -223,15 +235,21 @@ ieee80211_mgmt_output(struct ifnet *ifp, struct ieee80
ieee80211_debug > 1 ||
#endif
(type & IEEE80211_FC0_SUBTYPE_MASK) !=
- IEEE80211_FC0_SUBTYPE_PROBE_RESP)
+ IEEE80211_FC0_SUBTYPE_PROBE_RESP) {
+ const char *subtype_name;
+ if ((type & IEEE80211_FC0_SUBTYPE_MASK) ==
+ IEEE80211_FC0_SUBTYPE_ACTION)
+ subtype_name = ieee80211_action_name(wh);
+ else
+ subtype_name = ieee80211_mgt_subtype_name[
+ (type & IEEE80211_FC0_SUBTYPE_MASK) >>
+ IEEE80211_FC0_SUBTYPE_SHIFT];
printf("%s: sending %s to %s on channel %u mode %s\n",
- ifp->if_xname,
- ieee80211_mgt_subtype_name[
- (type & IEEE80211_FC0_SUBTYPE_MASK)
- >> IEEE80211_FC0_SUBTYPE_SHIFT],
+ ifp->if_xname, subtype_name,
ether_sprintf(ni->ni_macaddr),
ieee80211_chan2ieee(ic, ni->ni_chan),
ieee80211_phymode_name[ic->ic_curmode]);
+ }
}
#ifndef IEEE80211_STA_ONLY
Re: fix iwx(4) firmware loading during resume
On Tue, Sep 07, 2021 at 07:29:37PM +0200, Martin Pieuchot wrote:
> On 07/09/21(Tue) 18:03, Stefan Sperling wrote:
> > On Tue, Sep 07, 2021 at 05:16:52PM +0200, Martin Pieuchot wrote:
> > > On 07/09/21(Tue) 15:48, Stefan Sperling wrote:
> > > > This patch makes iwx(4) resume reliably for me.
> > > >
> > > > There were missing splnet() calls which leads to an obvious race
> > > > between the interrupt handler and the code which triggers firmware
> > > > loading and then sleeps to wait for confirmation.
> > > > This patch adds the missing splnet().
> > > >
> > > > However, even with splnet() protection added I need to add the
> > > > following two lines of code to make the patch work reliably:
> > > >
> > > > /* wait for the firmware to load */
> > > > for (w = 0; !sc->sc_uc.uc_intr && w < 10; w++) {
> > > > err = tsleep_nsec(>sc_uc, 0, "iwxuc",
> > > > MSEC_TO_NSEC(100));
> > > > + /* XXX This should not be needed, should it: */
> > > > + if (err == EWOULDBLOCK && sc->sc_uc.uc_intr)
> > > > + err = 0;
> > > > }
> > > > if (err || !sc->sc_uc.uc_ok)
> > > > printf("%s: could not load firmware, %d\n",
> > > > DEVNAME(sc), err);
> > > >
> > > > Which seems odd. I would expect tsleep to return EWOULDBLOCK only when
> > > > the interrupt handler did not already set uc_intr and call wakeup().
> > >
> > > That suggests the timeout fires before the wakeup(9).
> >
> > Yes, it does.
> >
> > But how could uc_intr already be set to 1 in that case?
>
> Is it set before the timeout fires or before tsleep(9) returns?
>
>
As discussed off-list, the interrupt fired between endtsleep() and
sleep_finish(). This means the driver cannot expect that EWOULDBLOCK
is only returned if the interrupt did not fire.
Instead of dancing around a 100msec timeout, do this like iwn(4) does it
and wait for up to 1 second. This avoids the race between sleep_finish()
and the interrupt handler.
ok?
I will fix the splnet() issue separately later.
diff c05ef66598a004c1e173a5d0fd4cdf5403f2ad99 /usr/src (staged changes)
blob - be92ca7dabbc95aa664db41dbcc8806c766d5c9e
blob + cb1fcf19f26a512274ac04a4e42a6389a203d08a
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -4145,9 +4145,10 @@ iwm_load_firmware_8000(struct iwm_softc *sc, enum iwm_
int
iwm_load_firmware(struct iwm_softc *sc, enum iwm_ucode_type ucode_type)
{
- int err, w;
+ int err;
sc->sc_uc.uc_intr = 0;
+ sc->sc_uc.uc_ok = 0;
if (sc->sc_device_family >= IWM_DEVICE_FAMILY_8000)
err = iwm_load_firmware_8000(sc, ucode_type);
@@ -4158,9 +4159,7 @@ iwm_load_firmware(struct iwm_softc *sc, enum iwm_ucode
return err;
/* wait for the firmware to load */
- for (w = 0; !sc->sc_uc.uc_intr && w < 10; w++) {
- err = tsleep_nsec(>sc_uc, 0, "iwmuc", MSEC_TO_NSEC(100));
- }
+ err = tsleep_nsec(>sc_uc, 0, "iwmuc", SEC_TO_NSEC(1));
if (err || !sc->sc_uc.uc_ok)
printf("%s: could not load firmware\n", DEVNAME(sc));
blob - 428a299e7a2d859aa68a934d4048d843be1835ce
blob + 39e5ecf5f6435687d6c998f65d69416ee592217d
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -3348,9 +3348,10 @@ int
iwx_load_firmware(struct iwx_softc *sc)
{
struct iwx_fw_sects *fws;
- int err, w;
+ int err;
sc->sc_uc.uc_intr = 0;
+ sc->sc_uc.uc_ok = 0;
fws = >sc_fw.fw_sects[IWX_UCODE_TYPE_REGULAR];
err = iwx_ctxt_info_init(sc, fws);
@@ -3360,9 +3361,7 @@ iwx_load_firmware(struct iwx_softc *sc)
}
/* wait for the firmware to load */
- for (w = 0; !sc->sc_uc.uc_intr && w < 10; w++) {
- err = tsleep_nsec(>sc_uc, 0, "iwxuc", MSEC_TO_NSEC(100));
- }
+ err = tsleep_nsec(>sc_uc, 0, "iwxuc", SEC_TO_NSEC(1));
if (err || !sc->sc_uc.uc_ok)
printf("%s: could not load firmware, %d\n", DEVNAME(sc), err);
Re: mutex(9): initialize some more mutexes before use?
On 07/09/21(Tue) 14:19, Patrick Wildt wrote:
> Hi,
>
> I was playing around a little with the mutex code and found that on
> arm64 there some uninitialized mutexes out there.
>
> I think the arm64 specific one is comparatively easy to solve. We
> either initialize the mtx when we initialize the rest of the pmap, or
> we move it into the global definition of those. I opted for the former
> version.
Is the kernel pmap mutex supposed to be used? On i386 it isn't so the
mutex's IPL is set to -1 and we added a KASSERT() in splraise() to spot
any mistake.
> The other one prolly needs more discussion/debugging. So uvm_init()
> calls first pmap_init() and then uvm_km_page_init(). The latter does
> initialize the mutex, but arm64's pmap_init() already uses pools, which
> uses km_alloc, which then uses that mutex. Now one easy fix would be
> to just initialize the definition right away instead of during runtime.
>
> But there might be the question if arm64's pmap is allowed to use pools
> and km_alloc during pmap_init.
That's a common question for the family of pmaps calling pool_setlowat()
in pmap_init(). That's where pool_prime() is called from.
> #0 0xff800073f984 in mtx_enter (mtx=0xff8000f3b048 )
> at /usr/src/sys/kern/kern_lock.c:281
> #1 0xff8000937e6c in km_alloc (sz= dwarf expression opcode 0xa3>, kv=0xff8000da6a30 ,
> kp=0xff8000da6a48 , kd=0xff8000e934d8)
> at /usr/src/sys/uvm/uvm_km.c:899
> #2 0xff800084d804 in pool_page_alloc (pp= Unhandled dwarf expression opcode 0xa3>, flags= Unhandled dwarf expression opcode 0xa3>,
> slowdown= 0xa3>) at /usr/src/sys/kern/subr_pool.c:1633
> #3 0xff800084f8dc in pool_allocator_alloc (pp=0xff8000ea6e40
> , flags=65792, slowdown=0xff80026cd098) at
> /usr/src/sys/kern/subr_pool.c:1602
> #4 0xff800084ef08 in pool_p_alloc (pp=0xff8000ea6e40
> , flags=2, slowdown=0xff8000e9359c) at
> /usr/src/sys/kern/subr_pool.c:926
> #5 0xff800084f808 in pool_prime (pp=, n= variable: Unhandled dwarf expression opcode 0xa3>) at
> /usr/src/sys/kern/subr_pool.c:896
> #6 0xff800048c20c in pmap_init () at
> /usr/src/sys/arch/arm64/arm64/pmap.c:1682
> #7 0xff80009384dc in uvm_init () at /usr/src/sys/uvm/uvm_init.c:118
> #8 0xff800048e664 in main (framep= dwarf expression opcode 0xa3>) at /usr/src/sys/kern/init_main.c:235
>
> diff --git a/sys/arch/arm64/arm64/pmap.c b/sys/arch/arm64/arm64/pmap.c
> index 79a344cc84e..f070f4540ec 100644
> --- a/sys/arch/arm64/arm64/pmap.c
> +++ b/sys/arch/arm64/arm64/pmap.c
> @@ -1308,10 +1308,12 @@ pmap_bootstrap(long kvo, paddr_t lpt1, long
> kernelstart, long kernelend,
> pmap_kernel()->pm_vp.l1 = (struct pmapvp1 *)va;
> pmap_kernel()->pm_privileged = 1;
> pmap_kernel()->pm_asid = 0;
> + mtx_init(_kernel()->pm_mtx, IPL_VM);
>
> pmap_tramp.pm_vp.l1 = (struct pmapvp1 *)va + 1;
> pmap_tramp.pm_privileged = 1;
> pmap_tramp.pm_asid = 0;
> + mtx_init(_tramp.pm_mtx, IPL_VM);
>
> /* Mark ASID 0 as in-use. */
> pmap_asid[0] |= (3U << 0);
> diff --git a/sys/uvm/uvm_km.c b/sys/uvm/uvm_km.c
> index 4a60377e9d7..e77afeda832 100644
> --- a/sys/uvm/uvm_km.c
> +++ b/sys/uvm/uvm_km.c
> @@ -644,7 +644,7 @@ uvm_km_page_lateinit(void)
> * not zero filled.
> */
>
> -struct uvm_km_pages uvm_km_pages;
> +struct uvm_km_pages uvm_km_pages = { .mtx = MUTEX_INITIALIZER(IPL_VM) };
>
> void uvm_km_createthread(void *);
> void uvm_km_thread(void *);
> @@ -664,7 +664,6 @@ uvm_km_page_init(void)
> int len, bulk;
> vaddr_t addr;
>
> - mtx_init(_km_pages.mtx, IPL_VM);
> if (!uvm_km_pages.lowat) {
> /* based on physmem, calculate a good value here */
> uvm_km_pages.lowat = physmem / 256;
>
