[PATCH] libata-zpodd: must use ata_tf_init()

[PATCH] libata-zpodd: must use ata_tf_init()

[Sergei Shtylyov]

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(-)

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;

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Sergei Shtylyov]

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

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Aaron Lu]

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
> 

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Sergei Shtylyov]

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

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Aaron Lu]

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

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Sergei Shtylyov]

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

Re: [PATCH] libata-zpodd: must use ata_tf_init()

[Aaron Lu]

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