Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

[Jeff Garzik]

akpm@osdl.org wrote:
> The patch titled
> 
>      Enable reads on Plextor 712-SA on 2.6.11.5
> 
> has been added to the -mm tree.  Its filename is
> 
>      enable-reads-on-plextor-712-sa-on-26115.patch
> 
> Patches currently in -mm which might be from andystewart@comcast.net are
> 
> enable-reads-on-plextor-712-sa-on-26115.patch

Andrew -- The use of the word 'hack' didn't trigger any response??

By hardcoding so much of the inquiry data, this patch -overwrites- valid 
inquiry data provided by the device, with generic data.  This patch 
makes generic the probe data that the SCSI layer -depends on to be 
different-.

Effectively you made one CD-ROM device work, killed all the others, and 
enabled an oops generator.

Good show.

Even if this patch worked, you still need to fix the following:

* Patch INQUIRY data -slightly- to fool the SCSI layer into working 
correctly.  This is what Andy's patch [poorly] attempts to address.
* Handling DRQ interrupts (early patch exists)
* Padding DMA data (50% patch exists)
* Fix error handling (patch exists)
* Fix all FIS-based drivers so that an error doesn't cause an oops
* Implement non-polled REQUEST SENSE error handling for FIS-based drivers

Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

[Andy Stewart]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Garzik wrote:
> akpm@osdl.org wrote:
>> The patch titled
>>
>>      Enable reads on Plextor 712-SA on 2.6.11.5
>>
>> has been added to the -mm tree.  Its filename is
>>
>>      enable-reads-on-plextor-712-sa-on-26115.patch
>>
>> Patches currently in -mm which might be from andystewart@comcast.net are
>>
>> enable-reads-on-plextor-712-sa-on-26115.patch
> 
> Andrew -- The use of the word 'hack' didn't trigger any response??

HI Jeff et al,

If you are referencing the commented out debug messages, I apologize,
and will endeavor to omit those in the future.

> 
> By hardcoding so much of the inquiry data, this patch -overwrites- valid
> inquiry data provided by the device, with generic data.  This patch
> makes generic the probe data that the SCSI layer -depends on to be
> different-.

The SCSI inquiry command does not work on this device for reasons
unknown to me.  I saw in the code where the SCSI inquiry command was
"emulated", or handled in software, for ATA devices.  I simply copied
that method for ATAPI devices.  At least that was my intent.  I cloned
one function, modified it slightly, and (I thought) called it in a
reasonable place.

> 
> Effectively you made one CD-ROM device work, killed all the others, and
> enabled an oops generator.

I fail to see how other devices would have been killed by this patch.
The inquiry data were simply moved from one data structure to another.

I tested this patch on my system with many different reads, mounts, and
unmounts and never generated an oops.  Would you tell me what you did
that caused an oops?  That would help me to improve my testing before
attempting to submit future patches.

> 
> Good show.

Aw, come on, Jeff.  I gave it a shot, I'm trying to give back to the
community rather than simply complain.  OK, so my work isn't perfect,
and you've pointed ont valid technical reasons why.  At least *I tried*
to contribute code rather than just offering complaints, and I'm willing
to admit that I'll need to try harder in the future.

> 
> Even if this patch worked, you still need to fix the following:
> 
> * Patch INQUIRY data -slightly- to fool the SCSI layer into working
> correctly.  This is what Andy's patch [poorly] attempts to address.
> * Handling DRQ interrupts (early patch exists)
> * Padding DMA data (50% patch exists)
> * Fix error handling (patch exists)
> * Fix all FIS-based drivers so that an error doesn't cause an oops
> * Implement non-polled REQUEST SENSE error handling for FIS-based drivers

As with any patch, it is completely at the discretion of the kernel
developers as to whether it gets included.  If you believe this patch is
inappropriate, then please ask Andrew Morton to remove it, and I'll
respect the decision.

Thanks for looking at the code and for offering your comments.

Andy

- --
Andy Stewart, Founder
Worcester Linux Users' Group
Worcester, MA, USA
http://www.wlug.org

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCknQ8Hl0iXDssISsRAtvpAKCBwfatiK8bLCSTz6EztI+C50KP2QCeM7vd
TCIX1cYtKEdsQK8zRTNr0Do=
=ZZKj
-----END PGP SIGNATURE-----

Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

[Andrew Morton]

Andy Stewart <andystewart@comcast.net> wrote:
>
> > 
> > Good show.
> 
> Aw, come on, Jeff.  I gave it a shot,

You did, and that's very much appreciated, thanks.

> If you believe this patch is
> inappropriate, then please ask Andrew Morton to remove it

I already have removed it, but only because it appears that the patch might
cause regressions in other areas.

