Patchwork ipr: qc_fill_rtf() method should not store alternate status register

login
register
mail settings
Submitter Sergei Shtylyov
Date May 21, 2013, 7:24 p.m.
Message ID <201305212324.22822.sergei.shtylyov@cogentembedded.com>
Download mbox | patch
Permalink /patch/245394/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Sergei Shtylyov - May 21, 2013, 7:24 p.m.
The 'ctl' field of  the 'struct ata_taskfile' is not really dual purpose, i.e.
it is not intended  for storing the alternate status register (which is mapped
at the same address in the legacy IDE controllers) in the qc_fill_rtf() method.
No other 'libata' driver except 'drivers/scsi/ipr.c' stores the alternate status
register's value in the 'ctl' field of 'qc->result_tf', hence this driver should
not do this as well...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against the recent Linus' tree.

 drivers/scsi/ipr.c |    1 -
 1 file changed, 1 deletion(-)

--
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
Brian King - May 31, 2013, 12:46 p.m.
On 05/21/2013 02:24 PM, Sergei Shtylyov wrote:
> The 'ctl' field of  the 'struct ata_taskfile' is not really dual purpose, i.e.
> it is not intended  for storing the alternate status register (which is mapped
> at the same address in the legacy IDE controllers) in the qc_fill_rtf() method.
> No other 'libata' driver except 'drivers/scsi/ipr.c' stores the alternate status
> register's value in the 'ctl' field of 'qc->result_tf', hence this driver should
> not do this as well...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Acked-by: Brian King <brking@linux.vnet.ibm.com>
Sergei Shtylyov - May 31, 2013, 10:37 p.m.
Hello.

On 05/31/2013 04:46 PM, Brian King wrote:

>
>> The 'ctl' field of  the 'struct ata_taskfile' is not really dual purpose, i.e.
>> it is not intended  for storing the alternate status register (which is mapped
>> at the same address in the legacy IDE controllers) in the qc_fill_rtf() method.
>> No other 'libata' driver except 'drivers/scsi/ipr.c' stores the alternate status
>> register's value in the 'ctl' field of 'qc->result_tf', hence this driver should
>> not do this as well...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Acked-by: Brian King <brking@linux.vnet.ibm.com>
>

     Tejun, I would prefer that this patch be merged thru the libata tree
as my future cleanup patches depend on it.

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
Tejun Heo - June 2, 2013, 8:15 a.m.
On Sat, Jun 01, 2013 at 02:37:00AM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/31/2013 04:46 PM, Brian King wrote:
> 
> >
> >>The 'ctl' field of  the 'struct ata_taskfile' is not really dual purpose, i.e.
> >>it is not intended  for storing the alternate status register (which is mapped
> >>at the same address in the legacy IDE controllers) in the qc_fill_rtf() method.
> >>No other 'libata' driver except 'drivers/scsi/ipr.c' stores the alternate status
> >>register's value in the 'ctl' field of 'qc->result_tf', hence this driver should
> >>not do this as well...
> >>
> >>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >Acked-by: Brian King <brking@linux.vnet.ibm.com>
> >
> 
>     Tejun, I would prefer that this patch be merged thru the libata tree
> as my future cleanup patches depend on it.

Applied to libata/for-3.11 w/ Brian's ack added.

Thanks.

Patch

Index: linux/drivers/scsi/ipr.c
===================================================================
--- linux.orig/drivers/scsi/ipr.c
+++ linux/drivers/scsi/ipr.c
@@ -6662,7 +6662,6 @@  static bool ipr_qc_fill_rtf(struct ata_q
 	tf->hob_lbal = g->hob_lbal;
 	tf->hob_lbam = g->hob_lbam;
 	tf->hob_lbah = g->hob_lbah;
-	tf->ctl = g->alt_status;
 
 	return true;
 }