Hi,

On Friday, July 19, 2013 06:45:31 PM Andreas Mohr wrote:

Hi,

On Fri, Jul 19, 2013 at 04:30:22PM +0200, Bartlomiej Zolnierkiewicz wrote:

quoted

On Friday, July 19, 2013 02:26:53 PM Andreas Mohr wrote:

quoted

- do a cable correction patch for libata side (I do think that indicating
  40c if even one device is 40c-only is the way to go [as long as libata
  cannot handle per-device settings], for safety reasons,

Why would libata need per-device cable setting? There is only one cable
per-port and it is shared between master/slave devices and some fancy
embedded implementations of the cable (CF slot + connector to slave
device) are not changing this fundamental design issue AFAIK.

I think it's simply because while the CF slot more or less obviously
seems to be rated "80c capable"-equivalent (due to its short connection?
c.f. various notebook situations),
*extension cable*(!) connections being connected to 44pin connectors
("ATA-44" ? conflicts with ATA/44 speed naming though)
seem to be rated as ATA/33 (https://de.wikipedia.org/wiki/ATA/ATAPI)
or IOW UDMA 2 (see various pages of 44pin CF adapter vendors).
So that means that you'd end up with a "mixed" capability primary port
(however my BIOS advertises 80c for both connectors,
which I suspect may not really be appropriate).

If you are able to argue hard evidence that this primary port Master/Slave
apparatus metallic layer connection is "one big shared resource"
(i.e. from a HF / EMI POV)
where behaviour ends up identical both for command and address timings
for all devices for all things practical that matter, then yeah.
But I have my doubts... (i.e., that it [address or command timings]
actually is to be seen in a device-connection-specific light,
to be reasonably safe).

But
https://en.wikipedia.org/wiki/Parallel_ATA

"For all modern ATA host adapters this is not true, as modern ATA host
adapters support independent device timing. This allows each device on
the cable to transfer data at its own best speed. Even with older
adapters without independent timing, this effect applies only to the
data transfer phase of a read or write operation. This is usually the
shortest part of a complete read or write operation."
seems to tend towards indicating that libata *should* have a per-device
form of .cable_detect(), unless "timing" != "cable type".

and indeed "timing" != "cable type"..

And, OTOH, you simply could pose the argument
that all that pseudo-intellectual reasoning does not matter,
since CS5536 actively *chose* to offer *per-device-specific* cable type bits
thus since it *does* offer such config possibility, libata *should*
support such (unless CS5536 somehow happened to be wrong in offering such
[useless?] liberty).


One way to extend libata to support such functionality might be
to have the driver set a flag to indicate that it has per-device cable type,
and then let libata serve a .cable_detect_per_device() or some such
instead.

Or choose to rework all drivers to have a per-device-extended
.cable_detect() instead (where most drivers would not [need to] make the
per-device distinction i.e. simply return the same setting for both).

I still don't see much need for this rework currently given that:
* such hardware setups are very rare (only CS5336 PATA controller gives
  you per-device cable type info, no other PATA controller does it AFAIK)
* we have fallback to lower speeds on errors
* even CS5536 BIOS describes the 44pin connector as 80c one and returns
  identical cable types for both master and slave devices (as reported by
  you in previous mail)

IOW until there is a practical need to enhance cable detection to work
per device it should not be done.

quoted

quoted

  and if so that's probably also preferrable to advertising an UNK value,
  since UNK would skip towards device-side cable detection
  which might be unreliable in case ATA ID values (i.e., ATA_ID_HW_CONFIG)
  happen to be incorrect,
  e.g. especially in the case of CF cards (OTOH it specifically looks
  for a 0x2000 bit being *set* for 80c indication,
  thus it should be reliable after all since you'd expect ignorant/older
  devices to provide zeroed fields only...)

Most controllers have per-port cable detection and CS5536 is being
an exception here. I don't know how this detection is implemented in
the hardware / BIOS but it could be that it is already taking into
the account the drive side cable detection and that is why we have two
values per-port. Please also note that the cable detection is only one
of components of selecting device transfer speed which also involves
comparing maximum host and device capabilities.

Yes, possibly because most controllers *do* have one single shared-medium
cable connection location per port only, whereas CS5536 (it only offers
one port in the first place, not two unlike many others!)
seems to treat the two connectors
(soldered-socket CF, and ATA-44 44pin, in the case of my system)
differently, *for that single port*.


CS5536 actually implicitly doing drive-side cable detection might be
a reason for the separate bits as you say, but I doubt it
(the controller would have to read out the device's ATA ID words, right??).

quoted

Indicating 40c in case if one device detects the cable to be 40c and
the other one detects 80c would unnecessarily punish the faster device
(which would then need usage of kernel parameter to make it work at
the full speed). Moreover it is unlikely that the hardware / BIOS
incorrectly detects 40c cable as 80c one and if this happens the error
recovery should trigger on CRC transfer errors and downgrade the current
speed. Therefore I would stronly suggest leaving pata_cs5536 cable
detection as it is currently.

OK, those are somewhat good {counter arguments | safety mechanisms}
(but see my point above about my ATA-44 connector getting advertised as 80c),
so perhaps...

BTW, does error recovery actively downgrade DMA mode on errors?
(kernel-side? or hardware even?)

Kernel downgrades UDMA modes on CRC (checked by hardware) transfer errors.

Didn't know that, but if so then a more aggressive default configuration
probably is acceptable.

quoted

quoted

- and what about cable correction patch for IDE side? Since IDE layer
  cable probing sequence algo appears to be different ("why??"),
  this might require advertising a different cable flag

I think that IDE CS5536 host driver doesn't need any changes w.r.t.
cable detection currently.

The IDE layer code is different for historical reasons and having
different probe method than libata (ah, the wonders of having two
pieces of code supporing the same hardware).

OK, so that leaves us with a possibly remaining debate on libata side only,
and I'd argue for extending libata for per-device cable detect support,
since that's arguably more precise and doesn't really cost anything
(that callback likely is utter slowpath, init-only)...
well, except for a sizeable amount of driver rework patches.

Until there is a demonstreable need for it (real hardware configuration
requiring such changes to work) there is no need to do it IMHO.

Instead I would like to come back to the wrong driver being selected
issue and fix it (cause this is a real problem with the pata_amd driver
taking control over CS5536 PATA controller and pata_cs5536 never being
used).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics