[PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[Prabhakar Kushwaha]

When a single device error is detected, the device under the error is indicated
by the error bit set in the DER. There is a one to one mapping between register
bit and devices on Port multiplier(PMP) i.e. bit 0 represents PMP device 0 and
bit 1 represents PMP device 1 etc.

Current implementation treats Device error register value as device number not
set of bits representing multiple device on PMP. It is changed to consider bit
level.
No need to check for each set bit as all command is going to be aborted.

Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
Signed-off-by: Ashish Kalra <B00888@freescale.com>
---
 git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git (branch master)

 Changes for v1: Incorporated David Laight's comment
 	- Single usage of ffs()

 Changes for v2: Incorporated David Laight's comment
 	- Changed type of dev_num to unsigned

 drivers/ata/sata_fsl.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index b0214d0..895771c 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1040,12 +1040,14 @@ static void sata_fsl_error_intr(struct ata_port *ap)
 
 		/* find out the offending link and qc */
 		if (ap->nr_pmp_links) {
+			unsigned int dev_num;
 			dereg = ioread32(hcr_base + DE);
 			iowrite32(dereg, hcr_base + DE);
 			iowrite32(cereg, hcr_base + CE);
 
-			if (dereg < ap->nr_pmp_links) {
-				link = &ap->pmp_link[dereg];
+			dev_num = ffs(dereg)-1;
+			if (dev_num < ap->nr_pmp_links) {
+				link = &ap->pmp_link[dev_num];
 				ehi = &link->eh_info;
 				qc = ata_qc_from_tag(ap, link->active_tag);
 				/*
-- 
1.7.3

Re: [PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[Benjamin Herrenschmidt]

On Mon, 2011-02-21 at 15:27 +0530, Prabhakar Kushwaha wrote:
> When a single device error is detected, the device under the error is indicated
> by the error bit set in the DER. There is a one to one mapping between register
> bit and devices on Port multiplier(PMP) i.e. bit 0 represents PMP device 0 and
> bit 1 represents PMP device 1 etc.

It might help to send those patches to the linux-ide mailing list and
appropriate libata maintainers in addition to CC'ing linuxppc-dev.

Cheers,
Ben.

> Current implementation treats Device error register value as device number not
> set of bits representing multiple device on PMP. It is changed to consider bit
> level.
> No need to check for each set bit as all command is going to be aborted.
> 
> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
> Signed-off-by: Ashish Kalra <B00888@freescale.com>
> ---
>  git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git (branch master)
> 
>  Changes for v1: Incorporated David Laight's comment
>  	- Single usage of ffs()
> 
>  Changes for v2: Incorporated David Laight's comment
>  	- Changed type of dev_num to unsigned
> 
>  drivers/ata/sata_fsl.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index b0214d0..895771c 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1040,12 +1040,14 @@ static void sata_fsl_error_intr(struct ata_port *ap)
>  
>  		/* find out the offending link and qc */
>  		if (ap->nr_pmp_links) {
> +			unsigned int dev_num;
>  			dereg = ioread32(hcr_base + DE);
>  			iowrite32(dereg, hcr_base + DE);
>  			iowrite32(cereg, hcr_base + CE);
>  
> -			if (dereg < ap->nr_pmp_links) {
> -				link = &ap->pmp_link[dereg];
> +			dev_num = ffs(dereg)-1;
> +			if (dev_num < ap->nr_pmp_links) {
> +				link = &ap->pmp_link[dev_num];
>  				ehi = &link->eh_info;
>  				qc = ata_qc_from_tag(ap, link->active_tag);
>  				/*

RE: [PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[Kushwaha Prabhakar-B32579]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogV2VkbmVz
ZGF5LCBNYXJjaCAwMiwgMjAxMSA4OjU2IEFNDQo+IFRvOiBLdXNod2FoYSBQcmFiaGFrYXItQjMy
NTc5DQo+IENjOiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgbWVldDJwcmFiaHVAZ21h
aWwuY29tOyBLYWxyYSBBc2hpc2gtDQo+IEIwMDg4OA0KPiBTdWJqZWN0OiBSZTogW1BBVENIXVt2
Ml0gZHJpdmVyL0ZTTCBTQVRBOkZpeCB3cm9uZyBEZXZpY2UgRXJyb3IgUmVnaXN0ZXINCj4gdXNh
Z2UNCj4gDQo+IE9uIE1vbiwgMjAxMS0wMi0yMSBhdCAxNToyNyArMDUzMCwgUHJhYmhha2FyIEt1
c2h3YWhhIHdyb3RlOg0KPiA+IFdoZW4gYSBzaW5nbGUgZGV2aWNlIGVycm9yIGlzIGRldGVjdGVk
LCB0aGUgZGV2aWNlIHVuZGVyIHRoZSBlcnJvciBpcw0KPiA+IGluZGljYXRlZCBieSB0aGUgZXJy
b3IgYml0IHNldCBpbiB0aGUgREVSLiBUaGVyZSBpcyBhIG9uZSB0byBvbmUNCj4gPiBtYXBwaW5n
IGJldHdlZW4gcmVnaXN0ZXIgYml0IGFuZCBkZXZpY2VzIG9uIFBvcnQgbXVsdGlwbGllcihQTVAp
IGkuZS4NCj4gPiBiaXQgMCByZXByZXNlbnRzIFBNUCBkZXZpY2UgMCBhbmQgYml0IDEgcmVwcmVz
ZW50cyBQTVAgZGV2aWNlIDEgZXRjLg0KPiANCj4gSXQgbWlnaHQgaGVscCB0byBzZW5kIHRob3Nl
IHBhdGNoZXMgdG8gdGhlIGxpbnV4LWlkZSBtYWlsaW5nIGxpc3QgYW5kDQo+IGFwcHJvcHJpYXRl
IGxpYmF0YSBtYWludGFpbmVycyBpbiBhZGRpdGlvbiB0byBDQydpbmcgbGludXhwcGMtZGV2Lg0K
PiANClRoYW5rcyBCZW5qYW1pbiEhDQpJIHdpbGwgdGFrZSBjYXJlIHlvdXIgcG9pbnQgaW4gbmVh
ciBmdXR1cmUuDQoNCi0tUHJhYmhha2FyDQoNCj4gPiBDdXJyZW50IGltcGxlbWVudGF0aW9uIHRy
ZWF0cyBEZXZpY2UgZXJyb3IgcmVnaXN0ZXIgdmFsdWUgYXMgZGV2aWNlDQo+ID4gbnVtYmVyIG5v
dCBzZXQgb2YgYml0cyByZXByZXNlbnRpbmcgbXVsdGlwbGUgZGV2aWNlIG9uIFBNUC4gSXQgaXMN
Cj4gPiBjaGFuZ2VkIHRvIGNvbnNpZGVyIGJpdCBsZXZlbC4NCj4gPiBObyBuZWVkIHRvIGNoZWNr
IGZvciBlYWNoIHNldCBiaXQgYXMgYWxsIGNvbW1hbmQgaXMgZ29pbmcgdG8gYmUNCj4gYWJvcnRl
ZC4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFByYWJoYWthciBLdXNod2FoYSA8cHJhYmhha2Fy
QGZyZWVzY2FsZS5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogQXNoaXNoIEthbHJhIDxCMDA4ODhA
ZnJlZXNjYWxlLmNvbT4NCj4gPiAtLS0NCj4gPiAgZ2l0Oi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3Nj
bS9saW51eC9rZXJuZWwvZ2l0L3RvcnZhbGRzL2xpbnV4LTIuNi5naXQNCj4gPiAoYnJhbmNoIG1h
c3RlcikNCj4gPg0KPiA+ICBDaGFuZ2VzIGZvciB2MTogSW5jb3Jwb3JhdGVkIERhdmlkIExhaWdo
dCdzIGNvbW1lbnQNCj4gPiAgCS0gU2luZ2xlIHVzYWdlIG9mIGZmcygpDQo+ID4NCj4gPiAgQ2hh
bmdlcyBmb3IgdjI6IEluY29ycG9yYXRlZCBEYXZpZCBMYWlnaHQncyBjb21tZW50DQo+ID4gIAkt
IENoYW5nZWQgdHlwZSBvZiBkZXZfbnVtIHRvIHVuc2lnbmVkDQo+ID4NCj4gPiAgZHJpdmVycy9h
dGEvc2F0YV9mc2wuYyB8ICAgIDYgKysrKy0tDQo+ID4gIDEgZmlsZXMgY2hhbmdlZCwgNCBpbnNl
cnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMv
YXRhL3NhdGFfZnNsLmMgYi9kcml2ZXJzL2F0YS9zYXRhX2ZzbC5jIGluZGV4DQo+ID4gYjAyMTRk
MC4uODk1NzcxYyAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2F0YS9zYXRhX2ZzbC5jDQo+ID4g
KysrIGIvZHJpdmVycy9hdGEvc2F0YV9mc2wuYw0KPiA+IEBAIC0xMDQwLDEyICsxMDQwLDE0IEBA
IHN0YXRpYyB2b2lkIHNhdGFfZnNsX2Vycm9yX2ludHIoc3RydWN0DQo+ID4gYXRhX3BvcnQgKmFw
KQ0KPiA+DQo+ID4gIAkJLyogZmluZCBvdXQgdGhlIG9mZmVuZGluZyBsaW5rIGFuZCBxYyAqLw0K
PiA+ICAJCWlmIChhcC0+bnJfcG1wX2xpbmtzKSB7DQo+ID4gKwkJCXVuc2lnbmVkIGludCBkZXZf
bnVtOw0KPiA+ICAJCQlkZXJlZyA9IGlvcmVhZDMyKGhjcl9iYXNlICsgREUpOw0KPiA+ICAJCQlp
b3dyaXRlMzIoZGVyZWcsIGhjcl9iYXNlICsgREUpOw0KPiA+ICAJCQlpb3dyaXRlMzIoY2VyZWcs
IGhjcl9iYXNlICsgQ0UpOw0KPiA+DQo+ID4gLQkJCWlmIChkZXJlZyA8IGFwLT5ucl9wbXBfbGlu
a3MpIHsNCj4gPiAtCQkJCWxpbmsgPSAmYXAtPnBtcF9saW5rW2RlcmVnXTsNCj4gPiArCQkJZGV2
X251bSA9IGZmcyhkZXJlZyktMTsNCj4gPiArCQkJaWYgKGRldl9udW0gPCBhcC0+bnJfcG1wX2xp
bmtzKSB7DQo+ID4gKwkJCQlsaW5rID0gJmFwLT5wbXBfbGlua1tkZXZfbnVtXTsNCj4gPiAgCQkJ
CWVoaSA9ICZsaW5rLT5laF9pbmZvOw0KPiA+ICAJCQkJcWMgPSBhdGFfcWNfZnJvbV90YWcoYXAs
IGxpbmstPmFjdGl2ZV90YWcpOw0KPiA+ICAJCQkJLyoNCj4gDQo+IA0KDQo=

RE: [PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[David Laight]

=20
>  Case when ffs return will never arise.This scenario is=20
> already been discussed
>  on linuxppc-dev@lists.ozlabs.org. Please see below explanation:
>  sata_fsl_error_intr() is called during device error.The=20
> mentioned scenario
>  will never comes. It can be observed via code:-
>         if (cereg) {   --> cereg is set on command error.=20
> Means there is at least 1 device present.

This all relies on the hardware working properly, and
nothing else accidentally clearing the device 'error'
bit mask.

Being slightly defensive when the out of range value
causes an array subscript error seems sensible.

It isn't as though the code size or execution time
matters here.

	David

[PATCH][v2] driver/FSL SATA:Fix wrong Device Error Register usage

[Prabhakar Kushwaha]

When a single device error is detected, the device under the error is indicated
by the error bit set in the DER. There is a one to one mapping between register
bit and devices on Port multiplier(PMP) i.e. bit 0 represents PMP device 0 and
bit 1 represents PMP device 1 etc.

Current implementation treats Device error register value as device number not
set of bits representing multiple device on PMP. It is changed to consider bit
level.
No need to check for each set bit as all command is going to be aborted.

Signed-off-by: Ashish Kalra <B00888@freescale.com>
Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
---

 git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git (branch master)

 This patch is already gone through review of linuxppc-dev mail list.
 Making CC linuxppc-dev@lists.ozlabs.org

 Changes for v2: Incorporated Sergei Shtylyov's comment
	- Put space after -

 Case when ffs return will never arise.This scenario is already been discussed
 on linuxppc-dev@lists.ozlabs.org. Please see below explanation:
 sata_fsl_error_intr() is called during device error.The mentioned scenario
 will never comes. It can be observed via code:-
        if (cereg) {   --> cereg is set on command error. Means there is at least 1 device present.
                abort = 1;
                ---
                ---
                ---
                /* find out the offending link and qc */
                if (ap->nr_pmp_links) {  --> if Port multiplier
                        ---
                        ---
                        if ((ffs(dereg)-1) < ap->nr_pmp_links) {
                ---
                ---
                } else {  -->  Single device
                        dereg = ioread32(hcr_base + DE);
                        iowrite32(dereg, hcr_base + DE);
                        iowrite32(cereg, hcr_base + CE);

 drivers/ata/sata_fsl.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index b0214d0..d71e7c2 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1040,12 +1040,15 @@ static void sata_fsl_error_intr(struct ata_port *ap)
 
 		/* find out the offending link and qc */
 		if (ap->nr_pmp_links) {
+			unsigned int dev_num;
+
 			dereg = ioread32(hcr_base + DE);
 			iowrite32(dereg, hcr_base + DE);
 			iowrite32(cereg, hcr_base + CE);
 
-			if (dereg < ap->nr_pmp_links) {
-				link = &ap->pmp_link[dereg];
+			dev_num = ffs(dereg) - 1;
+			if (dev_num < ap->nr_pmp_links) {
+				link = &ap->pmp_link[dev_num];
 				ehi = &link->eh_info;
 				qc = ata_qc_from_tag(ap, link->active_tag);
 				/*
-- 
1.7.3