UBUNTU: SAUCE: Fixed ata error message printing when sb600 workaround is tried out

This patch fixes bug 444228 reported in launchpad.
It prevents early erroneous printing of ata error message.
It helps in a clean boot sequence.

This could also be applied for a merge upstream. Kindly do consider
merging this for Karmic.


The following changes since commit 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
  John Johansen (1):
        UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
allocation

are available in the git repository at:

  git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228

Surbhi Palande (1):
      UBUNTU: SAUCE: Fixed ata error message printing when sb600 work around
for a hardware bug is tried out

 drivers/ata/ahci.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)
From 4ad8fa70a83537307bec3300c318194e7e2188cc
Author: Surbhi Palande <surbhi.palande@canonical.com>
Date:   Fri Oct 9 15:02:09 2009 +0300
    
    Buglink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
    
    Soft reset fails on some ATI chips with IPMS set when PMP is enabled but
    SATA HDD/ODD is connected to SATA port. This is a hardware bug and a work
    around is to try a soft reset later at port 0.  Before this work around is
    tried out a error message indicating that the device is not ready was
    flashed out too early. Fixed this, to first try the work around if possible,
    otherwise flash an error message.
    
    Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>

Surbhi Palande wrote:
> This patch fixes bug 444228 reported in launchpad.
> It prevents early erroneous printing of ata error message.
> It helps in a clean boot sequence.
> 
> This could also be applied for a merge upstream. Kindly do consider
> merging this for Karmic.

I got a question to this: while the EIO will be handled with printing the 
message, ahci_do_softreset is also called from the generic ahci_softreset which 
does not handle that. Wouldn't it make sense to add error handling to the 
generic case?
Also just for me to learn: what is IPMS and PMP here?

> 
> The following changes since commit 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
>   John Johansen (1):
>         UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
> allocation
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228
> 
> Surbhi Palande (1):
>       UBUNTU: SAUCE: Fixed ata error message printing when sb600 work around
> for a hardware bug is tried out
> 
>  drivers/ata/ahci.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> From 4ad8fa70a83537307bec3300c318194e7e2188cc
> Author: Surbhi Palande <surbhi.palande@canonical.com>
> Date:   Fri Oct 9 15:02:09 2009 +0300
>     
>     Buglink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
>     
>     Soft reset fails on some ATI chips with IPMS set when PMP is enabled but
>     SATA HDD/ODD is connected to SATA port. This is a hardware bug and a work
>     around is to try a soft reset later at port 0.  Before this work around is
>     tried out a error message indicating that the device is not ready was
>     flashed out too early. Fixed this, to first try the work around if possible,
>     otherwise flash an error message.
>     
>     Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>
> 
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 289c4f8..1af5203 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1704,6 +1704,15 @@ static int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  
>  	/* wait for link to become ready */
>  	rc = ata_wait_after_reset(link, deadline, check_ready);
> +
> +	/*
> +	* Soft reset fails on some ATI chips with IPMS set when PMP
> +	* is enabled but SATA HDD/ODD is connected to SATA port.
> +	* A workaround shall be applied in the calling function
> +	* and we dont want to print error message without trying first
> +	*/
> +	if (rc == -EIO)
> +		return rc;
>  	if (rc == -EBUSY && hpriv->flags & AHCI_HFLAG_SRST_TOUT_IS_OFFLINE) {
>  		/*
>  		 * Workaround for cases where link online status can't
> @@ -1789,6 +1798,14 @@ static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
>  					"and retrying\n");
>  			rc = ahci_do_softreset(link, class, 0, deadline,
>  					       ahci_check_ready);
> +		} else {
> +			/* since no message was printed in ahci_do_softreset
> +			* when ret value was -EIO and since we are not trying
> +			* any work around, we should print the err msg here,
> +			* (ideally we should not reach here)
> +			*/
> +			ata_link_printk(link, KERN_ERR, "softreset failed "
> +					"(device not ready)\n");
>  		}
>  	}
> 
> 
>

