I don't remember it already, but you seem to be right, I see the 24 hours limit removed in 40e794ab19fb 9 years ago.

The only case I see AHCI to return CAM_SEL_TIMEOUT is:

 if (ch->devices == 0 ||
    (ch->pm_present == 0 &&
     ccb->ccb_h.target_id > 0 && ccb->ccb_h.target_id < 15)) {
        ccb->ccb_h.status = CAM_SEL_TIMEOUT;
        break;
}

, which means device is not detected. I don't see how it can be transient and why it should be retried.

I think both error codes are correct there, but ENXIO indeed should be more informative here.

In D35057#794539, @imp wrote:

So should I add AC_LOST_DEVICE as an explicit case here? I'm sorry, but I'm having trouble understanding your point...

async handlers are called only for the events explicitly specified to xpt_register_async(). As such, every periph driver specifies there AC_LOST_DEVICE, handled by cam_periph_async(). It means every driver has to know what cam_periph_async() can actually handle and how better call it. In such situation "default" case (calling always) can be useful only to not explicitly list new event types sometimes in the future, that may actually be more confusing.

Are you going to do the same for SCSI to not diverge them?

While I see no problem with this patch, I am not sure it is not an obfuscation. The same checks are done (or should be done) at every probe stage. The check seems to be missing on transition from ADA_CCB_RAHEAD to ADA_STATE_WCACHE. I think "goto skipstate" logic of dastart() could be more universal.

I'm just thinking why not to remove apei_devclass same time. Unless you wish this to be mergeable separately.

In D33693#792941, @hselasky wrote:

@mav : This is not about sbt vs HZ.

When multiple programs run at the same priority, I think they shifted around every system tick ???

@hselasky Do you know some time-critical places that still depending on HZ, not rewritten to use _sbt() variants?

I've just noticed this touches only kernel. It would probably be good for consistency do the same in user-space (lib/geom), like gpart_backup()/gpart_restore()/gpart_bootcode()/etc.

I have no objections except some thoughts on function names.

I have no objections. Only wonder what is the mentioned relations to MTU and what happen if MTU change live?

I am not really familiar with this hardware to review this. Only fixed couple mechanical issues.

I'm OK on the main topic, but have comment on another one touched.

I am not familiar with dialogs or this code. I've tried to look for GEOM-related changes, but haven't found any.

I don't have specific objections except nostalgic. I was thinking about it from perspective of Giant lock many of them use. There are few Giant-locked PCI sound drivers too, but that may go separately: either fix or also remove.

I've somehow missed intsts != 0xffffffff part when I looked on this patch, I though it is about locking. Is definitely explains why it helps. On downside I can only think that in theory there can be a controller with all 30 streams supported and all requiring attention same time. Though that may be quite unrealistic.

I don't see a problem from this patch, if it helps (assuming HDAC_INTSTS still returns sane zero when HDAC_RIRBSTS turns mentioned 0xff, otherwise I am not sure what it fixes). May be I am missing some other motivation Andriy had when written this part, but if we assume that all/most modern hardware use non-shared MSI interrupts, there should be almost no cases when interrupt got delivered but HDAC_INTSTS is still zero, and so taking lock before the read should only slightly increase lock hold time for two register reads. We pay that price in much more busy drivers than sound.

I have no objections. Just few thoughts inline. It would help if you describe it a bit more in description, so we would not have to guess what it should do.

It makes some sense to me. I suppose other cases could be NOP and BA_ACC/BA_REJ, which do not require action if unsolicited. I just guess incoming BA_* for outgoing ABTS are handled somewhere else, if they reach the driver at all.

I don't think I can properly review this change without diving in deeply. It just looks weird to me first print "shutting down sport" but then "p2p winner, domain already attached\n" and post OCS_EVT_DOMAIN_ATTACH_OK.