On Mon, Mar 10, 2025 at 04:43:30PM +0200, Andrius V wrote: > Taylor has a good analysis and explanation of the issue > https://mail-index.netbsd.org/netbsd-bugs/2024/03/26/msg082202.html, > which I also noticed by testing ATA RAID setup on VIA controllers.
> [...] Good analysis. > Given that three statuses are defined for adi_status > (https://nxr.netbsd.org/xref/src/sys/dev/ata/ata_raidvar.h#75), I > probably need to check if any of the flags are defined ((adi_status & > (ADI_S_ONLINE | ADI_S_ASSIGNED | ADI_S_SPARE)) instead > (https://netbsd.org/~andvar/ata_raid_fix2.diff). The first part seems fine. Although I didn't look at what the difference between vn_bdev_open() vs. the deleted code is. In the second part this bit is weird for me to read: + else + vp = NULL; if (vp == NULL) { I'd write it as: - vp = ata_raid_disk_vnode_find(adi); + vp = NULL; + if (adi->adi_status & + (ADI_S_ONLINE | ADI_S_ASSIGNED | ADI_S_SPARE)) if (vp == NULL) { /* * XXX This is bogus. We should just mark the But Taylor seemed to be fine with it if I remember the discussion on ICB correctly. A matter of style I guess. > Another alternative is to check that adi->adi_dev IS NULL as Taylor > proposed in his analysis thread. But that proposal never explained how an adv with adi->adi_dev==NULL ended up on the ataraid_disk_vnode_list which isn't supposed to happen! --chris
