Any objections to putting this in soon? Any OKs?
I'd like to move forward with iwm(4) (less fixing bugs, more
adding new features...)
On Sun, Oct 11, 2015 at 04:55:29PM +0200, Stefan Sperling wrote:
> The iwm(4) driver pre-allocates fw command payload buffers of 320 bytes.
>
> For some firmware commands, particularly those used when configuring
> the PHY (iwm_send_phy_db_cmd) and running scans (iwm_mvm_scan_request),
> the payload exceeds 320 bytes. I've seen somewhere between 2k and 3.5k
> being used. Precisely these commands fail often while we're trying to
> bring the interface up. You've probably seen "could not initiate scan".
> The PHY failure case doesn't print anything unless IWM_DEBUG is set.
>
> If the payload doesn't fit, the driver tries to use an mbuf instead
> of the pre-allocated payload buffer. This seems to be based on the
> approach taken in iwn(4).
>
> The current code seems confused about sizes.
> If a command requires 'sizeof(cmd->hdr) + paylen' bytes, the driver only
> maps hcmd->len[0] bytes for DMA. But 'paylen' is the sum of hcmod->len[0]
> and hcmd->len[1]. And the DMA map used to map large payloads only handles
> mappings of up to MCLBYTES, which is only 2k...
>
> Fix this by sizing the mbuf and its DMA map correctly.
>
> Index: if_iwm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 if_iwm.c
> --- if_iwm.c 11 Oct 2015 10:22:28 -0000 1.55
> +++ if_iwm.c 11 Oct 2015 14:37:41 -0000
> @@ -1108,14 +1108,21 @@ iwm_alloc_tx_ring(struct iwm_softc *sc,
> paddr = ring->cmd_dma.paddr;
> for (i = 0; i < IWM_TX_RING_COUNT; i++) {
> struct iwm_tx_data *data = &ring->data[i];
> + size_t mapsize;
>
> data->cmd_paddr = paddr;
> data->scratch_paddr = paddr + sizeof(struct iwm_cmd_header)
> + offsetof(struct iwm_tx_cmd, scratch);
> paddr += sizeof(struct iwm_device_cmd);
>
> - error = bus_dmamap_create(sc->sc_dmat, MCLBYTES,
> - IWM_NUM_OF_TBS - 2, MCLBYTES, 0, BUS_DMA_NOWAIT,
> + /* FW commands may require more mapped space than packets. */
> + if (qid == IWM_MVM_CMD_QUEUE)
> + mapsize = (sizeof(struct iwm_cmd_header) +
> + IWM_MAX_CMD_PAYLOAD_SIZE);
> + else
> + mapsize = MCLBYTES;
> + error = bus_dmamap_create(sc->sc_dmat, mapsize,
> + IWM_NUM_OF_TBS - 2, mapsize, 0, BUS_DMA_NOWAIT,
> &data->map);
> if (error != 0) {
> printf("%s: could not create TX buf DMA map\n",
> DEVNAME(sc));
> @@ -3393,30 +3400,31 @@ iwm_send_cmd(struct iwm_softc *sc, struc
> data = &ring->data[ring->cur];
>
> if (paylen > sizeof(cmd->data)) {
> - /* Command is too large */
> - if (sizeof(cmd->hdr) + paylen > IWM_RBUF_SIZE) {
> + /* Command is too large to fit in pre-allocated space. */
> + size_t totlen = sizeof(cmd->hdr) + paylen;
> + if (paylen > IWM_MAX_CMD_PAYLOAD_SIZE) {
> + printf("%s: firmware command too long (%zd bytes)\n",
> + DEVNAME(sc), totlen);
> error = EINVAL;
> goto out;
> }
> - m = m_gethdr(M_DONTWAIT, MT_DATA);
> + m = MCLGETI(NULL, M_DONTWAIT, NULL, totlen);
> if (m == NULL) {
> - error = ENOMEM;
> - goto out;
> - }
> - MCLGETI(m, M_DONTWAIT, NULL, IWM_RBUF_SIZE);
> - if (!(m->m_flags & M_EXT)) {
> - m_freem(m);
> + printf("%s: could not get fw cmd mbuf (%zd bytes)\n",
> + DEVNAME(sc), totlen);
> error = ENOMEM;
> goto out;
> }
> cmd = mtod(m, struct iwm_device_cmd *);
> error = bus_dmamap_load(sc->sc_dmat, data->map, cmd,
> - hcmd->len[0], NULL, BUS_DMA_NOWAIT | BUS_DMA_WRITE);
> + totlen, NULL, BUS_DMA_NOWAIT | BUS_DMA_WRITE);
> if (error != 0) {
> + printf("%s: could not load fw cmd mbuf (%zd bytes)\n",
> + DEVNAME(sc), totlen);
> m_freem(m);
> goto out;
> }
> - data->m = m;
> + data->m = m; /* mbuf will be freed in iwm_cmd_done() */
> paddr = data->map->dm_segs[0].ds_addr;
> } else {
> cmd = &ring->cmd[ring->cur];
> @@ -3447,13 +3455,13 @@ iwm_send_cmd(struct iwm_softc *sc, struc
> code, hcmd->len[0] + hcmd->len[1] + sizeof(cmd->hdr),
> async ? " (async)" : ""));
>
> - if (hcmd->len[0] > sizeof(cmd->data)) {
> - bus_dmamap_sync(sc->sc_dmat, data->map, 0, hcmd->len[0],
> - BUS_DMASYNC_PREWRITE);
> + if (paylen > sizeof(cmd->data)) {
> + bus_dmamap_sync(sc->sc_dmat, data->map, 0,
> + sizeof(cmd->hdr) + paylen, BUS_DMASYNC_PREWRITE);
> } else {
> bus_dmamap_sync(sc->sc_dmat, ring->cmd_dma.map,
> (char *)(void *)cmd - (char *)(void *)ring->cmd_dma.vaddr,
> - hcmd->len[0] + 4, BUS_DMASYNC_PREWRITE);
> + sizeof(cmd->hdr) + hcmd->len[0], BUS_DMASYNC_PREWRITE);
> }
> bus_dmamap_sync(sc->sc_dmat, ring->desc_dma.map,
> (char *)(void *)desc - (char *)(void *)ring->desc_dma.vaddr,
> Index: if_iwmreg.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_iwmreg.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 if_iwmreg.h
> --- if_iwmreg.h 15 Jun 2015 08:06:11 -0000 1.4
> +++ if_iwmreg.h 11 Oct 2015 14:36:04 -0000
> @@ -5141,6 +5141,7 @@ enum iwm_power_scheme {
> };
>
> #define IWM_DEF_CMD_PAYLOAD_SIZE 320
> +#define IWM_MAX_CMD_PAYLOAD_SIZE (4096 - sizeof(struct iwm_cmd_header))
> #define IWM_CMD_FAILED_MSK 0x40
>
> struct iwm_device_cmd {