Dear Stefan,

Hello!

>> I got a question to this: while the EIO will be handled with printing
the message, ahci_do_softreset is
>> also called from the generic ahci_softreset which does not handle
that. Wouldn't it make sense to add
>> error handling to the generic case?

When ahci_softreset() calls ahci_do_softreset(), it never gets -EIO
return value. It is only returned when ahci_sb600_check_ready() is
called ( i.e when
checkready = ahci_sb600_check_ready() and ahci_sb600_check_ready() is
the caller)
The only return values for the other checkready functions are 0
(SUCCESS), 1 (BUSY) or -ENODEV)
ahci_sb600_check_ready() uses -EIO to indicate that this result could be
because of a hardware bug.


>> Also just for me to learn: what is IPMS and PMP here?

If I understand it correctly, it is the port multiplier where the soft
reset signal has to be sent.

Warm Regards,
Surbhi.








Stefan Bader wrote:
> Surbhi Palande wrote:
>> This patch fixes bug 444228 reported in launchpad.
>> It prevents early erroneous printing of ata error message.
>> It helps in a clean boot sequence.
>>
>> This could also be applied for a merge upstream. Kindly do consider
>> merging this for Karmic.
>
> I got a question to this: while the EIO will be handled with printing
> the message, ahci_do_softreset is also called from the generic
> ahci_softreset which does not handle that. Wouldn't it make sense to
> add error handling to the generic case?
> Also just for me to learn: what is IPMS and PMP here?
>
>>
>> The following changes since commit
>> 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
>>   John Johansen (1):
>>         UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
>> allocation
>>
>> are available in the git repository at:
>>
>>   git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228
>>
>> Surbhi Palande (1):
>>       UBUNTU: SAUCE: Fixed ata error message printing when sb600 work
>> around
>> for a hardware bug is tried out
>>
>>  drivers/ata/ahci.c |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>> From 4ad8fa70a83537307bec3300c318194e7e2188cc
>> Author: Surbhi Palande <surbhi.palande@canonical.com>
>> Date:   Fri Oct 9 15:02:09 2009 +0300
>>         Buglink:
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
>>         Soft reset fails on some ATI chips with IPMS set when PMP is
>> enabled but
>>     SATA HDD/ODD is connected to SATA port. This is a hardware bug
>> and a work
>>     around is to try a soft reset later at port 0.  Before this work
>> around is
>>     tried out a error message indicating that the device is not ready
>> was
>>     flashed out too early. Fixed this, to first try the work around
>> if possible,
>>     otherwise flash an error message.
>>         Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>
>>
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 289c4f8..1af5203 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1704,6 +1704,15 @@ static int ahci_do_softreset(struct ata_link
>> *link, unsigned int *class,
>>  
>>      /* wait for link to become ready */
>>      rc = ata_wait_after_reset(link, deadline, check_ready);
>> +
>> +    /*
>> +    * Soft reset fails on some ATI chips with IPMS set when PMP
>> +    * is enabled but SATA HDD/ODD is connected to SATA port.
>> +    * A workaround shall be applied in the calling function
>> +    * and we dont want to print error message without trying first
>> +    */
>> +    if (rc == -EIO)
>> +        return rc;
>>      if (rc == -EBUSY && hpriv->flags &
>> AHCI_HFLAG_SRST_TOUT_IS_OFFLINE) {
>>          /*
>>           * Workaround for cases where link online status can't
>> @@ -1789,6 +1798,14 @@ static int ahci_sb600_softreset(struct
>> ata_link *link, unsigned int *class,
>>                      "and retrying\n");
>>              rc = ahci_do_softreset(link, class, 0, deadline,
>>                             ahci_check_ready);
>> +        } else {
>> +            /* since no message was printed in ahci_do_softreset
>> +            * when ret value was -EIO and since we are not trying
>> +            * any work around, we should print the err msg here,
>> +            * (ideally we should not reach here)
>> +            */
>> +            ata_link_printk(link, KERN_ERR, "softreset failed "
>> +                    "(device not ready)\n");
>>          }
>>      }
>>
>>
>>
>
>

