Re: 2.6.19-rc3 system freezes when ripping with cdparanoia at ioctl(SG_IO)

[Luben Tuikov <ltuikov@yahoo.com>]

--- Douglas Gilbert <dougg@torque.net> wrote:
> Luben Tuikov wrote:
> > --- Douglas Gilbert <dougg@torque.net> wrote:
> >> Tejun Heo wrote:
> >>> [CC'ing Monty and Douglas.]
> >>>
> >>> Hello, the original thread can be read from the following URL.
> >>>
> >>> http://thread.gmane.org/gmane.linux.ide/13708/focus=13708
> >>>
> >>> Brice Goglin wrote:
> >>>> ens Axboe wrote:
> >>>>> On Mon, Oct 30 2006, Gregor Jasny wrote:
> >>>>>  
> >>>>>> 2006/10/30, Jens Axboe <jens.axboe@oracle.com>:
> >>>>>>    
> >>>>>>> Can you confirm that 2.6.18 works?
> >>>>>>>       
> >>>>>> The reporter of [1] states that his SATA Thinkpad freezes with 2.6.17
> >>>>>> and 2.6.18, too.
> >>>>>>
> >>>>>> Gregor
> >>>>>>
> >>>>>> [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=391901
> >>>>>>     
> >>>>> Ok, mainly just checking if this was a potential dupe of another bug.
> >>>>>
> >>>>>   
> >>>> Jens (or anybody else who has any idea of how to debug this),
> >>>>
> >>>> Did you have a chance to reproduce the problem? I guess we "only" need a
> >>>> machine with SATA/ata_piix and cdparanoia 3.10. If you want me to debug
> >>>> some stuff, feel free to tell me what. But, since it freezes the machine
> >>>> and sysrq doesn't even work, I don't really know what to try...
> >>>>
> >>>> I just tried on rc5 and rc5-mm1, both have the problem (as 2.6.16, .17
> >>>> and .18 do, don't know about earlier kernels). I didn't have a audio CD
> >>>> here, so I tried abcde on a DVD on purpose. With cdparanoia 3.10-pre0
> >>>> (from Debian testing), it reports nothing during about 5 seconds and
> >>>> then the machine freezes. With cdparanoia 3a9.8-11 (from Debian stable),
> >>>> it reports an error very quickly, and dmesg gets a couple line like
> >>>> these:
> >>>>     sg_write: data in/out 12/12 bytes for SCSI command 0x43--guessing
> >>>> data in;
> >>>>        program cdparanoia not setting count and/or reply_len properly
> >>> Okay, here's the story.
> >>>
> >>> In interface/scan_devices.c::cdda_identify_scsi(), cdparanoia calls
> >>> scsi_inquiry() to identify the device and determine interface type. This
> >>> seems to be the first time to actually issue commands to the device.  As
> >>> interface type isn't completely determined, for sg devices, it first
> >>> issues the command w/ d->interface set to SGIO_SCSI.  If that fails, it
> >>> falls back to SGIO_SCSI_BUGGY1.
> >>>
> >>> For to-device request, both SGIO_SCSI and SGIO_SCSI_BUGGY1 set
> >>> sg_io_hdr.dxfer_direction to SG_DXFER_TO_DEV.  But for from-device
> >>> request, SGIO_SCSI uses SG_DXFER_TO_FROM_DEV while SGIO_SCSI_BUGGY1 uses
> >>> SG_DXFER_FROM_DEV.  So, cdparanoia first issues inquiry w/
> >>> SG_DXFER_TO_FROM_DEV and if that fails falls back to SG_DXFER_FROM_DEV.
> >>>
> >>> drivers/scsi/sg.c interprets SG_DXFER_TO_FROM_DEV as read while
> >>> block/scsi_ioctl.c interprets it as write.  I guess this is historic
> >>> thing (scsi/sg.c updated but block/scsi_ioctl.c is forgotten).  As
> >>> written above, cdparanoia can handle both cases as long as the kernel
> >>> promptly fails command issued with the wrong direction.
> >>>
> >>> This works for most PATA ATAPI devices.  Most devices detect reversed
> >>> transfer and terminate the command promptly.  But this doesn't seem to
> >>> be true for SATA device.  Many just hang and time out commands with the
> >>> wrong transfer direction.  If you consider that most early SATA ATAPI
> >>> devices are actually PATA + bridge, this is sorta inevitable.  The
> >>> PATA-SATA bridge cannot issue D2H FIS to abort the command by itself.
> >>> It's just mirroring the status of PATA side and PATA side doesn't know
> >>> SATA protocol mismatch has occurred.
> >>>
> >>> So, IDENTIFY w/ write-DMA protocol times out after quite some seconds.
> >>> This is where things go worse from bad.  SATA controllers which have
> >>> shadow TF registers don't handle timeout conditions very well,
> >>> especially when they're waiting for data transfer.  They basically hold
> >>> the PCI bus and hang till the transfer completes (which never happens).
> >>>  That's where the hard lock up comes from.
> >>>
> >>> Jens, I think we need to match block sg's behavior to SCSI's.  Monty,
> >>> the timeout and hard lock up are due to hardware restrictions.  Kernel
> >>> and libata can't do much about it.  So, please find other way to detect
> >>> interface.
> >> Tejun,
> >> Your SG_DXFER_TO_FROM_DEV analysis is correct.
> >>
> >> The stupid ~!@# who wrote the code, and the documentation
> >> for it, defined SG_DXFER_TO_FROM_DEV to mean a "transfer
> >> from device" operation where the kernel buffer receiving
> >> the DMA transfer was prefilled with data that the application
> >> provided. That certainly isn't a bidirectional transfer to/from
> >> the device, but it is a bidirectional transfer to kernel
> >> buffers when indirect IO is used.
> >>
> >> Why do this? Because the 'resid' field indicating how much
> >> less data was transferred in a "from_device" transfer than
> >> was requested, was not added to SCSI infrastructure till much
> >> later. There are still LLDs out there that don't implement it.
> >> It also reflected a similar technique used with the sg_header
> >> structure (circa 1992) for precisely the same reason. And
> >> application writers wanted that functionality. Joerg was the
> >> first name of one such application writer.
> >>
> >>
> >> Coincidentally I am sitting on a patch from Luben Tuikov
> >> to cause the same breakage in the sg driver itself.
> > 
> > Here is a link to the recently posted 8 month patch:
> > http://marc.theaimsgroup.com/?l=linux-scsi&m=116267031029025&w=2
> > 
> > The patch would appear to fix the problem Tejun is describing.
> > 
> > I cannot quite remember exactly what I was doing that day 8 months
> > ago, but was either disk or tape devices testing and arrived
> > at that patch.
> > 
> > This patch had been in my dev (gateway) tree for the last 8
> > months, without any problems.
> > 
> >     Luben
> > 
> > 
> >> Nobody has proposed a patch to the documentation for
> >> the explanation of SG_DXFER_TO_FROM_DEV :-)
> >>     http://www.torque.net/sg/p/sg_v3_ho.html
>                                   ^^^^^^^^^^^^^
> 
> Luben,
> The failure being reported is that the block layer
> SG_IO ioctl already does what you are proposing to
> do for the sg driver.
> 
> Hence an application, cdparanoia in this case, since
> it coded against documented behaviour, assumes that
> SG_DXFER_TO_FROM_DEV will read from the device.
> See the definition of SG_DXFER_TO_FROM_DEV in sg.h and
> the document above.
> 
> So your proposed patch would compound the problem. The
> solution is _not_ to change the sg driver and put the
> equivalent of the reverse of your patch in the block
> layer SG_IO ioctl.
> 
> There is nothing to stop a new direction flag being
> added called SG_DXFER_BIDIRECTIONAL that maps to
> DMA_BIDIRECTIONAL.

Sounds good!

  Luben

P.S. I'd love to see SG_DXFER_TO_FROM_DEV completely ripped out
of sg.c, for obvious reasons.  Can you not duplicate the resid "fix"
it provides into "FROM_DEV" -- do apps really rely on it?