RE: [PATCH 6/7] sata_mv: endian fix

RE: [PATCH 6/7] sata_mv: endian fix

[Zang Roy-r61911]

> Zang Roy-r61911 wrote:
> >> This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
> >> on the powerpc platform.  His original patch also had some other
> >> platform-specific changes in #ifdef's, but I'm not sure yet how to
> >> incorporate them.  Look for another patch for those (soon).
> >>
> >> Signed-off-by: Mark Lord <liml@rtr.ca>
> >>
> >> ---
> >> --- linux/drivers/scsi/sata_mv.c	2006-05-19 
> >> 15:44:11.000000000 -0400
> >> +++ linux/drivers/scsi/sata_mv.c	2006-05-19 
> >> 15:58:21.000000000 -0400
> >> @@ -1030,8 +1030,9 @@
> >>  
> >>  static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 
> >> addr, unsigned last)
> >>  {
> >> -	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
> >> +	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
> >>  		(last ? CRQB_CMD_LAST : 0);
> >> +	*cmdw = cpu_to_le16(tmp);
> >>  }
> >>  
> >>  /**
> >>
> > 
> > 
> > Does this patch will affect other platform except powerpc?
> 
> This patch affects all platforms (as it should).  Most Linux code is 
> intentionally written in a cross-platform, portable manner 
> without ifdefs.
> 
> 	Jeff
> 
> 
> 
I can see it work on powerpc platform, while does it work for other 
platform such as i386?
Roy

Re: [PATCH 6/7] sata_mv: endian fix

[Jeff Garzik]

Zang Roy-r61911 wrote:
>> Zang Roy-r61911 wrote:
>>>> This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
>>>> on the powerpc platform.  His original patch also had some other
>>>> platform-specific changes in #ifdef's, but I'm not sure yet how to
>>>> incorporate them.  Look for another patch for those (soon).
>>>>
>>>> Signed-off-by: Mark Lord <liml@rtr.ca>
>>>>
>>>> ---
>>>> --- linux/drivers/scsi/sata_mv.c	2006-05-19 
>>>> 15:44:11.000000000 -0400
>>>> +++ linux/drivers/scsi/sata_mv.c	2006-05-19 
>>>> 15:58:21.000000000 -0400
>>>> @@ -1030,8 +1030,9 @@
>>>>  
>>>>  static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 
>>>> addr, unsigned last)
>>>>  {
>>>> -	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
>>>> +	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
>>>>  		(last ? CRQB_CMD_LAST : 0);
>>>> +	*cmdw = cpu_to_le16(tmp);
>>>>  }
>>>>  
>>>>  /**
>>>>
>>>
>>> Does this patch will affect other platform except powerpc?
>> This patch affects all platforms (as it should).  Most Linux code is 
>> intentionally written in a cross-platform, portable manner 
>> without ifdefs.
>>
>> 	Jeff
>>
>>
>>
> I can see it work on powerpc platform, while does it work for other 
> platform such as i386?

Yes.  Look at the definition of the function in question:  for little 
endian platforms, it is a simple assignment.  For big endian platforms, 
it does the expected byte swap.

This is also the standard for Linux code:  one function Does The Right 
Thing, regardless of platform.

	Jeff

RE: [PATCH 6/7] sata_mv: endian fix

[Zang Roy-r61911]

 
> Zang Roy-r61911 wrote:
> >> Zang Roy-r61911 wrote:
> >>>> This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
> >>>> on the powerpc platform.  His original patch also had some other
> >>>> platform-specific changes in #ifdef's, but I'm not sure 
> yet how to
> >>>> incorporate them.  Look for another patch for those (soon).
> >>>>
> >>>> Signed-off-by: Mark Lord <liml@rtr.ca>
> >>>>
> >>>> ---
> >>>> --- linux/drivers/scsi/sata_mv.c	2006-05-19 
> >>>> 15:44:11.000000000 -0400
> >>>> +++ linux/drivers/scsi/sata_mv.c	2006-05-19 
> >>>> 15:58:21.000000000 -0400
> >>>> @@ -1030,8 +1030,9 @@
> >>>>  
> >>>>  static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 
> >>>> addr, unsigned last)
> >>>>  {
> >>>> -	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | 
> CRQB_CMD_CS |
> >>>> +	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) 
> | CRQB_CMD_CS |
> >>>>  		(last ? CRQB_CMD_LAST : 0);
> >>>> +	*cmdw = cpu_to_le16(tmp);
> >>>>  }
> >>>>  
> >>>>  /**
> >>>>
> >>>
> >>> Does this patch will affect other platform except powerpc?
> >> This patch affects all platforms (as it should).  Most 
> Linux code is 
> >> intentionally written in a cross-platform, portable manner 
> >> without ifdefs.
> >>
> >> 	Jeff
> >>
> >>
> >>
> > I can see it work on powerpc platform, while does it work for other 
> > platform such as i386?
> 
> Yes.  Look at the definition of the function in question:  for little 
> endian platforms, it is a simple assignment.  For big endian 
> platforms, 
> it does the expected byte swap.
> 
> This is also the standard for Linux code:  one function Does 
> The Right 
> Thing, regardless of platform.
> 
> 	Jeff
> 
> 
> 
I see it !
Roy

RE: [PATCH 6/7] sata_mv: endian fix

[Zang Roy-r61911]

> This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
> on the powerpc platform.  His original patch also had some other
> platform-specific changes in #ifdef's, but I'm not sure yet how to
> incorporate them.  Look for another patch for those (soon).
> 
> Signed-off-by: Mark Lord <liml@rtr.ca>
> 
> ---
> --- linux/drivers/scsi/sata_mv.c	2006-05-19 
> 15:44:11.000000000 -0400
> +++ linux/drivers/scsi/sata_mv.c	2006-05-19 
> 15:58:21.000000000 -0400
> @@ -1030,8 +1030,9 @@
>  
>  static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 
> addr, unsigned last)
>  {
> -	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
> +	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
>  		(last ? CRQB_CMD_LAST : 0);
> +	*cmdw = cpu_to_le16(tmp);
>  }
>  
>  /**
> 


Does this patch will affect other platform except powerpc?

Roy

Re: [PATCH 6/7] sata_mv: endian fix

[Jeff Garzik]

Zang Roy-r61911 wrote:
>> This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
>> on the powerpc platform.  His original patch also had some other
>> platform-specific changes in #ifdef's, but I'm not sure yet how to
>> incorporate them.  Look for another patch for those (soon).
>>
>> Signed-off-by: Mark Lord <liml@rtr.ca>
>>
>> ---
>> --- linux/drivers/scsi/sata_mv.c	2006-05-19 
>> 15:44:11.000000000 -0400
>> +++ linux/drivers/scsi/sata_mv.c	2006-05-19 
>> 15:58:21.000000000 -0400
>> @@ -1030,8 +1030,9 @@
>>  
>>  static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 
>> addr, unsigned last)
>>  {
>> -	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
>> +	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
>>  		(last ? CRQB_CMD_LAST : 0);
>> +	*cmdw = cpu_to_le16(tmp);
>>  }
>>  
>>  /**
>>
> 
> 
> Does this patch will affect other platform except powerpc?

This patch affects all platforms (as it should).  Most Linux code is 
intentionally written in a cross-platform, portable manner without ifdefs.

	Jeff

[PATCH 13/13] libata-hp: move ata_do_reset() to libata-eh.c

[Tejun Heo]

With ops->probe_init() gone, no user is left in libata-core.c.  Move
ata_do_reset() to libata-eh.c and make it static.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   28 ----------------------------
 drivers/scsi/libata-eh.c   |   28 ++++++++++++++++++++++++++++
 drivers/scsi/libata.h      |    2 --
 3 files changed, 28 insertions(+), 30 deletions(-)

8e2c3e2ccff15e2d111a3b3f21acd9f864672b4e
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 6b9586c..350df1d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2783,34 +2783,6 @@ void ata_std_postreset(struct ata_port *
 	DPRINTK("EXIT\n");
 }
 
-int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
-		 unsigned int *classes)
-{
-	int i, rc;
-
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		classes[i] = ATA_DEV_UNKNOWN;
-
-	rc = reset(ap, classes);
-	if (rc)
-		return rc;
-
-	/* If any class isn't ATA_DEV_UNKNOWN, consider classification
-	 * is complete and convert all ATA_DEV_UNKNOWN to
-	 * ATA_DEV_NONE.
-	 */
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		if (classes[i] != ATA_DEV_UNKNOWN)
-			break;
-
-	if (i < ATA_MAX_DEVICES)
-		for (i = 0; i < ATA_MAX_DEVICES; i++)
-			if (classes[i] == ATA_DEV_UNKNOWN)
-				classes[i] = ATA_DEV_NONE;
-
-	return 0;
-}
-
 /**
  *	ata_dev_same_device - Determine whether new ID matches configured device
  *	@dev: device to compare against
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index 4357465..b1ac851 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -1317,6 +1317,34 @@ static void ata_eh_report(struct ata_por
 	}
 }
 
+static int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
+			unsigned int *classes)
+{
+	int i, rc;
+
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		classes[i] = ATA_DEV_UNKNOWN;
+
+	rc = reset(ap, classes);
+	if (rc)
+		return rc;
+
+	/* If any class isn't ATA_DEV_UNKNOWN, consider classification
+	 * is complete and convert all ATA_DEV_UNKNOWN to
+	 * ATA_DEV_NONE.
+	 */
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		if (classes[i] != ATA_DEV_UNKNOWN)
+			break;
+
+	if (i < ATA_MAX_DEVICES)
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (classes[i] == ATA_DEV_UNKNOWN)
+				classes[i] = ATA_DEV_NONE;
+
+	return 0;
+}
+
 static int ata_eh_followup_srst_needed(int rc, int classify,
 				       const unsigned int *classes)
 {
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 0176c40..a954500 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -57,8 +57,6 @@ extern int sata_down_spd_limit(struct at
 extern int sata_set_spd_needed(struct ata_port *ap);
 extern int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0);
 extern int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
-extern int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
-			unsigned int *classes);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
-- 
1.3.2

