sata_promise.c:pdc_irq_clear() buglet?

sata_promise.c:pdc_irq_clear() buglet?

[Mikael Pettersson]

Jeff,

In the 2.6.8 kernel you added sata_promise.c:pdc_irq_clear()
which looks as follows:

>static void pdc_irq_clear(struct ata_port *ap)
>{
>        struct ata_host *host = ap->host;
>        void __iomem *mmio = host->iomap[PDC_MMIO_BAR];
>
>        readl(mmio + PDC_INT_SEQMASK);
>}

I have two issues with this code:
1. It reads from the SEQMASK register, which is global for
   all ports. Hence, calling pdc_irq_clear() on one port
   potentially affects all ports.
2. Promise's drivers (both 1st and 2nd generation chip drivers)
   instead read the port's ATA Command/Status register when
   they want to clear any pending IRQs.

Did you write the code this way on purpose, or did you just
accidentally use the wrong mechanism?

If you have no objections, I intend to change this to read
Command/Status instead.

/Mikael

Re: sata_promise.c:pdc_irq_clear() buglet?

[Jeff Garzik]

Mikael Pettersson wrote:
> Jeff,
> 
> In the 2.6.8 kernel you added sata_promise.c:pdc_irq_clear()
> which looks as follows:
> 
>> static void pdc_irq_clear(struct ata_port *ap)
>> {
>>        struct ata_host *host = ap->host;
>>        void __iomem *mmio = host->iomap[PDC_MMIO_BAR];
>>
>>        readl(mmio + PDC_INT_SEQMASK);
>> }
> 
> I have two issues with this code:
> 1. It reads from the SEQMASK register, which is global for
>    all ports. Hence, calling pdc_irq_clear() on one port
>    potentially affects all ports.
> 2. Promise's drivers (both 1st and 2nd generation chip drivers)
>    instead read the port's ATA Command/Status register when
>    they want to clear any pending IRQs.
> 
> Did you write the code this way on purpose, or did you just
> accidentally use the wrong mechanism?
> 
> If you have no objections, I intend to change this to read
> Command/Status instead.

For sata_promise, at the time it was written, irq_clear was largely a 
sledgehammer for SFF situations that didn't automatically resolve 
themselves through normal operation.  Given the amount of SFF code that 
is _not_ enabled when using sata_promise is in use, ->irq_clear() is 
largely a relic for PIO data xfer situations.

Feel free to replace if you think is appropriate...

	Jeff

Re: sata_promise.c:pdc_irq_clear() buglet?

[Mikael Pettersson]

Jeff Garzik writes:
 > Mikael Pettersson wrote:
 > > Jeff,
 > > 
 > > In the 2.6.8 kernel you added sata_promise.c:pdc_irq_clear()
 > > which looks as follows:
 > > 
 > >> static void pdc_irq_clear(struct ata_port *ap)
 > >> {
 > >>        struct ata_host *host = ap->host;
 > >>        void __iomem *mmio = host->iomap[PDC_MMIO_BAR];
 > >>
 > >>        readl(mmio + PDC_INT_SEQMASK);
 > >> }
 > > 
 > > I have two issues with this code:
 > > 1. It reads from the SEQMASK register, which is global for
 > >    all ports. Hence, calling pdc_irq_clear() on one port
 > >    potentially affects all ports.
 > > 2. Promise's drivers (both 1st and 2nd generation chip drivers)
 > >    instead read the port's ATA Command/Status register when
 > >    they want to clear any pending IRQs.
 > > 
 > > Did you write the code this way on purpose, or did you just
 > > accidentally use the wrong mechanism?
 > > 
 > > If you have no objections, I intend to change this to read
 > > Command/Status instead.
 > 
 > For sata_promise, at the time it was written, irq_clear was largely a 
 > sledgehammer for SFF situations that didn't automatically resolve 
 > themselves through normal operation.  Given the amount of SFF code that 
 > is _not_ enabled when using sata_promise is in use, ->irq_clear() is 
 > largely a relic for PIO data xfer situations.
 > 
 > Feel free to replace if you think is appropriate...

Thanks, will do after I've tested it.

/Mikael