Surbhi Palande wrote:
> Dear Stefan,
> 
> Hello!
> 
>>> I got a question to this: while the EIO will be handled with printing
> the message, ahci_do_softreset is
>>> also called from the generic ahci_softreset which does not handle
> that. Wouldn't it make sense to add
>>> error handling to the generic case?
> 
> When ahci_softreset() calls ahci_do_softreset(), it never gets -EIO
> return value. It is only returned when ahci_sb600_check_ready() is
> called ( i.e when
> checkready = ahci_sb600_check_ready() and ahci_sb600_check_ready() is
> the caller)
> The only return values for the other checkready functions are 0
> (SUCCESS), 1 (BUSY) or -ENODEV)
> ahci_sb600_check_ready() uses -EIO to indicate that this result could be
> because of a hardware bug.

Ah ok, so that message was only created with a sb600 chipset anyways. Ok, so 
this basically just delays printing the message and should be safe to do.

Acked-by: Stefan Bader <stefan.bader@canonical.com>


> 
>>> Also just for me to learn: what is IPMS and PMP here?
> 
> If I understand it correctly, it is the port multiplier where the soft
> reset signal has to be sent.

Thanks, makes it a bit clearer.

Stefan

> Warm Regards,
> Surbhi.
> 
> 
> 
> 
> 
> 
> 
> 
> Stefan Bader wrote:
>> Surbhi Palande wrote:
>>> This patch fixes bug 444228 reported in launchpad.
>>> It prevents early erroneous printing of ata error message.
>>> It helps in a clean boot sequence.
>>>
>>> This could also be applied for a merge upstream. Kindly do consider
>>> merging this for Karmic.
>> I got a question to this: while the EIO will be handled with printing
>> the message, ahci_do_softreset is also called from the generic
>> ahci_softreset which does not handle that. Wouldn't it make sense to
>> add error handling to the generic case?
>> Also just for me to learn: what is IPMS and PMP here?
>>
>>> The following changes since commit
>>> 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
>>>   John Johansen (1):
>>>         UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
>>> allocation
>>>
>>> are available in the git repository at:
>>>
>>>   git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228
>>>
>>> Surbhi Palande (1):
>>>       UBUNTU: SAUCE: Fixed ata error message printing when sb600 work
>>> around
>>> for a hardware bug is tried out
>>>
>>>  drivers/ata/ahci.c |   17 +++++++++++++++++
>>>  1 files changed, 17 insertions(+), 0 deletions(-)
>>> From 4ad8fa70a83537307bec3300c318194e7e2188cc
>>> Author: Surbhi Palande <surbhi.palande@canonical.com>
>>> Date:   Fri Oct 9 15:02:09 2009 +0300
>>>         Buglink:
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
>>>         Soft reset fails on some ATI chips with IPMS set when PMP is
>>> enabled but
>>>     SATA HDD/ODD is connected to SATA port. This is a hardware bug
>>> and a work
>>>     around is to try a soft reset later at port 0.  Before this work
>>> around is
>>>     tried out a error message indicating that the device is not ready
>>> was
>>>     flashed out too early. Fixed this, to first try the work around
>>> if possible,
>>>     otherwise flash an error message.
>>>         Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>
>>>
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index 289c4f8..1af5203 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -1704,6 +1704,15 @@ static int ahci_do_softreset(struct ata_link
>>> *link, unsigned int *class,
>>>  
>>>      /* wait for link to become ready */
>>>      rc = ata_wait_after_reset(link, deadline, check_ready);
>>> +
>>> +    /*
>>> +    * Soft reset fails on some ATI chips with IPMS set when PMP
>>> +    * is enabled but SATA HDD/ODD is connected to SATA port.
>>> +    * A workaround shall be applied in the calling function
>>> +    * and we dont want to print error message without trying first
>>> +    */
>>> +    if (rc == -EIO)
>>> +        return rc;
>>>      if (rc == -EBUSY && hpriv->flags &
>>> AHCI_HFLAG_SRST_TOUT_IS_OFFLINE) {
>>>          /*
>>>           * Workaround for cases where link online status can't
>>> @@ -1789,6 +1798,14 @@ static int ahci_sb600_softreset(struct
>>> ata_link *link, unsigned int *class,
>>>                      "and retrying\n");
>>>              rc = ahci_do_softreset(link, class, 0, deadline,
>>>                             ahci_check_ready);
>>> +        } else {
>>> +            /* since no message was printed in ahci_do_softreset
>>> +            * when ret value was -EIO and since we are not trying
>>> +            * any work around, we should print the err msg here,
>>> +            * (ideally we should not reach here)
>>> +            */
>>> +            ata_link_printk(link, KERN_ERR, "softreset failed "
>>> +                    "(device not ready)\n");
>>>          }
>>>      }
>>>
>>>
>>>
>>
>

