Hello,
On 04/19/2010 12:17 PM, Harry Zhang wrote:
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c44d112..8ca16f5 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1185,7 +1185,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> /* set enclosure management message type */
> if (ap->flags & ATA_FLAG_EM)
> - ap->em_message_type = ahci_em_messages;
> + ap->em_message_type = hpriv->em_msg_type;
>
>
> /* disabled/not-implemented port */
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 733def2..6605b03 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -220,13 +220,16 @@ enum {
> ICH_MAP = 0x90, /* ICH MAP register */
>
> /* em constants */
> - EM_MAX_SLOTS = 8,
> - EM_MAX_RETRY = 5,
> + EM_MAX_SLOTS = 8,
> + EM_MAX_RETRY = 5,
Why change the indentation?
> };
>
> struct ahci_cmd_hdr {
> @@ -282,9 +285,10 @@ struct ahci_host_priv {
> u32 saved_cap2; /* saved initial cap2 */
> u32 saved_port_map; /* saved initial port_map */
> u32 em_loc; /* enclosure management location */
> + u32 em_buf_sz; /* EM buffer size in byte*/
> + u32 em_msg_type; /* EM message type */
> };
>
> -extern int ahci_em_messages;
> extern int ahci_ignore_sss;
Please separate out em message enable control changes into a separate
patch.
> +static ssize_t ahci_read_em_buffer(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct Scsi_Host *shost = class_to_shost(dev);
> + struct ata_port *ap = ata_shost_to_port(shost);
> + struct ahci_host_priv *hpriv = ap->host->private_data;
> + void __iomem *mmio = hpriv->mmio;
> + void __iomem *em_mmio = mmio + hpriv->em_loc;
> + u32 em_ctl, msg;
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(ap->lock, flags);
> +
> + em_ctl = readl(mmio + HOST_EM_CTL);
> + if (!(ap->flags & ATA_FLAG_EM) || em_ctl & EM_CTL_XMT ||
> + !(em_ctl & EM_CTL_MR)) {
> + spin_unlock_irqrestore(ap->lock, flags);
> + return -EINVAL;
> + }
Wouldn't !MR be better represented by -EAGAIN?
> + if (!(em_ctl & EM_CTL_SMB))
> + em_mmio += hpriv->em_buf_sz;
> +
> + for (i = 0; i < hpriv->em_buf_sz; i += 4) {
> + msg = readl(em_mmio + i);
> + buf[i] = msg & 0xff;
> + buf[i + 1] = (msg >> 8) & 0xff;
> + buf[i + 2] = (msg >> 16) & 0xff;
> + buf[i + 3] = (msg >> 24) & 0xff;
> + }
Maybe it would be a good idea to check em_buf_sz against PAGE_SIZE?
Hmmm... a question, does the standard define anything about endian?
Other than the above small points, things generally look good to me.
Implementing proper r/w blocking semantics would be nice but given
that ahci doesn't implement EM interrupts (right?), I think the
minimal implementation is okay.
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
