Patchwork libata-zpodd: must use ata_tf_init()

login
register
mail settings
Submitter Sergei Shtylyov
Date June 23, 2013, 7:25 p.m.
Message ID <201306232325.04732.sergei.shtylyov@cogentembedded.com>
Download mbox | patch
Permalink /patch/253591/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Sergei Shtylyov - June 23, 2013, 7:25 p.m.
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
Sergei Shtylyov - June 23, 2013, 7:34 p.m.
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
Tejun Heo - June 24, 2013, 10:46 p.m.
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.
Aaron Lu - June 26, 2013, 2:05 a.m.
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
Sergei Shtylyov - June 26, 2013, 11:10 a.m.
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
Aaron Lu - June 27, 2013, 1:15 a.m.
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
Sergei Shtylyov - June 28, 2013, 12:49 p.m.
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
Aaron Lu - June 29, 2013, 1:04 p.m.
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

Patch

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;