Surbhi Palande wrote:
> This patch fixes bug 444228 reported in launchpad.
> It prevents early erroneous printing of ata error message.
> It helps in a clean boot sequence.
> 
> This could also be applied for a merge upstream. Kindly do consider
> merging this for Karmic.
> 
> 
> The following changes since commit 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
>   John Johansen (1):
>         UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
> allocation
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228
> 
> Surbhi Palande (1):
>       UBUNTU: SAUCE: Fixed ata error message printing when sb600 work around
> for a hardware bug is tried out
> 
>  drivers/ata/ahci.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> From 4ad8fa70a83537307bec3300c318194e7e2188cc
> Author: Surbhi Palande <surbhi.palande@canonical.com>
> Date:   Fri Oct 9 15:02:09 2009 +0300
>     
>     Buglink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
>     
>     Soft reset fails on some ATI chips with IPMS set when PMP is enabled but
>     SATA HDD/ODD is connected to SATA port. This is a hardware bug and a work
>     around is to try a soft reset later at port 0.  Before this work around is
>     tried out a error message indicating that the device is not ready was
>     flashed out too early. Fixed this, to first try the work around if possible,
>     otherwise flash an error message.
>     
>     Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>
> 
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 289c4f8..1af5203 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1704,6 +1704,15 @@ static int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  
>  	/* wait for link to become ready */
>  	rc = ata_wait_after_reset(link, deadline, check_ready);
> +
> +	/*
> +	* Soft reset fails on some ATI chips with IPMS set when PMP
> +	* is enabled but SATA HDD/ODD is connected to SATA port.
> +	* A workaround shall be applied in the calling function
> +	* and we dont want to print error message without trying first
> +	*/
> +	if (rc == -EIO)
> +		return rc;
>  	if (rc == -EBUSY && hpriv->flags & AHCI_HFLAG_SRST_TOUT_IS_OFFLINE) {
>  		/*
>  		 * Workaround for cases where link online status can't
> @@ -1789,6 +1798,14 @@ static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
>  					"and retrying\n");
>  			rc = ahci_do_softreset(link, class, 0, deadline,
>  					       ahci_check_ready);
> +		} else {
> +			/* since no message was printed in ahci_do_softreset
> +			* when ret value was -EIO and since we are not trying
> +			* any work around, we should print the err msg here,
> +			* (ideally we should not reach here)
> +			*/
> +			ata_link_printk(link, KERN_ERR, "softreset failed "
> +					"(device not ready)\n");
>  		}
>  	}
> 
> 
> 

NAK - I don't think this is necessary. Its a bonafide warning message
that merely indicates a workaround for a hardware issue. While your
patch works for the SB600 case, it _does_ propagate device specific
behavior into the generic layer (-EIO), not something that the
maintainer (Garzik) would likely allow.