Re: [PATCH 13/13] libata-hp: move ata_do_reset() to libata-eh.c

[Jeff Garzik]

Tejun Heo wrote:
> With ops->probe_init() gone, no user is left in libata-core.c.  Move
> ata_do_reset() to libata-eh.c and make it static.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 8-13

[PATCH 0/7] sata_mv: assorted fixes

[Mark Lord]

The following seven patches contain various fixes and cleanups for
the libata Marvell SATA driver sata_mv.c taken from my internal tree.

These fixes have reportedly solved many problems for a number of
different users.  The driver is still HIGHLY EXPERIMENTAL, at least until
more people exercise this latest driver and report on success/issues.

Cheers

Mark

[PATCH 6/7] sata_mv: endian fix

[Mark Lord]

This fixes a byte-swap issue on PPC, found by Zang Roy-r61911
on the powerpc platform.  His original patch also had some other
platform-specific changes in #ifdef's, but I'm not sure yet how to
incorporate them.  Look for another patch for those (soon).

Signed-off-by: Mark Lord <liml@rtr.ca>

---
--- linux/drivers/scsi/sata_mv.c	2006-05-19 15:44:11.000000000 -0400
+++ linux/drivers/scsi/sata_mv.c	2006-05-19 15:58:21.000000000 -0400
@@ -1030,8 +1030,9 @@
 
 static inline void mv_crqb_pack_cmd(u16 *cmdw, u8 data, u8 addr, unsigned last)
 {
-	*cmdw = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
+	u16 tmp = data | (addr << CRQB_CMD_ADDR_SHIFT) | CRQB_CMD_CS |
 		(last ? CRQB_CMD_LAST : 0);
+	*cmdw = cpu_to_le16(tmp);
 }
 
 /**