Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=df33c77e3981e71afc8727ee5c432ba1a1bba68c
> Commit:     df33c77e3981e71afc8727ee5c432ba1a1bba68c
> Parent:     908e0a8a265fe8057604a9a30aec3f0be7bb5ebb
> Author:     Kristen Accardi <kristen.c.accardi@intel.com>
> AuthorDate: Fri Mar 9 18:15:33 2007 -0500
> Committer:  Len Brown <len.brown@intel.com>
> CommitDate: Fri Mar 9 18:15:33 2007 -0500
> 
>     libata-acpi: allow _GTF on SATA, but disable on PATA for now
>     
>     The ACPI specification states, and BIOS implementations depend on,
>     _STM being called before _GTF.
>     
>     SATA does this, but PATA does not.  So for now, simply
>     prevent execution of _GTF on PATA devices.  Longer term we
>     should implement ACPI support for PATA devices in libata.
>     
>     Signed-off-by: Kristen Accardi <kristen.c.accardi@intel.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/ata/libata-acpi.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index d14a48e..89aaf74 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -561,6 +561,13 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
>  
>  	if (noacpi)
>  		return 0;
> +	/*
> +	 * TBD - implement PATA support.  For now,
> +	 * we should not run GTF on PATA devices since some
> +	 * PATA require execution of GTM/STM before GTF.
> +	 */
> +	if (!(ap->cbl == ATA_CBL_SATA))
> +		return 0;
>  
>  	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
>  		if (!ata_dev_enabled(&ap->device[ix]))

Grumble!

This /really/ should have gone through me and linux-ide first.

Alan has been actively working on PATA ACPI, and we have been debugging 
ACPI issues as well.  PLEASE coordinate with the maintainer, when 
touching code outside of drivers/acpi!

AFAICS this patch went in with zero appearance on LKML or another 
related list, until submission.  This is /not/ how we do Linux development.

	Jeff

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Alan Cox]

> Alan has been actively working on PATA ACPI, and we have been debugging 
> ACPI issues as well.  PLEASE coordinate with the maintainer, when 
> touching code outside of drivers/acpi!

ap->cbl is not a reliable way to tell SATA from PATA at the moment. We
need to fix the real crashes too - right now no non PCI controller
actually works at all (crash on boot) because of the ACPI crap.

Please drop this patch as it will cause regressions on systems that do
not need the extra methods. Instead merge the one that fixe the non PCI
crashes and we might get further.

> AFAICS this patch went in with zero appearance on LKML or another 
> related list, until submission.  This is /not/ how we do Linux development.

Agreed.

NAK the patch likewise.

Alan

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Kristen Carlson Accardi]

On Sat, 10 Mar 2007 16:40:59 +0000
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> Instead merge the one that fixe the non PCI
> crashes and we might get further.

In bugzilla #7907 you could post your patch and see if the reporters there
can at least confirm that it fixes their problem.

Meanwhile, to solve the problem for these people, and without introducing
a lot of new code at this stage in the release, perhaps you can suggest 
a more accurate way to detect PATA vs. SATA so we can fix the bug?  Either 
that or Len should drop the ATA ACPI support from 2.6.21 altogether and
we should try to coordinate our efforts better for 2.6.22.  The alternative
to dropping the ACPI support is to set the default for libata.acpi to off.

Thanks,
Kristen

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Alan Cox]

> Meanwhile, to solve the problem for these people, and without introducing
> a lot of new code at this stage in the release, perhaps you can suggest 
> a more accurate way to detect PATA vs. SATA so we can fix the bug?  

I think the problem starts with the question. It supposes the BIOS knows
the answer to the question in the first place. It also supposes we
haven't reconfigured the port in some cases I believe.

My guess is the problem needs approaching from the other end. Ask the
question "What does the ACPI BIOS expect it to be ?". If I follow the
spec correctly we can answer that question by asking what methods (_SDD,
_GTF etc) it provides.

I don't think anything else is reliable.

Alan

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Kristen Carlson Accardi]