Basically it goes like this:

- Someone sends a patch

- Patch gets ignored.

- I merge it, without even looking at the thing.

I do this as a reminder to myself and to other developers that there is
some kernel bug or shortcoming.  Basically, it's a bug-tracking system. 
Periodically I'll spam the maintainer with the patch and eventually he'll
tell me to drop the thing because the problem is fixed, or he'll tell the
originator why the patch will never be merged.

Either way, the patch (and the problem which caused you to write the patch)
gets some sort of definite dispositive handling, rather than falling
through a crack.

Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

[Andy Stewart]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


HI Andrew,

Andrew Morton wrote:
> Andy Stewart <andystewart@comcast.net> wrote:
>>>Good show.
>>Aw, come on, Jeff.  I gave it a shot,
> 
> You did, and that's very much appreciated, thanks.

Thank you for the encouraging words.  :-)

> 
>>If you believe this patch is
>>inappropriate, then please ask Andrew Morton to remove it
> 
> I already have removed it, but only because it appears that the patch might
> cause regressions in other areas.

I agree that my patch should be removed if it is breaking other things.
Thanks for taking it out.

> 
> Basically it goes like this:
> 
> - Someone sends a patch
> 
> - Patch gets ignored.
> 
> - I merge it, without even looking at the thing.
> 
> I do this as a reminder to myself and to other developers that there is
> some kernel bug or shortcoming.  Basically, it's a bug-tracking system. 
> Periodically I'll spam the maintainer with the patch and eventually he'll
> tell me to drop the thing because the problem is fixed, or he'll tell the
> originator why the patch will never be merged.
> 
> Either way, the patch (and the problem which caused you to write the patch)
> gets some sort of definite dispositive handling, rather than falling
> through a crack.

Thank you for the explaination, and for the feedback to let me know that
the patch didn't fall through the cracks.

Andy

- --
Andy Stewart, Founder
Worcester Linux Users' Group
Worcester, MA, USA
http://www.wlug.org

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCknj3Hl0iXDssISsRApp1AJ97vyP5CGavkNmP0TIHL4yvVga6nwCdFL1+
wYN6ergAnBeboO0OQK9/NBo=
=RwPj
-----END PGP SIGNATURE-----

Re: enable-reads-on-plextor-712-sa-on-26115.patch added to -mm tree

[Jeff Garzik]

Andy Stewart wrote:
> Jeff Garzik wrote:
>>By hardcoding so much of the inquiry data, this patch -overwrites- valid
>>inquiry data provided by the device, with generic data.  This patch
>>makes generic the probe data that the SCSI layer -depends on to be
>>different-.

> The SCSI inquiry command does not work on this device for reasons
> unknown to me.  I saw in the code where the SCSI inquiry command was
> "emulated", or handled in software, for ATA devices.  I simply copied
> that method for ATAPI devices.  At least that was my intent.  I cloned
> one function, modified it slightly, and (I thought) called it in a
> reasonable place.

All of SCSI is emulated for ATA; for ATAPI, 99% of SCSI is passed 
through to the underlying device.  The two must be treated very differently.

The SCSI layer needs to see the per-device data returned by passing the 
INQUIRY command to the device via the ATA PACKET command.


>>Effectively you made one CD-ROM device work, killed all the others, and
>>enabled an oops generator.
> 
> 
> I fail to see how other devices would have been killed by this patch.

The SCSI layer discovers, and interprets devices based on the data 
returned by the INQUIRY command.  Your patch causes the kernel to act as 
if all ATAPI devices behave just like your Plextor, which is very very 
wrong.

It's a good thing nobody tried to use an ATAPI tape drive with your 
patch, for example.  The kernel would have thought it was a CD-ROM, and 
tried to talk to it as such.


> I tested this patch on my system with many different reads, mounts, and
> unmounts and never generated an oops.  Would you tell me what you did
> that caused an oops?  That would help me to improve my testing before
> attempting to submit future patches.

Any use of ATAPI in certain drivers (like AHCI) will cause an oops, 
because those drivers are not yet fully ATAPI aware.


>>Good show.
> 
> 
> Aw, come on, Jeff.  I gave it a shot, I'm trying to give back to the
> community rather than simply complain.  OK, so my work isn't perfect,
> and you've pointed ont valid technical reasons why.  At least *I tried*
> to contribute code rather than just offering complaints, and I'm willing
> to admit that I'll need to try harder in the future.

There's nothing wrong with contributing to ATAPI debugging and development!

We just don't need to be merging such a broken patch into -mm, where it 
will cause more headaches than it will solve.

Thou Shalt Not Turn On ATAPI Before Its Time.  It's still in the realm 
of debugging patches.

	Jeff