Date: Wed, 6 Aug 2025 13:54:52 +0000 (UTC) From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: Andriy Gapon <avg@FreeBSD.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 6eb503116e88 - main - sdio: don't use CAM_PRIORITY_NONE for queued CCB-s Message-ID: <9s0qo0o4-3275-p062-0qnq-nro5prqq2881@SerrOFQ.bet> In-Reply-To: <504491b6-932a-4e68-a324-2f7fc2637f63@FreeBSD.org> References: <202508051629.575GTBYK035918@gitrepo.freebsd.org> <030509rq-32n5-3rn0-98nq-o154pqo676s9@SerrOFQ.bet> <504491b6-932a-4e68-a324-2f7fc2637f63@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1098556516-1822392576-1754488492=:4561 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT On Wed, 6 Aug 2025, Andriy Gapon wrote: > On 05/08/2025 23:32, Bjoern A. Zeeb wrote: >> On Tue, 5 Aug 2025, Andriy Gapon wrote: >> >>> The branch main has been updated by avg: >>> >>> URL: https://cgit.FreeBSD.org/src/commit/? >>> id=6eb503116e88cc430c2c9f01f48aa979fb0a7e1b >>> >>> commit 6eb503116e88cc430c2c9f01f48aa979fb0a7e1b >>> Author: Andriy Gapon <avg@FreeBSD.org> >>> AuthorDate: 2025-08-05 14:40:21 +0000 >>> Commit: Andriy Gapon <avg@FreeBSD.org> >>> CommitDate: 2025-08-05 16:27:12 +0000 >>> >>> sdio: don't use CAM_PRIORITY_NONE for queued CCB-s >>> >>> This is similar to changes done in other CAM drivers and fixes a panic >>> because of a sanity check added in b4b166b8c46b. >>> >>> While here, remove unneeded ccb setup in sdiobdiscover. >>> It's possible that ccb allocation in that function is not needed as >>> well, but I wasn't sure about that. >> >> I have a lot more changes to sdio here; I would appreciate if they go >> through review if you have more. > > Sorry about that. > No, this was a one-off change to fix a panic on boot after an upgrade. Sorry, I fixed too many things recently incl. dwmmc and probably forgot that it didn't boot anymore (I likely had done the NONE->NORMAL change even before booting following imp's other changes) and didn't even notice. > I didn't see any recent non-trivial activity in the file, so I skipped a > review. > >> Are you testing this with any device? > > Not really testing but I have an old Orange Pi PC Plus which got broken > (panic on boot) after an update to the latest main (+INVARIANTS). I think > that it has a WiFi device on SDIO. What WiFi is that? Broadcom, Realtek or something else? > sdiob0: <SDIO CAM-Newbus bridge> on aw_mmc1 > sdiob0 at aw_mmc_sim1 bus 0 scbus1 target 0 lun 0 > sdiob0: Relative addr: 00000001 > Card features: <SDIO> > Card IO OCR: 00fc0000 > > >>> MFC after: 1 week >>> --- >>> sys/dev/sdio/sdiob.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/sys/dev/sdio/sdiob.c b/sys/dev/sdio/sdiob.c >>> index 4ec2058fa2e4..cb2cc0da6b77 100644 >>> --- a/sys/dev/sdio/sdiob.c >>> +++ b/sys/dev/sdio/sdiob.c >>> @@ -150,7 +150,7 @@ sdiob_rw_direct_sc(struct sdiob_softc *sc, uint8_t fn, >>> uint32_t addr, bool wr, >>> sc->ccb = xpt_alloc_ccb(); >>> else >>> memset(sc->ccb, 0, sizeof(*sc->ccb)); >>> - xpt_setup_ccb(&sc->ccb->ccb_h, sc->periph->path, CAM_PRIORITY_NONE); >>> + xpt_setup_ccb(&sc->ccb->ccb_h, sc->periph->path, >>> CAM_PRIORITY_NORMAL); >>> CAM_DEBUG(sc->ccb->ccb_h.path, CAM_DEBUG_TRACE, >>> ("%s(fn=%d, addr=%#02x, wr=%d, *val=%#02x)\n", __func__, >>> fn, addr, wr, *val)); >>> @@ -250,7 +250,7 @@ sdiob_rw_extended_cam(struct sdiob_softc *sc, uint8_t >>> fn, uint32_t addr, >>> sc->ccb = xpt_alloc_ccb(); >>> else >>> memset(sc->ccb, 0, sizeof(*sc->ccb)); >>> - xpt_setup_ccb(&sc->ccb->ccb_h, sc->periph->path, CAM_PRIORITY_NONE); >>> + xpt_setup_ccb(&sc->ccb->ccb_h, sc->periph->path, >>> CAM_PRIORITY_NORMAL); >>> CAM_DEBUG(sc->ccb->ccb_h.path, CAM_DEBUG_TRACE, >>> ("%s(fn=%d addr=%#0x wr=%d b_count=%u blksz=%u buf=%p incr=%d)\n", >>> __func__, fn, addr, wr, b_count, blksz, buffer, incaddr)); >>> @@ -977,9 +977,6 @@ sdiobdiscover(void *context, int pending) >>> >>> if (sc->ccb == NULL) >>> sc->ccb = xpt_alloc_ccb(); >>> - else >>> - memset(sc->ccb, 0, sizeof(*sc->ccb)); >>> - xpt_setup_ccb(&sc->ccb->ccb_h, periph->path, CAM_PRIORITY_NONE); >> >> This likely just made a problem worse of ccb re-use. I have locally >> changed them to be allocated/freed per transaction now. > From my examination of the code, the sc->ccb is always re-initialized before > actual use. I mean memset + xpt_setup_ccb in sdiob_rw_direct_sc and > sdiob_rw_extended_cam. > > -- Bjoern A. Zeeb r15:7 --1098556516-1822392576-1754488492=:4561--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9s0qo0o4-3275-p062-0qnq-nro5prqq2881>