Jeff Garzik wrote:
>> + * - ATA disks work.
>> + * - Hotplug works.
>> + * - ATAPI read works but burning doesn't. This thing is really
>> + * peculiar about ATAPI and I couldn't figure out how ATAPI PIO and
>> + * ATAPI DMA WRITE should be programmed. If you've got a clue, be
>> + * my guest.
>> + * - Both STR and STD work.
>
> Do everyday users get a sane error, or something bad like a lockup, when
> trying to ATAPI write?
It will simply fail. No lock up.
>> +struct inic_port_priv {
>> + u8 dfl_prdctl, cached_prdctl;
>
>> + u8 cached_pirq_mask;
>
> apply standard struct style:
>
> 1) one struct member per line
> 2) indent between type and member name
Okay.
>> +static void set_pirq_mask(struct ata_port *ap, u8 mask)
>> +{
>> + struct inic_port_priv *pp = ap->private_data;
>> +
>> + if (pp->cached_pirq_mask != mask)
>> + __set_pirq_mask(ap, mask);
>> +}
>
>
> You should either flush here, or in the one case you need it, ->thaw
Still dazed and scared about those flushes. Will add it to ->freeze and
->thaw.
>> +static u32 inic_scr_read(struct ata_port *ap, unsigned sc_reg)
>> +{
>> + void __iomem *scr_addr = (void __iomem *)ap->ioaddr.scr_addr;
>> + if (sc_reg < ARRAY_SIZE(scr_map)) {
>> + void __iomem *addr;
>> + u32 val;
>> +
>> + addr = scr_addr + scr_map[sc_reg] * 4;
>> + val = readl(scr_addr + scr_map[sc_reg] * 4);
>> +
>> + /* this controller has stuck DIAG.N, ignore it */
>> + if (sc_reg == SCR_ERROR)
>> + val &= ~SERR_PHYRDY_CHG;
>> + return val;
>> + }
>> + return 0xffffffffU;
>> +}
>
> style: the main body of code should not be indented.
>
> more appropriate:
>
> if (unlikely(sc_reg >= ARRAY_SIZE(scr_map)))
> return 0xffffffffU;
> ...
>
> Or perhaps audit the code and ensure that the core never calls with an
> SCR index greater than 2 (SCR_CONTROL), and then add
>
> BUG_ON(sc_ref > SCR_CONTROL);
I'll take the first option for the time being.
>> +static void inic_error_handler(struct ata_port *ap)
>> +{
>> + void __iomem *port_base = get_port_base(ap);
>> + struct inic_port_priv *pp = ap->private_data;
>> + unsigned long flags;
>> +
>> + /* reset PIO HSM and stop DMA engine */
>> + reset_port(port_base);
>
> This function name is a bit too generic, and more difficult to narrow
> down to the single driver when viewed in an oops stack trace
Will add inic_ prefixes.
>> +static void inic_dev_config(struct ata_port *ap, struct ata_device *dev)
>> +{
>> + /* inic can only handle upto LBA28 max sectors */
>> + if (dev->max_sectors > ATA_MAX_SECTORS)
>> + dev->max_sectors = ATA_MAX_SECTORS;
>> +}
>
> why is this needed? scsi host template should take care of this, right?
No, not really. This is the only and correct place to configure max
sectors. We do 'blk_queue_max_sectors(sdev->request_queue,
dev->max_sectors)' unconditionally during slave_config().
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
