[PATCH] ata: propagate return value of scc_wait_after_reset

[PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

scc_bus_softreset not necessarily should return zero.
Propagate the error code.
Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 drivers/ata/pata_scc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index 4e006d7..5618c4518 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
  *	Note: Original code is ata_bus_softreset().
  */
 
-static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
+static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
                                       unsigned long deadline)
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
@@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
 	udelay(20);
 	out_be32(ioaddr->ctl_addr, ap->ctl);
 
-	scc_wait_after_reset(&ap->link, devmask, deadline);
-
-	return 0;
+	return scc_wait_after_reset(&ap->link, devmask, deadline);
 }
 
 /**
@@ -618,7 +616,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
 {
 	struct ata_port *ap = link->ap;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
-	unsigned int devmask = 0, err_mask;
+	unsigned int devmask = 0;
+	int err_mask;
 	u8 err;
 
 	DPRINTK("ENTER\n");
-- 
1.7.11.7

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

Hello,

On Sat, Aug 16, 2014 at 03:47:17PM +0530, Arjun Sreedharan wrote:
> scc_bus_softreset not necessarily should return zero.
> Propagate the error code.
> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
> ---
>  drivers/ata/pata_scc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
> index 4e006d7..5618c4518 100644
> --- a/drivers/ata/pata_scc.c
> +++ b/drivers/ata/pata_scc.c
> @@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
>   *	Note: Original code is ata_bus_softreset().
>   */
>  
> -static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
> +static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>                                        unsigned long deadline)
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
> @@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,

scc_bus_softreset() should return int too, right?

