Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
On Sun, Sep 07, 2014 at 01:24:47PM +0200, Hannes Reinecke wrote: > Which was actually my first attempt, but then I figured I'd be > increasing the stacksize in doing so. > But sure, if you're okay with it I'll be redoing the patch. The struct is only 32 bytes. I don't think it's gonna make any meaningful difference. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
On 09/06/2014 02:52 PM, Tejun Heo wrote: Hello, On Sat, Sep 06, 2014 at 10:21:51AM +0200, Hannes Reinecke wrote: Well, yes, in principle. I was looking into that, too. But then I figured that moving to ata_taskfile would be a major overhaul for libsas, which would be quite beyond scope here. And all for a puny little patch. Hmm? Just allocate a ata_taskfile stuct on stack when invoking the function. The change would be a lot smaller actually. Which was actually my first attempt, but then I figured I'd be increasing the stacksize in doing so. But sure, if you're okay with it I'll be redoing the patch. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
Hello, On Sat, Sep 06, 2014 at 10:21:51AM +0200, Hannes Reinecke wrote: > Well, yes, in principle. I was looking into that, too. > But then I figured that moving to ata_taskfile would be a major overhaul for > libsas, which would be quite beyond scope here. > And all for a puny little patch. Hmm? Just allocate a ata_taskfile stuct on stack when invoking the function. The change would be a lot smaller actually. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
On 09/06/2014 01:42 AM, Tejun Heo wrote: Hello, Hannes. Sorry about the delay. On Wed, Jul 30, 2014 at 09:55:08AM +0200, Hannes Reinecke wrote: ata_dev_classify() just uses the 'lbah' and 'lbam' fields from the taskfile, so we can as well use those as arguments and rip out the custom code from sas_ata. I wonder whether it'd easier to just make sas code pass in ata_taskfile instead? The interface which takes three consecutive u8's is kinda error-prone. Well, yes, in principle. I was looking into that, too. But then I figured that moving to ata_taskfile would be a major overhaul for libsas, which would be quite beyond scope here. And all for a puny little patch. --- a/drivers/scsi/aic94xx/aic94xx_task.c +++ b/drivers/scsi/aic94xx/aic94xx_task.c @@ -373,10 +373,10 @@ static int asd_build_ata_ascb(struct asd_ascb *ascb, struct sas_task *task, if (unlikely(task->ata_task.device_control_reg_update)) scb->header.opcode = CONTROL_ATA_DEV; - else if (dev->sata_dev.command_set == ATA_COMMAND_SET) - scb->header.opcode = INITIATE_ATA_TASK; - else + else if (dev->sata_dev.class == ATA_DEV_ATAPI) scb->header.opcode = INITIATE_ATAPI_TASK; + else + scb->header.opcode = INITIATE_ATA_TASK; Are these changes covered by the patch description? Looks like the patch is mixing two separate logical changes. Okay, I'll be splitting them up. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
Hello, Hannes. Sorry about the delay. On Wed, Jul 30, 2014 at 09:55:08AM +0200, Hannes Reinecke wrote: > ata_dev_classify() just uses the 'lbah' and 'lbam' > fields from the taskfile, so we can as well use those > as arguments and rip out the custom code from sas_ata. I wonder whether it'd easier to just make sas code pass in ata_taskfile instead? The interface which takes three consecutive u8's is kinda error-prone. > --- a/drivers/scsi/aic94xx/aic94xx_task.c > +++ b/drivers/scsi/aic94xx/aic94xx_task.c > @@ -373,10 +373,10 @@ static int asd_build_ata_ascb(struct asd_ascb *ascb, > struct sas_task *task, > > if (unlikely(task->ata_task.device_control_reg_update)) > scb->header.opcode = CONTROL_ATA_DEV; > - else if (dev->sata_dev.command_set == ATA_COMMAND_SET) > - scb->header.opcode = INITIATE_ATA_TASK; > - else > + else if (dev->sata_dev.class == ATA_DEV_ATAPI) > scb->header.opcode = INITIATE_ATAPI_TASK; > + else > + scb->header.opcode = INITIATE_ATA_TASK; Are these changes covered by the patch description? Looks like the patch is mixing two separate logical changes. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] libata: consolidate ata_dev_classify()
ata_dev_classify() just uses the 'lbah' and 'lbam' fields from the taskfile, so we can as well use those as arguments and rip out the custom code from sas_ata. Cc: Dan Williams Signed-off-by: Hannes Reinecke --- drivers/ata/libahci.c | 11 +++ drivers/ata/libata-core.c | 14 + drivers/ata/libata-sff.c| 2 +- drivers/ata/sata_fsl.c | 11 +++ drivers/ata/sata_inic162x.c | 2 +- drivers/ata/sata_sil24.c| 2 +- drivers/scsi/aic94xx/aic94xx_task.c | 10 +++--- drivers/scsi/isci/request.c | 4 +-- drivers/scsi/libsas/sas_ata.c | 63 + drivers/scsi/mvsas/mv_sas.c | 4 +-- drivers/scsi/pm8001/pm8001_hwi.c| 2 +- drivers/scsi/pm8001/pm80xx_hwi.c| 2 +- include/linux/libata.h | 2 +- include/scsi/libsas.h | 11 ++- 14 files changed, 44 insertions(+), 96 deletions(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index d72ce04..09f9fcb 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1185,16 +1185,15 @@ static void ahci_dev_config(struct ata_device *dev) unsigned int ahci_dev_classify(struct ata_port *ap) { void __iomem *port_mmio = ahci_port_base(ap); - struct ata_taskfile tf; + u8 lbah, lbam, lbal; u32 tmp; tmp = readl(port_mmio + PORT_SIG); - tf.lbah = (tmp >> 24) & 0xff; - tf.lbam = (tmp >> 16) & 0xff; - tf.lbal = (tmp >> 8)& 0xff; - tf.nsect= (tmp) & 0xff; + lbah= (tmp >> 24) & 0xff; + lbam= (tmp >> 16) & 0xff; + lbal= (tmp >> 8)& 0xff; - return ata_dev_classify(&tf); + return ata_dev_classify(lbah, lbam, lbal); } EXPORT_SYMBOL_GPL(ahci_dev_classify); diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 18d97d5..2c4d742 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1032,7 +1032,9 @@ const char *sata_spd_string(unsigned int spd) /** * ata_dev_classify - determine device type based on ATA-spec signature - * @tf: ATA taskfile register set for device to be identified + * @lbah: LBA high byte (LBA bits 23:16) + * @lbam: LBA mid byte (LBA bits 15:8) + * @lbal: LBA low byte (LBA bits 7:0) * * Determine from taskfile register contents whether a device is * ATA or ATAPI, as per "Signature and persistence" section @@ -1045,7 +1047,7 @@ const char *sata_spd_string(unsigned int spd) * Device type, %ATA_DEV_ATA, %ATA_DEV_ATAPI, %ATA_DEV_PMP or * %ATA_DEV_UNKNOWN the event of failure. */ -unsigned int ata_dev_classify(const struct ata_taskfile *tf) +unsigned int ata_dev_classify(u8 lbah, u8 lbam, u8 lbal) { /* Apple's open source Darwin code hints that some devices only * put a proper signature into the LBA mid/high registers, @@ -1068,22 +1070,22 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf) * SEMB signature. This is worked around in * ata_dev_read_id(). */ - if ((tf->lbam == 0) && (tf->lbah == 0)) { + if ((lbam == 0) && (lbah == 0)) { DPRINTK("found ATA device by sig\n"); return ATA_DEV_ATA; } - if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) { + if ((lbam == 0x14) && (lbah == 0xeb)) { DPRINTK("found ATAPI device by sig\n"); return ATA_DEV_ATAPI; } - if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) { + if ((lbam == 0x69) && (lbah == 0x96)) { DPRINTK("found PMP device by sig\n"); return ATA_DEV_PMP; } - if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) { + if ((lbam == 0x3c) && (lbah == 0xc3)) { DPRINTK("found SEMB device by sig (could be ATA device)\n"); return ATA_DEV_SEMB; } diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index 1121153..ec9b5db 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -1903,7 +1903,7 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present, return ATA_DEV_NONE; /* determine if device is ATA or ATAPI */ - class = ata_dev_classify(&tf); + class = ata_dev_classify(tf.lbah, tf.lbam, tf.lbal); if (class == ATA_DEV_UNKNOWN) { /* If the device failed diagnostic, it's likely to diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index 616a6d2..2194fa0 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -807,8 +807,8 @@ static unsigned int sata_fsl_dev_classify(struct ata_port *ap) { struct sata_fsl_host_priv *host_priv = ap->host->private_data; void __iomem *hcr_base = host_priv->hcr_base; - struct ata_taskfile tf; u32 temp; + u8 lbah, lbam, lbal;
[PATCH 1/4] libata: consolidate ata_dev_classify()
ata_dev_classify() just uses the 'lbah' and 'lbam' fields from the taskfile, so we can as well use those as arguments and rip out the custom code from sas_ata. Cc: Tejun Heo Cc: Dan Williams Signed-off-by: Hannes Reinecke --- drivers/ata/libahci.c | 11 +++ drivers/ata/libata-core.c | 14 + drivers/ata/libata-sff.c| 2 +- drivers/ata/sata_fsl.c | 11 +++ drivers/ata/sata_inic162x.c | 2 +- drivers/ata/sata_sil24.c| 2 +- drivers/scsi/aic94xx/aic94xx_task.c | 10 +++--- drivers/scsi/isci/request.c | 4 +-- drivers/scsi/libsas/sas_ata.c | 63 + drivers/scsi/mvsas/mv_sas.c | 4 +-- drivers/scsi/pm8001/pm8001_hwi.c| 2 +- drivers/scsi/pm8001/pm80xx_hwi.c| 2 +- include/linux/libata.h | 2 +- include/scsi/libsas.h | 11 ++- 14 files changed, 44 insertions(+), 96 deletions(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index d72ce04..09f9fcb 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1185,16 +1185,15 @@ static void ahci_dev_config(struct ata_device *dev) unsigned int ahci_dev_classify(struct ata_port *ap) { void __iomem *port_mmio = ahci_port_base(ap); - struct ata_taskfile tf; + u8 lbah, lbam, lbal; u32 tmp; tmp = readl(port_mmio + PORT_SIG); - tf.lbah = (tmp >> 24) & 0xff; - tf.lbam = (tmp >> 16) & 0xff; - tf.lbal = (tmp >> 8)& 0xff; - tf.nsect= (tmp) & 0xff; + lbah= (tmp >> 24) & 0xff; + lbam= (tmp >> 16) & 0xff; + lbal= (tmp >> 8)& 0xff; - return ata_dev_classify(&tf); + return ata_dev_classify(lbah, lbam, lbal); } EXPORT_SYMBOL_GPL(ahci_dev_classify); diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 18d97d5..2c4d742 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1032,7 +1032,9 @@ const char *sata_spd_string(unsigned int spd) /** * ata_dev_classify - determine device type based on ATA-spec signature - * @tf: ATA taskfile register set for device to be identified + * @lbah: LBA high byte (LBA bits 23:16) + * @lbam: LBA mid byte (LBA bits 15:8) + * @lbal: LBA low byte (LBA bits 7:0) * * Determine from taskfile register contents whether a device is * ATA or ATAPI, as per "Signature and persistence" section @@ -1045,7 +1047,7 @@ const char *sata_spd_string(unsigned int spd) * Device type, %ATA_DEV_ATA, %ATA_DEV_ATAPI, %ATA_DEV_PMP or * %ATA_DEV_UNKNOWN the event of failure. */ -unsigned int ata_dev_classify(const struct ata_taskfile *tf) +unsigned int ata_dev_classify(u8 lbah, u8 lbam, u8 lbal) { /* Apple's open source Darwin code hints that some devices only * put a proper signature into the LBA mid/high registers, @@ -1068,22 +1070,22 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf) * SEMB signature. This is worked around in * ata_dev_read_id(). */ - if ((tf->lbam == 0) && (tf->lbah == 0)) { + if ((lbam == 0) && (lbah == 0)) { DPRINTK("found ATA device by sig\n"); return ATA_DEV_ATA; } - if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) { + if ((lbam == 0x14) && (lbah == 0xeb)) { DPRINTK("found ATAPI device by sig\n"); return ATA_DEV_ATAPI; } - if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) { + if ((lbam == 0x69) && (lbah == 0x96)) { DPRINTK("found PMP device by sig\n"); return ATA_DEV_PMP; } - if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) { + if ((lbam == 0x3c) && (lbah == 0xc3)) { DPRINTK("found SEMB device by sig (could be ATA device)\n"); return ATA_DEV_SEMB; } diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index 1121153..ec9b5db 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -1903,7 +1903,7 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present, return ATA_DEV_NONE; /* determine if device is ATA or ATAPI */ - class = ata_dev_classify(&tf); + class = ata_dev_classify(tf.lbah, tf.lbam, tf.lbal); if (class == ATA_DEV_UNKNOWN) { /* If the device failed diagnostic, it's likely to diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index 616a6d2..2194fa0 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -807,8 +807,8 @@ static unsigned int sata_fsl_dev_classify(struct ata_port *ap) { struct sata_fsl_host_priv *host_priv = ap->host->private_data; void __iomem *hcr_base = host_priv->hcr_base; - struct ata_taskfile tf; u32 temp; + u8 lbah,
[PATCH 1/4] libata: consolidate ata_dev_classify()
ata_dev_classify() just uses the 'lbah' and 'lbam' fields from the taskfile, so we can as well use those as arguments and rip out the custom code from sas_ata. Cc: Tejun Heo Cc: Dan Williams Signed-off-by: Hannes Reinecke --- drivers/ata/libahci.c | 11 +++ drivers/ata/libata-core.c | 14 + drivers/ata/libata-sff.c| 2 +- drivers/ata/sata_fsl.c | 11 +++ drivers/ata/sata_inic162x.c | 2 +- drivers/ata/sata_sil24.c| 2 +- drivers/scsi/aic94xx/aic94xx_task.c | 10 +++--- drivers/scsi/isci/request.c | 4 +-- drivers/scsi/libsas/sas_ata.c | 63 + drivers/scsi/mvsas/mv_sas.c | 4 +-- drivers/scsi/pm8001/pm8001_hwi.c| 2 +- drivers/scsi/pm8001/pm80xx_hwi.c| 2 +- include/linux/libata.h | 2 +- include/scsi/libsas.h | 11 ++- 14 files changed, 44 insertions(+), 96 deletions(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index d72ce04..09f9fcb 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1185,16 +1185,15 @@ static void ahci_dev_config(struct ata_device *dev) unsigned int ahci_dev_classify(struct ata_port *ap) { void __iomem *port_mmio = ahci_port_base(ap); - struct ata_taskfile tf; + u8 lbah, lbam, lbal; u32 tmp; tmp = readl(port_mmio + PORT_SIG); - tf.lbah = (tmp >> 24) & 0xff; - tf.lbam = (tmp >> 16) & 0xff; - tf.lbal = (tmp >> 8)& 0xff; - tf.nsect= (tmp) & 0xff; + lbah= (tmp >> 24) & 0xff; + lbam= (tmp >> 16) & 0xff; + lbal= (tmp >> 8)& 0xff; - return ata_dev_classify(&tf); + return ata_dev_classify(lbah, lbam, lbal); } EXPORT_SYMBOL_GPL(ahci_dev_classify); diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 18d97d5..2c4d742 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1032,7 +1032,9 @@ const char *sata_spd_string(unsigned int spd) /** * ata_dev_classify - determine device type based on ATA-spec signature - * @tf: ATA taskfile register set for device to be identified + * @lbah: LBA high byte (LBA bits 23:16) + * @lbam: LBA mid byte (LBA bits 15:8) + * @lbal: LBA low byte (LBA bits 7:0) * * Determine from taskfile register contents whether a device is * ATA or ATAPI, as per "Signature and persistence" section @@ -1045,7 +1047,7 @@ const char *sata_spd_string(unsigned int spd) * Device type, %ATA_DEV_ATA, %ATA_DEV_ATAPI, %ATA_DEV_PMP or * %ATA_DEV_UNKNOWN the event of failure. */ -unsigned int ata_dev_classify(const struct ata_taskfile *tf) +unsigned int ata_dev_classify(u8 lbah, u8 lbam, u8 lbal) { /* Apple's open source Darwin code hints that some devices only * put a proper signature into the LBA mid/high registers, @@ -1068,22 +1070,22 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf) * SEMB signature. This is worked around in * ata_dev_read_id(). */ - if ((tf->lbam == 0) && (tf->lbah == 0)) { + if ((lbam == 0) && (lbah == 0)) { DPRINTK("found ATA device by sig\n"); return ATA_DEV_ATA; } - if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) { + if ((lbam == 0x14) && (lbah == 0xeb)) { DPRINTK("found ATAPI device by sig\n"); return ATA_DEV_ATAPI; } - if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) { + if ((lbam == 0x69) && (lbah == 0x96)) { DPRINTK("found PMP device by sig\n"); return ATA_DEV_PMP; } - if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) { + if ((lbam == 0x3c) && (lbah == 0xc3)) { DPRINTK("found SEMB device by sig (could be ATA device)\n"); return ATA_DEV_SEMB; } diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index 1121153..ec9b5db 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -1903,7 +1903,7 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present, return ATA_DEV_NONE; /* determine if device is ATA or ATAPI */ - class = ata_dev_classify(&tf); + class = ata_dev_classify(tf.lbah, tf.lbam, tf.lbal); if (class == ATA_DEV_UNKNOWN) { /* If the device failed diagnostic, it's likely to diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index 616a6d2..2194fa0 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -807,8 +807,8 @@ static unsigned int sata_fsl_dev_classify(struct ata_port *ap) { struct sata_fsl_host_priv *host_priv = ap->host->private_data; void __iomem *hcr_base = host_priv->hcr_base; - struct ata_taskfile tf; u32 temp; + u8 lbah,