On Mon, 12 Mar 2007 22:00:15 +0000
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Meanwhile, to solve the problem for these people, and without introducing
> > a lot of new code at this stage in the release, perhaps you can suggest 
> > a more accurate way to detect PATA vs. SATA so we can fix the bug?  
> 
> I think the problem starts with the question. It supposes the BIOS knows
> the answer to the question in the first place. It also supposes we
> haven't reconfigured the port in some cases I believe.
> 
> My guess is the problem needs approaching from the other end. Ask the
> question "What does the ACPI BIOS expect it to be ?". If I follow the
> spec correctly we can answer that question by asking what methods (_SDD,
> _GTF etc) it provides.
> 
> I don't think anything else is reliable.
> 
> Alan
> 

Ah - ok, so you are saying we could query the device to see if it supports
STM/GTM and then if we were to use the workaround I suggested for this bug
we could just refuse to execute GTF on devices which support STM (since
they may require execution of STM first).  If you guys decide you want 
the workaround rather than just either disable acpi or remove the acpi patch
entirely, let me know and I'd be glad to rework it, otherwise I'm assuming
you have decided to just disable acpi all together.

Re: libata-acpi: allow _GTF on SATA, but disable on PATA for now

[Len Brown]

On Saturday 10 March 2007 06:30, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=df33c77e3981e71afc8727ee5c432ba1a1bba68c
> > Commit:     df33c77e3981e71afc8727ee5c432ba1a1bba68c
> > Parent:     908e0a8a265fe8057604a9a30aec3f0be7bb5ebb
> > Author:     Kristen Accardi <kristen.c.accardi@intel.com>
> > AuthorDate: Fri Mar 9 18:15:33 2007 -0500
> > Committer:  Len Brown <len.brown@intel.com>
> > CommitDate: Fri Mar 9 18:15:33 2007 -0500
> > 
> >     libata-acpi: allow _GTF on SATA, but disable on PATA for now
> >     
> >     The ACPI specification states, and BIOS implementations depend on,
> >     _STM being called before _GTF.
> >     
> >     SATA does this, but PATA does not.  So for now, simply
> >     prevent execution of _GTF on PATA devices.  Longer term we
> >     should implement ACPI support for PATA devices in libata.
> >     
> >     Signed-off-by: Kristen Accardi <kristen.c.accardi@intel.com>
> >     Signed-off-by: Len Brown <len.brown@intel.com>
> > ---
> >  drivers/ata/libata-acpi.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index d14a48e..89aaf74 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -561,6 +561,13 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
> >  
> >  	if (noacpi)
> >  		return 0;
> > +	/*
> > +	 * TBD - implement PATA support.  For now,
> > +	 * we should not run GTF on PATA devices since some
> > +	 * PATA require execution of GTM/STM before GTF.
> > +	 */
> > +	if (!(ap->cbl == ATA_CBL_SATA))
> > +		return 0;
> >  
> >  	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
> >  		if (!ata_dev_enabled(&ap->device[ix]))
> 
> Grumble!
> 
> This /really/ should have gone through me and linux-ide first.

Back at you Jeff,
This feature /really/ should have never gone upstream in the first place,
as this failure was reported and isolated to git-libata-all.patch
back in 2.6.20-rc6-mm3:

http://bugzilla.kernel.org/show_bug.cgi?id=7907

It then went on to become the most widely reported "ACPI related"
regression in the 2.6.21-rc series -- for which ACPI gets smeared.
Thank you ATA...

> Alan has been actively working on PATA ACPI, and we have been debugging 
> ACPI issues as well.  PLEASE coordinate with the maintainer, when 
> touching code outside of drivers/acpi!

And PLEASE coordinate with the maintainer when invoking methods
that provoke errors in other sub-systems!

Re: "debugging ACPI issues as well"

What issues?
Why haven't I see any mention of them on linux-acpi?
Coordination and communication is a two-way street, Jeff.

> AFAICS this patch went in with zero appearance on LKML or another 
> related list, until submission.  This is /not/ how we do Linux development.

I proudly take credit+blame for shipping Kristen's patch with no delay.
It did appear on linux-acpi, as do all the patches I ship -- though
I admit it was the same day it went upstream.
I'm sorry I didn't CC linux-ide -- I'll get that part right next time.

However, I believe that late -rc3 is _well_ past the time to be developing
new code real-time in the upstream tree; and is instead time to
shut the damn thing off and set sights on the next release.

If you disagree with me, I'm not going to object
when you send a better fix to Linus for 2.6.21-rc4.

However, I do request that you first either start responding
to bugzilla traffic, or delete your account from bugzilla
so that people don't get the false impression that you're
paying attention.

thanks,
-Len