>  	udelay(20);
>  	out_be32(ioaddr->ctl_addr, ap->ctl);
>  
> -	scc_wait_after_reset(&ap->link, devmask, deadline);
> -
> -	return 0;
> +	return scc_wait_after_reset(&ap->link, devmask, deadline);
>  }
>  
>  /**
> @@ -618,7 +616,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
>  {
>  	struct ata_port *ap = link->ap;
>  	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
> -	unsigned int devmask = 0, err_mask;
> +	unsigned int devmask = 0;
> +	int err_mask;

@rc or @ret would be a better name for it.

Thanks.

-- 
tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

On 16 August 2014 17:07, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Sat, Aug 16, 2014 at 03:47:17PM +0530, Arjun Sreedharan wrote:
>> scc_bus_softreset not necessarily should return zero.
>> Propagate the error code.
>> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
>> ---
>>  drivers/ata/pata_scc.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
>> index 4e006d7..5618c4518 100644
>> --- a/drivers/ata/pata_scc.c
>> +++ b/drivers/ata/pata_scc.c
>> @@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
>>   *   Note: Original code is ata_bus_softreset().
>>   */
>>
>> -static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>> +static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>>                                        unsigned long deadline)
>>  {
>>       struct ata_ioports *ioaddr = &ap->ioaddr;
>> @@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>
> scc_bus_softreset() should return int too, right?

I have changed it from unsigned int to int for better clarity since
scc_wait_after_reset() could
return negative errnos. Any issues ??

>
>>       udelay(20);
>>       out_be32(ioaddr->ctl_addr, ap->ctl);
>>
>> -     scc_wait_after_reset(&ap->link, devmask, deadline);
>> -
>> -     return 0;
>> +     return scc_wait_after_reset(&ap->link, devmask, deadline);
>>  }
>>
>>  /**
>> @@ -618,7 +616,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
>>  {
>>       struct ata_port *ap = link->ap;
>>       unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
>> -     unsigned int devmask = 0, err_mask;
>> +     unsigned int devmask = 0;
>> +     int err_mask;
>
> @rc or @ret would be a better name for it.

rc would be better. yes. will do.

>
> Thanks.
>
> --
> tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

On Sat, Aug 16, 2014 at 05:34:31PM +0530, Arjun Sreedharan wrote:
> On 16 August 2014 17:07, Tejun Heo <tj@kernel.org> wrote:
> > Hello,
> >
> > On Sat, Aug 16, 2014 at 03:47:17PM +0530, Arjun Sreedharan wrote:
> >> scc_bus_softreset not necessarily should return zero.
> >> Propagate the error code.
> >> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
> >> ---
> >>  drivers/ata/pata_scc.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
> >> index 4e006d7..5618c4518 100644
> >> --- a/drivers/ata/pata_scc.c
> >> +++ b/drivers/ata/pata_scc.c
> >> @@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
> >>   *   Note: Original code is ata_bus_softreset().
> >>   */
> >>
> >> -static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
> >> +static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
> >>                                        unsigned long deadline)
> >>  {
> >>       struct ata_ioports *ioaddr = &ap->ioaddr;
> >> @@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
> >
> > scc_bus_softreset() should return int too, right?
> 
> I have changed it from unsigned int to int for better clarity since
> scc_wait_after_reset() could
> return negative errnos. Any issues ??

Oh, I missed the previous chunk which changes it from uint to int.
Sorry about that.

Thanks.

-- 
tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

On 16 August 2014 17:51, Tejun Heo <tj@kernel.org> wrote:
> On Sat, Aug 16, 2014 at 05:34:31PM +0530, Arjun Sreedharan wrote:
>> On 16 August 2014 17:07, Tejun Heo <tj@kernel.org> wrote:
>> > Hello,
>> >
>> > On Sat, Aug 16, 2014 at 03:47:17PM +0530, Arjun Sreedharan wrote:
>> >> scc_bus_softreset not necessarily should return zero.
>> >> Propagate the error code.
>> >> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
>> >> ---
>> >>  drivers/ata/pata_scc.c | 9 ++++-----
>> >>  1 file changed, 4 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
>> >> index 4e006d7..5618c4518 100644
>> >> --- a/drivers/ata/pata_scc.c
>> >> +++ b/drivers/ata/pata_scc.c
>> >> @@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
>> >>   *   Note: Original code is ata_bus_softreset().
>> >>   */
>> >>
>> >> -static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>> >> +static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>> >>                                        unsigned long deadline)
>> >>  {
>> >>       struct ata_ioports *ioaddr = &ap->ioaddr;
>> >> @@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
>> >
>> > scc_bus_softreset() should return int too, right?
>>
>> I have changed it from unsigned int to int for better clarity since
>> scc_wait_after_reset() could
>> return negative errnos. Any issues ??
>
> Oh, I missed the previous chunk which changes it from uint to int.
> Sorry about that.

OK. i will resend the patch.

>
> Thanks.
>
> --
> tejun

[PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

scc_bus_softreset not necessarily should return zero.
Propagate the error code.

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 drivers/ata/pata_scc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index 4e006d7..b5013dc 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
  *	Note: Original code is ata_bus_softreset().
  */
 
-static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
+static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
                                       unsigned long deadline)
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
@@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
 	udelay(20);
 	out_be32(ioaddr->ctl_addr, ap->ctl);
 
-	scc_wait_after_reset(&ap->link, devmask, deadline);
-
-	return 0;
+	return scc_wait_after_reset(&ap->link, devmask, deadline);
 }
 
 /**
@@ -618,7 +616,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
 {
 	struct ata_port *ap = link->ap;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
-	unsigned int devmask = 0, err_mask;
+	unsigned int devmask = 0;
+	int rc;
 	u8 err;
 
 	DPRINTK("ENTER\n");
@@ -634,8 +633,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
 
 	/* issue bus reset */
 	DPRINTK("about to softreset, devmask=%x\n", devmask);