Given that the default console log level has been raised to 2 (and the
message ought no longer be displayed on the console), I suggest you mark
this bug 'Invalid' because its not a software bug.

rtg

Tim Gardner wrote:
> Surbhi Palande wrote:
>> This patch fixes bug 444228 reported in launchpad.
>> It prevents early erroneous printing of ata error message.
>> It helps in a clean boot sequence.
>>
>> This could also be applied for a merge upstream. Kindly do consider
>> merging this for Karmic.
>>
>>
>> The following changes since commit 823da90960aa2f2442bec8cb0dc711b49f7a48ca:
>>   John Johansen (1):
>>         UBUNTU: SAUCE: AppArmor: Fix off by 2 error in getprocattr mem
>> allocation
>>
>> are available in the git repository at:
>>
>>   git://kernel.ubuntu.com/surbhi/ubuntu-karmic.git lp444228
>>
>> Surbhi Palande (1):
>>       UBUNTU: SAUCE: Fixed ata error message printing when sb600 work around
>> for a hardware bug is tried out
>>
>>  drivers/ata/ahci.c |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>> From 4ad8fa70a83537307bec3300c318194e7e2188cc
>> Author: Surbhi Palande <surbhi.palande@canonical.com>
>> Date:   Fri Oct 9 15:02:09 2009 +0300
>>     
>>     Buglink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/444228
>>     
>>     Soft reset fails on some ATI chips with IPMS set when PMP is enabled but
>>     SATA HDD/ODD is connected to SATA port. This is a hardware bug and a work
>>     around is to try a soft reset later at port 0.  Before this work around is
>>     tried out a error message indicating that the device is not ready was
>>     flashed out too early. Fixed this, to first try the work around if possible,
>>     otherwise flash an error message.
>>     
>>     Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com>
>>
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 289c4f8..1af5203 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1704,6 +1704,15 @@ static int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>>  
>>  	/* wait for link to become ready */
>>  	rc = ata_wait_after_reset(link, deadline, check_ready);
>> +
>> +	/*
>> +	* Soft reset fails on some ATI chips with IPMS set when PMP
>> +	* is enabled but SATA HDD/ODD is connected to SATA port.
>> +	* A workaround shall be applied in the calling function
>> +	* and we dont want to print error message without trying first
>> +	*/
>> +	if (rc == -EIO)
>> +		return rc;
>>  	if (rc == -EBUSY && hpriv->flags & AHCI_HFLAG_SRST_TOUT_IS_OFFLINE) {
>>  		/*
>>  		 * Workaround for cases where link online status can't
>> @@ -1789,6 +1798,14 @@ static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
>>  					"and retrying\n");
>>  			rc = ahci_do_softreset(link, class, 0, deadline,
>>  					       ahci_check_ready);
>> +		} else {
>> +			/* since no message was printed in ahci_do_softreset
>> +			* when ret value was -EIO and since we are not trying
>> +			* any work around, we should print the err msg here,
>> +			* (ideally we should not reach here)
>> +			*/
>> +			ata_link_printk(link, KERN_ERR, "softreset failed "
>> +					"(device not ready)\n");
>>  		}
>>  	}
>>
>>
>>
> 
> NAK - I don't think this is necessary. Its a bonafide warning message
> that merely indicates a workaround for a hardware issue. While your
> patch works for the SB600 case, it _does_ propagate device specific
> behavior into the generic layer (-EIO), not something that the
> maintainer (Garzik) would likely allow.

Still it might be worthwhile to discuss upstream / with Jeff. Maybe there is 
another solution but it might be a starting point to get rid of a totally 
useless warning.

-Stefan

> Given that the default console log level has been raised to 2 (and the
> message ought no longer be displayed on the console), I suggest you mark
> this bug 'Invalid' because its not a software bug.
> 
> rtg