Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] ahci: fix sdb fis semantics

[Stefan Hajnoczi]

On Fri, Jun 26, 2015 at 01:36:23PM -0400, John Snow wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote:
> >> @@ -682,19 +680,22 @@ static void ahci_write_fis_sdb(AHCIState
> >> *s, int port, uint32_t finished)
> >> 
> >> sdb_fis->type = SATA_FIS_TYPE_SDB; /* Interrupt pending &
> >> Notification bit */ -    sdb_fis->flags =
> >> (ad->hba->control_regs.irqstatus ? (1 << 6) : 0); +
> >> sdb_fis->flags = 0x40; /* Interrupt bit, always 1 for NCQ */
> > ...
> >> -    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); +    if
> >> (sdb_fis->flags & 0x40) { +        ahci_trigger_irq(s, ad,
> >> PORT_IRQ_SDB_FIS); +    }
> > 
> > This if statement is always true.
> > 
> 
> Yes. I did that on purpose. Maybe that was too weird.
> 
> The idea was to emphasize that the IRQ is definitely only triggered
> *IF* the interrupt bit is set. It just so happens that we currently
> always set it.
> 
> The generation and trigger mechanics are supposed to be separate, but
> we currently have no use case for them being separate (because we are
> an omniscient HBA-HDD amalgamation), so they're mushed together here.
> 
> This is kind of like a documentation "NB."
> 
> I can replace it with:
> 
> /* Trigger IRQ if interrupt bit is set, which currently it always is: */
> ahci_trigger_irq(...);

Good idea, that makes it clear.

pgp5UUzR9sFWZ.pgp
Description: PGP signature