| Message ID | 201306232325.04732.sergei.shtylyov@cogentembedded.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 06/23/2013 11:25 PM, Sergei Shtylyov wrote: > There are some SATA controllers which have both devices 0 and 1 but this module > just zeroes out taskfile and sets then ATA_TFLAG_DEVICE (not sure that's needed) > which could lead to a wrong device being selected just before issuing command. > Thus we should call ata_tf_init() which sets up the device register value > properly, like all other users of ata_exec_internal() do... > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Forgot to add Cc: stable@vger.kernel.org... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 23, 2013 at 11:25:04PM +0400, Sergei Shtylyov wrote: > There are some SATA controllers which have both devices 0 and 1 but this module > just zeroes out taskfile and sets then ATA_TFLAG_DEVICE (not sure that's needed) > which could lead to a wrong device being selected just before issuing command. > Thus we should call ata_tf_init() which sets up the device register value > properly, like all other users of ata_exec_internal() do... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Applied to libata/for-3.11 w/ stable cc'd. Thanks.
On 06/24/2013 03:25 AM, Sergei Shtylyov wrote: > There are some SATA controllers which have both devices 0 and 1 but this module Do you mean a SATA port can connect to two SATA devices without PMP? No objections to this patch, it's obviously correct, just want to learn something more :-) Thanks, Aaron > just zeroes out taskfile and sets then ATA_TFLAG_DEVICE (not sure that's needed) > which could lead to a wrong device being selected just before issuing command. > Thus we should call ata_tf_init() which sets up the device register value > properly, like all other users of ata_exec_internal() do... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > The patch is against the 'for-3.10-fixes' branch of Tejun Heo's 'libata.git' > repository. > > drivers/ata/libata-zpodd.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > Index: libata/drivers/ata/libata-zpodd.c > =================================================================== > --- libata.orig/drivers/ata/libata-zpodd.c > +++ libata/drivers/ata/libata-zpodd.c > @@ -32,13 +32,14 @@ struct zpodd { > > static int eject_tray(struct ata_device *dev) > { > - struct ata_taskfile tf = {}; > + struct ata_taskfile tf; > const char cdb[] = { GPCMD_START_STOP_UNIT, > 0, 0, 0, > 0x02, /* LoEj */ > 0, 0, 0, 0, 0, 0, 0, > }; > > + ata_tf_init(dev, &tf); > tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; > tf.command = ATA_CMD_PACKET; > tf.protocol = ATAPI_PROT_NODATA; > @@ -52,8 +53,7 @@ static enum odd_mech_type zpodd_get_mech > char buf[16]; > unsigned int ret; > struct rm_feature_desc *desc = (void *)(buf + 8); > - struct ata_taskfile tf = {}; > - > + struct ata_taskfile tf; > char cdb[] = { GPCMD_GET_CONFIGURATION, > 2, /* only 1 feature descriptor requested */ > 0, 3, /* 3, removable medium feature */ > @@ -62,6 +62,7 @@ static enum odd_mech_type zpodd_get_mech > 0, 0, 0, > }; > > + ata_tf_init(dev, &tf); > tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; > tf.command = ATA_CMD_PACKET; > tf.protocol = ATAPI_PROT_PIO; > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 26-06-2013 6:05, Aaron Lu wrote: >> There are some SATA controllers which have both devices 0 and 1 but this module > Do you mean a SATA port can connect to two SATA devices without PMP? No, I mean 2 SATA ports combined in one ATA channel as 2 devices. Look for SLAVE_POSS in drivers/ata/[s]ata*.c... > Thanks, > Aaron MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/2013 07:10 PM, Sergei Shtylyov wrote: > Hello. > > On 26-06-2013 6:05, Aaron Lu wrote: > >>> There are some SATA controllers which have both devices 0 and 1 but this module > >> Do you mean a SATA port can connect to two SATA devices without PMP? > > No, I mean 2 SATA ports combined in one ATA channel as 2 devices. > Look for SLAVE_POSS in drivers/ata/[s]ata*.c... Oh yes that happened when the SATA controller is in IDE programming mode, not AHCI. ZPODD is only supported for SATA controllers in AHCI programming mode, the ACPI table will not support power off the port if the controller is in IDE mode, so the device 1 case shouldn't happen in practice :-) Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 27-06-2013 5:15, Aaron Lu wrote: >>>> There are some SATA controllers which have both devices 0 and 1 but this module >>> Do you mean a SATA port can connect to two SATA devices without PMP? >> No, I mean 2 SATA ports combined in one ATA channel as 2 devices. >> Look for SLAVE_POSS in drivers/ata/[s]ata*.c... > Oh yes that happened when the SATA controller is in IDE programming > mode, not AHCI. AHCI is not the only mode for the SATA controllers. > ZPODD is only supported for SATA controllers in AHCI programming mode, Hm, really? How about the true SATA controllers that don't use AHCI? > the ACPI table will not support power off the port if the controller is > in IDE mode, so the device 1 case shouldn't happen in practice :-) Anyway, I need uniform setup for ata_exec_internal() call for my to be posted cleanup patches. > Thanks, > Aaron WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/28/2013 08:49 PM, Sergei Shtylyov wrote: > Hello. > > On 27-06-2013 5:15, Aaron Lu wrote: > >>>>> There are some SATA controllers which have both devices 0 and 1 but this module > >>>> Do you mean a SATA port can connect to two SATA devices without PMP? > >>> No, I mean 2 SATA ports combined in one ATA channel as 2 devices. >>> Look for SLAVE_POSS in drivers/ata/[s]ata*.c... > >> Oh yes that happened when the SATA controller is in IDE programming >> mode, not AHCI. > > AHCI is not the only mode for the SATA controllers. Right. > >> ZPODD is only supported for SATA controllers in AHCI programming mode, > > Hm, really? How about the true SATA controllers that don't use AHCI? If they don't set ATA_FLAG_ACPI_SATA, no. > >> the ACPI table will not support power off the port if the controller is >> in IDE mode, so the device 1 case shouldn't happen in practice :-) > > Anyway, I need uniform setup for ata_exec_internal() call for my to > be posted cleanup patches. No problem. Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: libata/drivers/ata/libata-zpodd.c =================================================================== --- libata.orig/drivers/ata/libata-zpodd.c +++ libata/drivers/ata/libata-zpodd.c @@ -32,13 +32,14 @@ struct zpodd { static int eject_tray(struct ata_device *dev) { - struct ata_taskfile tf = {}; + struct ata_taskfile tf; const char cdb[] = { GPCMD_START_STOP_UNIT, 0, 0, 0, 0x02, /* LoEj */ 0, 0, 0, 0, 0, 0, 0, }; + ata_tf_init(dev, &tf); tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; tf.command = ATA_CMD_PACKET; tf.protocol = ATAPI_PROT_NODATA; @@ -52,8 +53,7 @@ static enum odd_mech_type zpodd_get_mech char buf[16]; unsigned int ret; struct rm_feature_desc *desc = (void *)(buf + 8); - struct ata_taskfile tf = {}; - + struct ata_taskfile tf; char cdb[] = { GPCMD_GET_CONFIGURATION, 2, /* only 1 feature descriptor requested */ 0, 3, /* 3, removable medium feature */ @@ -62,6 +62,7 @@ static enum odd_mech_type zpodd_get_mech 0, 0, 0, }; + ata_tf_init(dev, &tf); tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; tf.command = ATA_CMD_PACKET; tf.protocol = ATAPI_PROT_PIO;
There are some SATA controllers which have both devices 0 and 1 but this module just zeroes out taskfile and sets then ATA_TFLAG_DEVICE (not sure that's needed) which could lead to a wrong device being selected just before issuing command. Thus we should call ata_tf_init() which sets up the device register value properly, like all other users of ata_exec_internal() do... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- The patch is against the 'for-3.10-fixes' branch of Tejun Heo's 'libata.git' repository. drivers/ata/libata-zpodd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html