Re: git: 6eb503116e88 - main - sdio: don't use CAM_PRIORITY_NONE for queued CCB-s

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.
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.

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.

-- 
Andriy Gapon