-	err_mask = scc_bus_softreset(ap, devmask, deadline);
-	if (err_mask) {
+	rc = scc_bus_softreset(ap, devmask, deadline);
+	if (rc) {
 		ata_port_err(ap, "SRST failed (err_mask=0x%x)\n", err_mask);
 		return -EIO;
 	}
-- 
1.7.11.7

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

On Sat, Aug 16, 2014 at 06:20:09PM +0530, Arjun Sreedharan wrote:
> scc_bus_softreset not necessarily should return zero.
> Propagate the error code.
> 
> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>

Applied to libata/for-3.17-fixes.

Thanks.

-- 
tejun

[PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

scc_bus_softreset not necessarily should return zero.
Propagate the error code.

Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
---
 drivers/ata/pata_scc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index 4e006d7..b5013dc 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -585,7 +585,7 @@ static int scc_wait_after_reset(struct ata_link *link, unsigned int devmask,
  *	Note: Original code is ata_bus_softreset().
  */
 
-static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
+static int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
                                       unsigned long deadline)
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
@@ -599,9 +599,7 @@ static unsigned int scc_bus_softreset(struct ata_port *ap, unsigned int devmask,
 	udelay(20);
 	out_be32(ioaddr->ctl_addr, ap->ctl);
 
-	scc_wait_after_reset(&ap->link, devmask, deadline);
-
-	return 0;
+	return scc_wait_after_reset(&ap->link, devmask, deadline);
 }
 
 /**
@@ -618,7 +616,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
 {
 	struct ata_port *ap = link->ap;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
-	unsigned int devmask = 0, err_mask;
+	unsigned int devmask = 0;
+	int rc;
 	u8 err;
 
 	DPRINTK("ENTER\n");
@@ -634,8 +633,8 @@ static int scc_softreset(struct ata_link *link, unsigned int *classes,
 
 	/* issue bus reset */
 	DPRINTK("about to softreset, devmask=%x\n", devmask);
-	err_mask = scc_bus_softreset(ap, devmask, deadline);
-	if (err_mask) {
- 		ata_port_err(ap, "SRST failed (err_mask=0x%x)\n", err_mask);
+	rc = scc_bus_softreset(ap, devmask, deadline);
+	if (rc) {
+ 		ata_port_err(ap, "SRST failed (err_mask=0x%x)\n", rc);
 		return -EIO;
 	}
-- 
1.7.11.7

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

On Sat, Aug 16, 2014 at 07:48:04PM +0530, Arjun Sreedharan wrote:
> scc_bus_softreset not necessarily should return zero.
> Propagate the error code.
> 
> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>

I reverted the change yesterday but didn't have time to reply.  Don't
send patches you haven't even compile tested.  This is the most basic
of your responsibilities when sending out patches.  Okay?

Applying the updated version to libata/for-3.17-fixes.

-- 
tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

On Sun, Aug 17, 2014 at 07:10:42AM -0400, Tejun Heo wrote:
> On Sat, Aug 16, 2014 at 07:48:04PM +0530, Arjun Sreedharan wrote:
> > scc_bus_softreset not necessarily should return zero.
> > Propagate the error code.
> > 
> > Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
> 
> I reverted the change yesterday but didn't have time to reply.  Don't
> send patches you haven't even compile tested.  This is the most basic
> of your responsibilities when sending out patches.  Okay?
> 
> Applying the updated version to libata/for-3.17-fixes.

Gees, and your new patch doesn't even apply.  What are you doing?

-- 
tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Arjun Sreedharan]

On 17 August 2014 16:57, Tejun Heo <tj@kernel.org> wrote:
> On Sun, Aug 17, 2014 at 07:10:42AM -0400, Tejun Heo wrote:
>> On Sat, Aug 16, 2014 at 07:48:04PM +0530, Arjun Sreedharan wrote:
>> > scc_bus_softreset not necessarily should return zero.
>> > Propagate the error code.
>> >
>> > Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
>>
>> I reverted the change yesterday but didn't have time to reply.  Don't
>> send patches you haven't even compile tested.  This is the most basic
>> of your responsibilities when sending out patches.  Okay?

Deepest apologies. I had compile tested the first patch; but with a small
variable name change, i overlooked the need to compile test. my fault.

here's the link to the first patch:
https://lkml.org/lkml/diff/2014/8/16/15/1

Also now i feel @err_mask is better.
I rest my case further. Thanks.

>>
>> Applying the updated version to libata/for-3.17-fixes.
>
> Gees, and your new patch doesn't even apply.  What are you doing?
>
> --
> tejun

Re: [PATCH] ata: propagate return value of scc_wait_after_reset

[Tejun Heo]

On Sun, Aug 17, 2014 at 05:31:53PM +0530, Arjun Sreedharan wrote:
> Also now i feel @err_mask is better.
> I rest my case further. Thanks.

The thing is err_mask is used in libata to carry AC_ERR_* values not
0/-errno's, so it's confusing to use it for that.  Can you please
refresh the patch w/ variable rename?

Thanks.

-- 
tejun