Message ID | 1255092797-5019-1-git-send-email-surbhi.palande@canonical.com |
---|---|
State | Rejected |
Headers | show |
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
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"); } }
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>