NetBSD-Bugs archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: kern/39297: mfi calls tsleep() from mfi_intr()



The following reply was made to PR kern/39297; it has been noted by GNATS.

From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: kern-bug-people%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost, 
netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/39297: mfi calls tsleep() from mfi_intr()
Date: Sat, 18 Oct 2008 22:27:32 +0200

 --Dxnq1zWXvFF0Q93v
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On Tue, Aug 05, 2008 at 05:25:00PM +0000, oster%NetBSD.org@localhost wrote:
 >      Running 4.99.71 (and some revisions earlier) on a machine with
 > using the mfi will result in the machine eventually locking up.  Breaking
 > into ddb yields the following:
 > 
 > login: fatal breakpoint trap in supervisor mode
 > trap type 1 code 0 rip ffffffff804dba45 cs 8 rflags 202 cr2  
 > ffff8000720a8000 cp
 > l 8 rsp ffff800062c4b7f8
 > Stopped in pid 0.2 (system) at  netbsd:breakpoint+0x5:  leave
 > db{0}> tr
 > breakpoint() at netbsd:breakpoint+0x5
 > comintr() at netbsd:comintr+0x53a
 > Xintr_ioapic_edge6() at netbsd:Xintr_ioapic_edge6+0xef
 > --- interrupt ---
 > mutex_spin_retry() at netbsd:mutex_spin_retry+0x5a
 > ltsleep() at netbsd:ltsleep+0xe5
 > mfi_mgmt() at netbsd:mfi_mgmt+0xe1
 > mfi_scsipi_request() at netbsd:mfi_scsipi_request+0x331
 > scsipi_run_queue() at netbsd:scsipi_run_queue+0x16e
 > mfi_intr() at netbsd:mfi_intr+0xc0
 > intr_biglock_wrapper() at netbsd:intr_biglock_wrapper+0x1d
 > Xintr_ioapic_level2() at netbsd:Xintr_ioapic_level2+0xf7
 
 Hi, can you try the attached patch ? It should avoid tsleep in the
 mfi_scsipi_request() path.
 
 -- 
 Manuel Bouyer <bouyer%antioche.eu.org@localhost>
      NetBSD: 26 ans d'experience feront toujours la difference
 --
 
 --Dxnq1zWXvFF0Q93v
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename=diff
 
 Index: mfi.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ic/mfi.c,v
 retrieving revision 1.18
 diff -u -r1.18 mfi.c
 --- mfi.c      24 Jun 2008 10:08:43 -0000      1.18
 +++ mfi.c      18 Oct 2008 20:25:38 -0000
 @@ -85,8 +85,10 @@
  static int            mfi_scsi_io(struct mfi_ccb *, struct scsipi_xfer *,
                                uint32_t, uint32_t);
  static void           mfi_scsi_xs_done(struct mfi_ccb *);
 -static int            mfi_mgmt(struct mfi_softc *, uint32_t, uint32_t,
 -                              uint32_t, void *, uint8_t *);
 +static int            mfi_mgmt_internal(struct mfi_softc *,
 +                          uint32_t, uint32_t, uint32_t, void *, uint8_t *);
 +static int            mfi_mgmt(struct mfi_ccb *,struct scsipi_xfer *,
 +                          uint32_t, uint32_t, uint32_t, void *, uint8_t *);
  static void           mfi_mgmt_done(struct mfi_ccb *);
  
  #if NBIO > 0
 @@ -436,7 +438,7 @@
  #endif
        DNPRINTF(MFI_D_MISC, "%s: mfi_get_info\n", DEVNAME(sc));
  
 -      if (mfi_mgmt(sc, MR_DCMD_CTRL_GET_INFO, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_CTRL_GET_INFO, MFI_DATA_IN,
            sizeof(sc->sc_info), &sc->sc_info, NULL))
                return 1;
  
 @@ -701,10 +703,7 @@
        memset(adapt, 0, sizeof(*adapt));
        adapt->adapt_dev = &sc->sc_dev;
        adapt->adapt_nchannels = 1;
 -      if (sc->sc_ld_cnt)
 -              adapt->adapt_openings = sc->sc_max_cmds / sc->sc_ld_cnt;
 -      else
 -              adapt->adapt_openings = sc->sc_max_cmds;
 +      adapt->adapt_openings = sc->sc_max_cmds - 1;
        adapt->adapt_max_periph = adapt->adapt_openings;
        adapt->adapt_request = mfi_scsipi_request;
        adapt->adapt_minphys = mfiminphys;
 @@ -844,8 +843,10 @@
        }
  
        pcq->mpc_consumer = consumer;
 -      bus_dmamap_sync(sc->sc_dmat, MFIMEM_MAP(sc->sc_pcq), 0,
 -          sizeof(uint32_t) * sc->sc_max_cmds + sizeof(struct mfi_prod_cons),
 +      bus_dmamap_sync(sc->sc_dmat, MFIMEM_MAP(sc->sc_pcq), 
 +          sizeof(uint32_t) * sc->sc_max_cmds +
 +          offsetof(struct mfi_prod_cons, mpc_consumer),
 +          sizeof(pcq->mpc_consumer),
            BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
  
        return claimed;
 @@ -1073,19 +1074,13 @@
                break;
  
        case SCSI_SYNCHRONIZE_CACHE_10:
 -              mfi_put_ccb(ccb); /* we don't need this */
 -
                mbox[0] = MR_FLUSH_CTRL_CACHE | MR_FLUSH_DISK_CACHE;
 -              if (mfi_mgmt(sc, MR_DCMD_CTRL_CACHE_FLUSH, MFI_DATA_NONE,
 -                  0, NULL, mbox))
 +              if (mfi_mgmt(ccb, xs,
 +                  MR_DCMD_CTRL_CACHE_FLUSH, MFI_DATA_NONE, 0, NULL, mbox)) {
 +                      mfi_put_ccb(ccb);
                        goto stuffup;
 -              xs->error = XS_NOERROR;
 -              xs->status = SCSI_OK;
 -              xs->resid = 0;
 -              scsipi_done(xs);
 -              splx(s);
 -              return;
 -              /* NOTREACHED */
 +              }
 +              break;
  
        /* hand it of to the firmware and let it deal with it */
        case SCSI_TEST_UNIT_READY:
 @@ -1211,17 +1206,45 @@
  }
  
  static int
 -mfi_mgmt(struct mfi_softc *sc, uint32_t opc, uint32_t dir, uint32_t len,
 -    void *buf, uint8_t *mbox)
 -{
 +mfi_mgmt_internal(struct mfi_softc *sc, uint32_t opc, uint32_t dir,
 +    uint32_t len, void *buf, uint8_t *mbox) {
        struct mfi_ccb          *ccb;
 -      struct mfi_dcmd_frame   *dcmd;
        int                     rv = 1;
  
 -      DNPRINTF(MFI_D_MISC, "%s: mfi_mgmt %#x\n", DEVNAME(sc), opc);
 -
        if ((ccb = mfi_get_ccb(sc)) == NULL)
                return rv;
 +      rv = mfi_mgmt(ccb, NULL, opc, dir, len, buf, mbox);
 +      if (rv)
 +              return rv;
 +
 +      if (cold) {
 +              if (mfi_poll(ccb))
 +                      goto done;
 +      } else {
 +              mfi_post(sc, ccb);
 +
 +              DNPRINTF(MFI_D_MISC, "%s: mfi_mgmt_internal sleeping\n",
 +                  DEVNAME(sc));
 +              while (ccb->ccb_state != MFI_CCB_DONE)
 +                      tsleep(ccb, PRIBIO, "mfi_mgmt", 0);
 +
 +              if (ccb->ccb_flags & MFI_CCB_F_ERR)
 +                      goto done;
 +      }
 +      rv = 0;
 +
 +done:
 +      mfi_put_ccb(ccb);
 +      return rv;
 +}
 +
 +static int
 +mfi_mgmt(struct mfi_ccb *ccb, struct scsipi_xfer *xs,
 +    uint32_t opc, uint32_t dir, uint32_t len, void *buf, uint8_t *mbox)
 +{
 +      struct mfi_dcmd_frame   *dcmd;
 +
 +      DNPRINTF(MFI_D_MISC, "%s: mfi_mgmt %#x\n", DEVNAME(ccb->ccb_sc), opc);
  
        dcmd = &ccb->ccb_frame->mfr_dcmd;
        memset(dcmd->mdf_mbox, 0, MFI_MBOX_SIZE);
 @@ -1231,6 +1254,7 @@
        dcmd->mdf_opcode = opc;
        dcmd->mdf_header.mfh_data_len = 0;
        ccb->ccb_direction = dir;
 +      ccb->ccb_xs = xs;
        ccb->ccb_done = mfi_mgmt_done;
  
        ccb->ccb_frame_size = MFI_DCMD_FRAME_SIZE;
 @@ -1246,33 +1270,15 @@
                ccb->ccb_sgl = &dcmd->mdf_sgl;
  
                if (mfi_create_sgl(ccb, BUS_DMA_WAITOK))
 -                      goto done;
 -      }
 -
 -      if (cold) {
 -              if (mfi_poll(ccb))
 -                      goto done;
 -      } else {
 -              mfi_post(sc, ccb);
 -
 -              DNPRINTF(MFI_D_MISC, "%s: mfi_mgmt sleeping\n", DEVNAME(sc));
 -              while (ccb->ccb_state != MFI_CCB_DONE)
 -                      tsleep(ccb, PRIBIO, "mfi_mgmt", 0);
 -
 -              if (ccb->ccb_flags & MFI_CCB_F_ERR)
 -                      goto done;
 +                      return 1;
        }
 -
 -      rv = 0;
 -
 -done:
 -      mfi_put_ccb(ccb);
 -      return rv;
 +      return 0;
  }
  
  static void
  mfi_mgmt_done(struct mfi_ccb *ccb)
  {
 +      struct scsipi_xfer      *xs = ccb->ccb_xs;
        struct mfi_softc        *sc = ccb->ccb_sc;
        struct mfi_frame_header *hdr = &ccb->ccb_frame->mfr_header;
  
 @@ -1294,8 +1300,18 @@
                ccb->ccb_flags |= MFI_CCB_F_ERR;
  
        ccb->ccb_state = MFI_CCB_DONE;
 -
 -      wakeup(ccb);
 +      if (xs) {
 +              if (hdr->mfh_cmd_status != MFI_STAT_OK) {
 +                      xs->error = XS_DRIVER_STUFFUP;
 +              } else {
 +                      xs->error = XS_NOERROR;
 +                      xs->status = SCSI_OK;
 +                      xs->resid = 0;
 +              }
 +              mfi_put_ccb(ccb);
 +              scsipi_done(xs);
 +      } else 
 +              wakeup(ccb);
  }
  
  #if NBIO > 0
 @@ -1365,7 +1381,8 @@
  
        /* get figures */
        cfg = malloc(sizeof *cfg, M_DEVBUF, M_WAITOK);
 -      if (mfi_mgmt(sc, MD_DCMD_CONF_GET, MFI_DATA_IN, sizeof *cfg, cfg, NULL))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_CONF_GET, MFI_DATA_IN,
 +          sizeof *cfg, cfg, NULL))
                goto freeme;
  
        strlcpy(bi->bi_dev, DEVNAME(sc), sizeof(bi->bi_dev));
 @@ -1387,7 +1404,7 @@
        DNPRINTF(MFI_D_IOCTL, "%s: mfi_ioctl_vol %#x\n",
            DEVNAME(sc), bv->bv_volid);
  
 -      if (mfi_mgmt(sc, MR_DCMD_LD_GET_LIST, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_LD_GET_LIST, MFI_DATA_IN,
            sizeof(sc->sc_ld_list), &sc->sc_ld_list, NULL))
                goto done;
  
 @@ -1396,7 +1413,7 @@
        DNPRINTF(MFI_D_IOCTL, "%s: mfi_ioctl_vol target %#x\n",
            DEVNAME(sc), mbox[0]);
  
 -      if (mfi_mgmt(sc, MR_DCMD_LD_GET_INFO, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_LD_GET_INFO, MFI_DATA_IN,
            sizeof(sc->sc_ld_details), &sc->sc_ld_details, mbox))
                goto done;
  
 @@ -1488,7 +1505,8 @@
  
        /* send single element command to retrieve size for full structure */
        cfg = malloc(sizeof *cfg, M_DEVBUF, M_WAITOK);
 -      if (mfi_mgmt(sc, MD_DCMD_CONF_GET, MFI_DATA_IN, sizeof *cfg, cfg, NULL))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_CONF_GET, MFI_DATA_IN,
 +          sizeof *cfg, cfg, NULL))
                goto freeme;
  
        size = cfg->mfc_size;
 @@ -1496,7 +1514,8 @@
  
        /* memory for read config */
        cfg = malloc(size, M_DEVBUF, M_WAITOK|M_ZERO);
 -      if (mfi_mgmt(sc, MD_DCMD_CONF_GET, MFI_DATA_IN, size, cfg, NULL))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_CONF_GET, MFI_DATA_IN,
 +          size, cfg, NULL))
                goto freeme;
  
        ar = cfg->mfc_array;
 @@ -1560,7 +1579,7 @@
        /* get the remaining fields */
        *((uint16_t *)&mbox) = ar[arr].pd[disk].mar_pd.mfp_id;
        memset(pd, 0, sizeof(*pd));
 -      if (mfi_mgmt(sc, MR_DCMD_PD_GET_INFO, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_PD_GET_INFO, MFI_DATA_IN,
            sizeof *pd, pd, mbox))
                goto freeme;
  
 @@ -1620,7 +1639,7 @@
                return EINVAL;
        }
  
 -      if (mfi_mgmt(sc, opc, dir, sizeof(ret), &ret, NULL))
 +      if (mfi_mgmt_internal(sc, opc, dir, sizeof(ret), &ret, NULL))
                rv = EINVAL;
        else
                if (ba->ba_opcode == BIOC_GASTATUS)
 @@ -1648,7 +1667,7 @@
  
        pd = malloc(MFI_PD_LIST_SIZE, M_DEVBUF, M_WAITOK);
  
 -      if (mfi_mgmt(sc, MR_DCMD_PD_GET_LIST, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_PD_GET_LIST, MFI_DATA_IN,
            MFI_PD_LIST_SIZE, pd, NULL))
                goto done;
  
 @@ -1683,7 +1702,7 @@
        }
  
  
 -      if (mfi_mgmt(sc, cmd, MFI_DATA_NONE, 0, NULL, mbox))
 +      if (mfi_mgmt_internal(sc, cmd, MFI_DATA_NONE, 0, NULL, mbox))
                goto done;
  
        rv = 0;
 @@ -1705,7 +1724,7 @@
  
        pd = malloc(MFI_PD_LIST_SIZE, M_DEVBUF, M_WAITOK);
  
 -      if (mfi_mgmt(sc, MR_DCMD_PD_GET_LIST, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_PD_GET_LIST, MFI_DATA_IN,
            MFI_PD_LIST_SIZE, pd, NULL))
                goto done;
  
 @@ -1750,7 +1769,8 @@
        }
  
  
 -      if (mfi_mgmt(sc, MD_DCMD_PD_SET_STATE, MFI_DATA_NONE, 0, NULL, mbox))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_PD_SET_STATE, MFI_DATA_NONE,
 +          0, NULL, mbox))
                goto done;
  
        rv = 0;
 @@ -1782,7 +1802,8 @@
  
        /* send single element command to retrieve size for full structure */
        cfg = malloc(sizeof *cfg, M_DEVBUF, M_WAITOK);
 -      if (mfi_mgmt(sc, MD_DCMD_CONF_GET, MFI_DATA_IN, sizeof *cfg, cfg, NULL))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_CONF_GET, MFI_DATA_IN,
 +          sizeof *cfg, cfg, NULL))
                goto freeme;
  
        size = cfg->mfc_size;
 @@ -1790,7 +1811,8 @@
  
        /* memory for read config */
        cfg = malloc(size, M_DEVBUF, M_WAITOK|M_ZERO);
 -      if (mfi_mgmt(sc, MD_DCMD_CONF_GET, MFI_DATA_IN, size, cfg, NULL))
 +      if (mfi_mgmt_internal(sc, MD_DCMD_CONF_GET, MFI_DATA_IN,
 +          size, cfg, NULL))
                goto freeme;
  
        /* calculate offset to hs structure */
 @@ -1815,7 +1837,7 @@
        /* get pd fields */
        memset(mbox, 0, sizeof mbox);
        *((uint16_t *)&mbox) = hs[i].mhs_pd.mfp_id;
 -      if (mfi_mgmt(sc, MR_DCMD_PD_GET_INFO, MFI_DATA_IN,
 +      if (mfi_mgmt_internal(sc, MR_DCMD_PD_GET_INFO, MFI_DATA_IN,
            sizeof *pd, pd, mbox)) {
                DNPRINTF(MFI_D_IOCTL, "%s: mfi_vol_hs illegal PD\n",
                    DEVNAME(sc));
 
 --Dxnq1zWXvFF0Q93v--
 


Home | Main Index | Thread Index | Old Index