Re: DISABLE_CLUSTERING in scsi drivers

[Michael Schmitz]

Am 26.11.2018 um 20:50 schrieb Christoph Hellwig:

On Thu, Nov 22, 2018 at 10:11:33AM +1300, Michael Schmitz wrote:

Christoph,
for Atari SCSI, commands can only be merged if the physical addresses
of all buffers are contiguous (limitation of the Falcon DMA engine).
Documentation/scsi/scsi_mid_low_api.tx does not spell out whether that
is the case.

Atari SCSI disables scatter/gather, so if that's sufficient to cue
midlevel or bio to not undertake any merging, the flag is no longer
needed.


Yes, if scatter/gather is disable (sg_tablesize == 1 or 0), there will
just be a single, contiguos segment up to .max_sectors, which might
straddle a page boundary if it is larger than PAGE_SIZE.  If that is
ok for the ata SCSI hardware we can remove the DISABLE_CLUSTERING
setting.


No requirements for DMA segments to be page aligned or not cross a page 
boundary - max. 255 sectors for Falcon (and the driver will fall back to 
PIO if that's exceeded IIRC).


No problem removing the setting - probably best if Finn queues that 
change for all NCR5380 drivers that can cope with it.


Cheers,

Michael

Re: DISABLE_CLUSTERING in scsi drivers

[Michael Schmitz]

Christoph,
for Atari SCSI, commands can only be merged if the physical addresses
of all buffers are contiguous (limitation of the Falcon DMA engine).
Documentation/scsi/scsi_mid_low_api.tx does not spell out whether that
is the case.

Atari SCSI disables scatter/gather, so if that's sufficient to cue
midlevel or bio to not undertake any merging, the flag is no longer
needed.

Cheers,

  Michael

On Wed, Nov 21, 2018 at 10:41 PM Christoph Hellwig  wrote:
>
> Hi all,
>
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments.  We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
>
> Can you go over your drivers and check if it could be removed?

Re: [PATCH v9] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Martin,

thank you so much!

Ben - please drop the Amiga SCSI patch for Debian testing from kernel
version 4.18!

Cheers,

Michael


Am 19.04.2018 um 16:01 schrieb Martin K. Petersen:
> 
> Michael,
> 
>> Anything else needed for linux-scsi?
> 
> Applied to 4.18/scsi-queue. Thanks!
> 

Re: [PATCH v9] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Thanks guys!

Anything else needed for linux-scsi?

Cheers,

  Michael


On Mon, Apr 16, 2018 at 10:34 PM, John Paul Adrian Glaubitz
<glaub...@physik.fu-berlin.de> wrote:
> On 04/16/2018 11:26 AM, Christian T. Steigies wrote:
>>
>> On Thu, Apr 12, 2018 at 01:53:26PM +1200, Michael Schmitz wrote:
>>>
>>> From: Michael Schmitz <schm...@debian.org>
>>>
>>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>>> m68k Amiga.
>>>
>>> Code largely based on board specific parts of the old drivers (blz1230.c,
>>> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
>>> after the 2.6 kernel series for lack of maintenance) with contributions
>>> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
>>> bugfix by use of PIO in extended message in transfer).
>>>
>>> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
>>> driver included in this patch.
>>>
>>> Use DMA transfers wherever possible, with board-specific DMA set-up
>>> functions copied from the old driver code. Three byte reselection
>>> messages
>>> do appear to cause DMA timeouts. So wire up a PIO transfer routine for
>>> these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma
>>> as
>>> target address for the message bytes but PIO requires a virtual address.
>>> Substiute kernel virtual address esp->cmd_block in PIO transfer call if
>>> DMA address is esp->cmd_block_dma and phase is message in.
>>>
>>> PIO code taken from mac_esp.c where the reselection timeout issue was
>>> debugged and fixed first, with minor macro and function rename.
>>>
>>> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
>>> Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
>>> Reviewed-by: Christoph Hellwig <h...@lst.de>
>>
>> Tested-by: Christian T. Steigies <c...@debian.org>
>>
>
> Also here on an Amiga 4000 with Cyberstorm I:
>
> Tested-by: John Paul Adrian Glaubitz <glaub...@physik.fu-berlin.de>
>
> --
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaub...@debian.org
> `. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
>   `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[PATCH v9] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
Reviewed-by: Christoph Hellwig <h...@lst.de>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver pri

Re: [PATCH v8] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert,

On Wed, Apr 11, 2018 at 8:56 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Michael,
>
> On Wed, Apr 11, 2018 at 10:36 AM, Michael Schmitz <schmitz...@gmail.com> 
> wrote:
>> From: Michael Schmitz <schm...@debian.org>
>>
>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>> m68k Amiga.
>
> Thanks a lot!

My pleasure entirely.

>
>> --- /dev/null
>> +++ b/drivers/scsi/zorro_esp.c
>
>> +static const struct zorro_device_id zorro_esp_zorro_tbl[] = {
>> +   {   /* Blizzard 1230 IV */
>> +   .id = ZORRO_ID(PHASE5, 0x11, 0),
>> +   .driver_data = (unsigned long)ZORRO_BLZ1230,
>
> These casts are not needed if ZORRO_BLZ1230 is an enum.

Oh dear - meant to check that but forgot in the end. I'll respin.

Cheers,

  Michael

>
>> +static int zorro_esp_probe(struct zorro_dev *z,
>> +  const struct zorro_device_id *ent)
>> +{
>> +   struct scsi_host_template *tpnt = _esp_template;
>> +   struct Scsi_Host *host;
>> +   struct esp *esp;
>> +   const struct zorro_driver_data *zdd;
>> +   struct zorro_esp_priv *zep;
>> +   unsigned long board, ioaddr, dmaaddr;
>> +   int err;
>> +
>> +   board = zorro_resource_start(z);
>> +   zdd = _esp_boards[(unsigned long)ent->driver_data];
>
> likewise
>
>> +   if (zep->zorro3 && ent->driver_data == (unsigned 
>> long)ZORRO_BLZ1230II) {
>
> likewise
>
>> +   if (zep->zorro3 && ent->driver_data == (unsigned 
>> long)ZORRO_BLZ1230II) {
>
> one more
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert, Christoph,

Am 11.04.2018 um 20:30 schrieb Geert Uytterhoeven:
> Hi Christoph,
> 
> On Wed, Apr 11, 2018 at 10:11 AM, Christoph Hellwig  
> wrote:
>> On Wed, Apr 11, 2018 at 10:03:12AM +0200, Geert Uytterhoeven wrote:
 That would be cool. Would that still be in time for the 4.17 merge?
>>>
>>> Nope, as new drivers need to be in linux-next before the merge window opens.
>>
>> I always throught that entirely new drivers were an exception to that.
> 
> Aren't you confusing this with the new device ID rules for stable?
> 
> It's debatable, and thus up to the maintainer...

Would be nice but this has been chugging along for over four years in my
queue so I'm sure it can wait a few more weeks :-)

Cheers,

Michael

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert,

Am 11.04.2018 um 18:51 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Tue, Apr 10, 2018 at 11:50 PM, Michael Schmitz <schmitz...@gmail.com> 
> wrote:
>>>> Short of a complete rewrite of the Zorro driver support code to be
>>>> closer to what PCI does, I don' see what can be done about the use of
>>>> Zorro IDs. I don't think such a rewrite is planned in the near future,
>>>> Geert?
>>>
>>> I think what Christoph means is the use of the define
>>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
>>> versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).
>>>
>>> We have a long list of ZORRO_PROD_* definitions in
>>> include/uapi/linux/zorro_ids.h because of historical reasons.  The list
>>> isn't really changing (no new IDs in git history) due to almost no new
>>> Zorro boards being made, unlike for PCI, where keeping an in-kernel list
>>> is a lot of work, and not desirable.
>>
>> now I see, thanks.
>>
>> I could change the device table to use ZORRO_ID(PHASE5, ...) style IDs
>> instead of the longish defines if you're OK with that.
> 
> I don't have a preference. If you think it makes the driver easier to read,
> go for it.

It does get rid of a few 'line over 80 characters' warnings from
checkpatch, and with comments in the device table describing what the
IDs mean, it isn't any less readable. Let's see ...

> Note that we can't get rid of the longish defines anyway, as they're in a
> uapi header file, so you're free to keep on using it.

I don't mind these defines remaining in the header file at all - the
only place they are now used is in the device table, and hardcoded IDs
are as good as the defines there, I think.

Have a look at the result in v8 - I can easily revert to the version
with the defines if that's the consensus, it's all in git after all.

Cheers,

Michael

[PATCH v8] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
Reviewed-by: Christoph Hellwig <h...@lst.de>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver pri

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert,

now I see, thanks.

I could change the device table to use ZORRO_ID(PHASE5, ...) style IDs
instead of the longish defines if you're OK with that.

The other changes have passed tests and I'd otherwise just send what I
have now, adding Christoph's Reviewed-by.

Cheers,

  Michael


On Tue, Apr 10, 2018 at 8:18 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Michael,
>
> On Tue, Apr 10, 2018 at 4:16 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
>> On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig <h...@infradead.org> wrote:
>>> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
>>>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>>>> m68k Amiga.
>>>> + {
>>>> + .id = 
>>>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>>>
>>> In PCI Land we've usually stopped using PCI IDs unless they are used
>>> in multiple
>
> (missing "places"?)
>
>> Short of a complete rewrite of the Zorro driver support code to be
>> closer to what PCI does, I don' see what can be done about the use of
>> Zorro IDs. I don't think such a rewrite is planned in the near future,
>> Geert?
>
> I think what Christoph means is the use of the define
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
> versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).
>
> We have a long list of ZORRO_PROD_* definitions in
> include/uapi/linux/zorro_ids.h because of historical reasons.  The list
> isn't really changing (no new IDs in git history) due to almost no new
> Zorro boards being made, unlike for PCI, where keeping an in-kernel list
> is a lot of work, and not desirable.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Christoph,

thanks for your review.

Short of a complete rewrite of the Zorro driver support code to be
closer to what PCI does, I don' see what can be done about the use of
Zorro IDs. I don't think such a rewrite is planned in the near future,
Geert?

I can certainly use board type enums as index into a driver data (or
feature) struct array made up from the current board config data. Adds
another level of indirection while still keeping the probe code
readable. Even allows to drop the last use of these long zorro_id
macros in the probe function, and that's a clear win.
I just wouldn't want to revert to patching up bits of config data
based on board type later - we had arrived at the current code after
quite some review with input from Finn and Geert.

May take a while to test any changes though (my test box appears to be
on the move at present).

Cheers,

  Michael



On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig <h...@infradead.org> wrote:
> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>> m68k Amiga.
>>
>> Code largely based on board specific parts of the old drivers (blz1230.c,
>> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
>> after the 2.6 kernel series for lack of maintenance) with contributions
>> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
>> bugfix by use of PIO in extended message in transfer).
>>
>> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
>> driver included in this patch.
>>
>> Use DMA transfers wherever possible, with board-specific DMA set-up
>> functions copied from the old driver code. Three byte reselection messages
>> do appear to cause DMA timeouts. So wire up a PIO transfer routine for
>> these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
>> target address for the message bytes but PIO requires a virtual address.
>> Substiute kernel virtual address esp->cmd_block in PIO transfer call if
>> DMA address is esp->cmd_block_dma and phase is message in.
>>
>> PIO code taken from mac_esp.c where the reselection timeout issue was
>> debugged and fixed first, with minor macro and function rename.
>>
>> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
>> Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
>>
>> ---
>>
>> Changes since v1:
>>
>> Fixed issues raised by Finn Thain in initial code review:
>>
>> - use KBUILD_MODNAME for driver name string.
>> - use pr_fmt() for pr_* format prefix.
>> - clean up DMA error reporting: clear error flag before each DMA
>>   operation, set error flag on PIO error. Don't test phase in dma_err hook.
>> - change confusing comment about semantics of read flag, add comments
>>   indicating DMA direction to DMA setup hooks.
>> - drop spurious braces around switch clauses.
>> - lift cfreq setting out of ID switch clauses.
>> - fix indentation.
>> - fix error codes on probe fail.
>> - drop check for board ID when unmapping DMA regs in error handling:
>>   the ioaddr > 0xff test already catches all cases where the DMA
>>   registers were ioremapped.
>> - dynamically alloc zorro_private_data.
>> - fix use of driver_data field (don't mix host and zorro_esp_priv
>>   pointers). Note: require esp pointer in zorro_esp_priv to find host
>>   pointer!
>> - back out phase bits changes to pio_cmd !write branch introduced
>>   to cope with ESP SELAS command. We don't use that code so keep
>>   it in sync with Finn's version.
>> - use esp_ops.dma_length_limit() to limit transfer size. After review
>>   of old driver code, use 0xff max transfer size throughout.
>>
>> Fixed issues raised by Geert Uytterhoven:
>>
>> - dynamically alloc zorro_private_data, store as device drvdata.
>> - store ctrl_data for CyberStormI in driver private data.
>> - use dma_sync_single_for_device() instead of cache_push/clear.
>> - handle case of duplicate board identity - check whether board is
>>   Zorro III or Zorro II (use ROM resource data for this). Also fix
>>   up DMA mask for Zorro II boards to 32 bits (these are really CPU
>>   expansion slot boards).
>> - remove zorro3 field from driver_data struct (now in private data).
>> - add braces around ambiguous if - else construct.
>> - use named structs instead of array for board config data.
>> - use scsi_option driver data flag for boards with optional ESP.
>>
>> Other improvements and bugfixes
>>
>> - fix Zorro device table error (duplicate ID used, also raised
>> 

[PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace related cleanup.

Changes since v3:

Finn  Thai

Re: [PATCH v6] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Finn,

Am 07.04.2018 um 22:19 schrieb Finn Thain:
> On Sat, 7 Apr 2018, Michael Schmitz wrote:
> 
>>>
>>>   CHECK   /home/fthain/src/kernel.org/linux/drivers/scsi/zorro_esp.c
>>> drivers/scsi/zorro_esp.c:274:14: warning: cast removes address space of 
>>> expression
>>> drivers/scsi/zorro_esp.c:712:14: warning: cast removes address space of 
>>> expression
>>>
>>> Why not just call writel() and get rid of the 't' temporary pointer?
>>
>> As far as I can see, that will invoke cpu_to_le32() on the data written.
> 
> Good point.
> 
> Since this is zorro card driver, why not use z_writel()?

Equally good point - that's what it's there for.

Cheers,

Michael

Re: [PATCH v6] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Finn,

Am 07.04.2018 um 12:59 schrieb Finn Thain:
> On Sat, 7 Apr 2018, Michael Schmitz wrote:
> 
>> Changes since v5:
>>
>> Christoph Hellwig:
>>
>> - fix comment style
>> - drop initialization to zero in driver data init
>> - fix alignment in struct declarations
>> - drop braces around asm macros
>> - change board_base type to void __iomem *
>> - drop unneeded void __iomem * cast from ZTWO_VADDR() use
>> - add comment to explain why ZTWO_VADDR() use is safe
>> - move board specific functions to per-board esp_ops
>>
> 
> The sparse warnings have changed to this:
> 
>   CHECK   /home/fthain/src/kernel.org/linux/drivers/scsi/zorro_esp.c
> drivers/scsi/zorro_esp.c:274:14: warning: cast removes address space of 
> expression
> drivers/scsi/zorro_esp.c:712:14: warning: cast removes address space of 
> expression
> 
> Why not just call writel() and get rid of the 't' temporary pointer?

As far as I can see, that will invoke cpu_to_le32() on the data written.
Geert?

Rewriting line this

*((__force unsigned long *)zep->board_base) = 0;

and this

*(__force unsigned long *)((addr & 0x00ff) + zep->board_base) = addr;

does suppress the warning and omits the temp pointer. But is that any
better?

Cheers,

Michael

[PATCH v6] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace related cleanup.

Changes since v3:

Finn  Thai

Re: [PATCH v5] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Christoph,

thank you for taking the time to review and comment - my responses
inline below.

Am 03.04.2018 um 19:45 schrieb Christoph Hellwig:
> Just a few style comments:
> 
>> +static int zorro_esp_irq_pending(struct esp *);
>> +static int cyber_esp_irq_pending(struct esp *);
>> +static int fastlane_esp_irq_pending(struct esp *);
> 
> Please avoid forward declarations wherever possible.
> 
>> +struct zorro_driver_data {
>> +const char *name;
>> +unsigned long offset;
>> +unsigned long dma_offset;
>> +int absolute;   /* offset is absolute address */
>> +int scsi_option;
>> +int (*irq_pending)(struct esp *esp);
>> +void (*dma_invalidate)(struct esp *esp);
>> +void (*send_dma_cmd)(struct esp *esp, u32 dma_addr, u32 esp_count,
>> + u32 dma_count, int write, u8 cmd);
>> +};
> 
> Please use different esp_driver_ops instances for the different board
> types.
> 
>> +static const struct zorro_driver_data cyberstormI_data = {
>> +.name = "CyberStormI",
>> +.offset = 0xf400,
>> +.dma_offset = 0xf800,
>> +.absolute = 0,
>> +.scsi_option = 0,
> 
> You can remove all the xero initializations on static data.
> Also please align the = signs with tabs in struct declarations.

Will do all that.

>> +static dma_addr_t zorro_esp_map_single(struct esp *esp, void *buf,
>> +  size_t sz, int dir)
>> +{
>> +return dma_map_single(esp->dev, buf, sz, dir);
>> +}
>> +
>> +static int zorro_esp_map_sg(struct esp *esp, struct scatterlist *sg,
>> +  int num_sg, int dir)
>> +{
>> +return dma_map_sg(esp->dev, sg, num_sg, dir);
>> +}
>> +
>> +static void zorro_esp_unmap_single(struct esp *esp, dma_addr_t addr,
>> +  size_t sz, int dir)
>> +{
>> +dma_unmap_single(esp->dev, addr, sz, dir);
>> +}
>> +
>> +static void zorro_esp_unmap_sg(struct esp *esp, struct scatterlist *sg,
>> +  int num_sg, int dir)
>> +{
>> +dma_unmap_sg(esp->dev, sg, num_sg, dir);
>> +}
> 
> These wrappers seem rather pointless.

They are nonetheless necessary, for two reasons as far as I can see:

ESP driver DMA ops hooks as defined in esp_scsi.h use struct esp * as
first parameter, not struct dev *. Changing that would require changing
all ESP based drivers (sun_esp.c, jazz_esp.c,  am53c974.c, sun3_esp.c,
mac_esp.c) and the driver core. I'd like to hear Dave's view on that.

The generic dma_map_* and dma_unmap_* ops are defined as inline
functions in dma-mapping.h. There may be a good reason for that choice.
You probably know better than me what the reason for inlining was.

All other ESP drivers use the same wrapper mechanism, and most of them
don't do more than dereference the struct device pointer and pass that
and the rest of parameters on to the generic or PCI API hooks. I see no
reason to deviate from that convention.

>> +/* PIO macros as used in mac_esp.c.
>> + * Note that addr and fifo arguments are local-scope variables declared
>> + * in zorro_esp_send_pio_cmd(), the macros are only used in that function,
>> + * and addr and fifo are referenced in each use of the macros so there
>> + * is no need to pass them as macro parameters.
>> + */
> 
> Please use normal Linux comment style:
> 
> /*
>  * Blah, blah, blah.
>  */

Thanks, done.

>> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
>> +{ \
>> +asm volatile ( \
>> + "1: moveb " operands "\n" \
>> + "   subqw #1,%1   \n" \
>> + "   jbne 1b   \n" \
>> + : "+a" (addr), "+r" (reg1) \
>> + : "a" (fifo)); \
>> +}
> 
> What is the point of the curly braces around the asm statement?

My overeager attempt at shutting up checkpatch ('need braces around
complex macro'). I'll revert to what Finn used in mac_esp.c (which is
where this code was taken from). That did pass review at the time, hope
this counts for something.

>> +static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
>> + u32 dma_count, int write, u8 cmd)
>> +{
>> +struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
>> +u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
>> +u8 phase = esp->sreg & ESP_STAT_PMASK;
>> +
>> +cmd &= ~ESP_CMD_DMA;
>> +
>> +if (write) {
> 
> It seems like this routine would benefit from being split into a read
> and a write one.

The ESP driver uses just a single send_dma_cmd() function, so we'd have
to conditionalize which of the two PIO functions to call from within
each board specific send_dma_cmd(). IMO that makes the code harder to
follow. (I wouldn't go as far as Finn and add another layer of
indirection, but the impact is much the same.)

Finn also provided another justification for using a single routine for
read and write - we plan to share that code between mac_esp.c and
zorro_esp.c so I'd rather keep it as close to the mac_esp.c version as
possible.

>> +// Blizzard 1230/60 

[PATCH v5] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace r

Re: [PATCH v4] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Kars,

thanks, I've fixed that typo.

Cheers,

Michael


Am 19.03.2018 um 03:51 schrieb Kars de Jong:
> Hi Michael,
> 
> 2018-03-17 2:16 GMT+01:00 Michael Schmitz <schmitz...@gmail.com>:
>> From: Michael Schmitz <schm...@debian.org>
>>
>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>> m68k Amiga.
>>
>> Code largely based on board specific parts of the old drivers (blz1230.c,
>> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
>> after the 2.6 kernel series for lack of maintenance) with contributions
>> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
>> bugfix by use of PIO in extended message in transfer).
>>
>> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
>> driver included in this patch.
>>
>> Use DMA transfers wherever possible, with board-specific DMA set-up
>> functions copied from the old driver code. Three byte reselection messages
>> do appear to cause DMA timeouts. So wire up a PIO transfer routine for
>> these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
>> target address for the message bytes but PIO requires a virtual address.
>> Substiute kernel virtual address esp->cmd_block in PIO transfer call if
>> DMA address is esp->cmd_block_dma and phase is message in.
>>
>> PIO code taken from mac_esp.c where the reselection timeout issue was
>> debugged and fixed first, with minor macro and function rename.
>>
>> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
>> Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
>> ---
>>
>> Changes since v1:
>>
>> Fixed issues raised by Finn Thain in initial code review:
>>
>> - use KBUILD_MODNAME for driver name string.
>> - use pr_fmt() for pr_* format prefix.
>> - clean up DMA error reporting: clear error flag before each DMA
>>   operation, set error flag on PIO error. Don't test phase in dma_err hook.
>> - change confusing comment about semantics of read flag, add comments
>>   indicating DMA direction to DMA setup hooks.
>> - drop spurious braces around switch clauses.
>> - lift cfreq setting out of ID switch clauses.
>> - fix indentation.
>> - fix error codes on probe fail.
>> - drop check for board ID when unmapping DMA regs in error handling:
>>   the ioaddr > 0xff test already catches all cases where the DMA
>>   registers were ioremapped.
>> - dynamically alloc zorro_private_data.
>> - fix use of driver_data field (don't mix host and zorro_esp_priv
>>   pointers). Note: require esp pointer in zorro_esp_priv to find host
>>   pointer!
>> - back out phase bits changes to pio_cmd !write branch introduced
>>   to cope with ESP SELAS command. We don't use that code so keep
>>   it in sync with Finn's version.
>> - use esp_ops.dma_length_limit() to limit transfer size. After review
>>   of old driver code, use 0xff max transfer size throughout.
>>
>> Fixed issues raised by Geert Uytterhoven:
>>
>> - dynamically alloc zorro_private_data, store as device drvdata.
>> - store ctrl_data for CyberStormI in driver private data.
>> - use dma_sync_single_for_device() instead of cache_push/clear.
>> - handle case of duplicate board identity - check whether board is
>>   Zorro III or Zorro II (use ROM resource data for this). Also fix
>>   up DMA mask for Zorro II boards to 32 bits (these are really CPU
>>   expansion slot boards).
>> - remove zorro3 field from driver_data struct (now in private data).
>> - add braces around ambiguous if - else construct.
>> - use named structs instead of array for board config data.
>> - use scsi_option driver data flag for boards with optional ESP.
>>
>> Other improvements and bugfixes
>>
>> - fix Zorro device table error (duplicate ID used, also raised
>>   by Kars de Jong).
>> - error code fixup in error handling path.
>> - add separate DMA setup for Blizzard 1230 II board.
>> - add support for Cyberstorm II board.
>> - add register structs and DMA setup for Zorro III Fastlane board,
>>   following logic from old fastlane.c driver. Wire up Fastlane DMA
>>   and interrupt status routines, map the necessary low 24 bit board
>>   address space used for DMA target address setting. Clean up DMA
>>   register space ioremap() branch for Zorro III boards (currently
>>   Fastlane only) to end confusion about what to do in error recovery.
>> - use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
>>   fastlane_esp_reset_dma() in DMA setup).
>> -

[PATCH v4] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
target address for the message bytes but PIO requires a virtual address.
Substiute kernel virtual address esp->cmd_block in PIO transfer call if
DMA address is esp->cmd_block_dma and phase is message in.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with minor macro and function rename.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
Reviewed-by: Finn Thain <fth...@telegraphics.com.au>
---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace r

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Finn, Geert,

In the interest of making minimal changes between the Mac and Amiga
versions, I'd leave the macros as they are, and add a comment to the
macro definitions stating that both addr and fifo are local-scope
variables in the only scope the macro is used in, to address reviewer's
concerns. Can you both live with that?

Placing the two macros in a suitable header in arch/m68k/include/asm/ so
Mac and Amiga can share the same code without duplicating it in two
files would be another option (that forces use of addr and fifo as
parameters), but let's not overengineer things. I don't expect any other
driver would need to share this code, or the PDMA macros also in the Mac
driver...

Other than that, I've implemented and tested all the suggested changes
and could post v4 of this patch now.

Cheers,

    Michael


Am 16.03.18 um 00:17 schrieb Finn Thain:
> On Wed, 14 Mar 2018, Michael Schmitz wrote:
>
>>> Please pass "addr" and "fifo" as macro parameters, too, so it's easier 
>>> for the reviewer to notice they are used.
>> Yes, I can do that (meaning Finn would need to make the same change to 
>> keep our versions in sync).
> Personally, I wouldn't want to change it. This wasn't an oversight. Maybe 
> if you (Geert) take a look at MAC_ESP_PDMA_LOOP, etc. this style might 
> make more sense.
>
> These are not macros in a header file that might get used in any random 
> scope. There is exactly one scope in which the macro appears, and the 
> variables that appear in the macro are simply the variables from that 
> scope.
>
> If you apply the rule, "the macro's parameters should exhaustively list 
> all the macro's symbols", then (in this case) you'd violate the rule 
> "Don't Repeat Yourself". And if you'd adhere to the latter rule then you'd 
> violate the former. So I chose the more readable option.
>
> The preprocessor allows us to pretend we are doing symbolic code 
> transformation, but that's not needed here. This was meant to be a textual 
> device.
>

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

Am 14.03.2018 um 21:30 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Wed, Mar 14, 2018 at 9:23 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
>> thanks for the review - largely uncontroversial except for the volatile...
> 
> The presence of volatile in drivers is always considered controversial ;-)

Yes indeed. I just hadn't looked at raw_io.h in ages (too many unhappy
memories), and didn't remember the __force volatile trick there.

>> Meaning writeb(val, reg) instead of reg = val?
>>
>> #define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))
>>
>> nicely hides the 'volatile' but suggests I also need to pass it a
>> pointer, so
>>
>> writeb((addr >> 24) & 0xff, >dma_addr)
> 
> Yes, you have to pass it an (__iomem) pointer.

Thanks.

>> I'll have to compare the assembly generated by the two versions before I
>> dare test that, but I'll give that a try. Liberal use of wmb() did fix
>> the miscompile but that just looked too ugly.
> 
> Using the macros should have the same effect due to the embedded volatile.

I tend to agree, but crashing elgar or trashing the SCSI filesystem
there isn't something I would like to risk (Adrian has been very
accommodating allowing me to test the driver there, but a reset tends to
be taking a few hours thanks to the time zone difference). So I'd rather
make sure nothing can go wrong.

Cheers,

Michael


> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Tuomas,

oddly enough, it does. How embarrassing...

Hope I got the names of the boards right this time.

Thanks for spotting this!

Cheers,

Michael

Am 14.03.2018 um 20:13 schrieb Vainikka Tuomas:
> The config description lists 53c700 devices, not ESP devices?
> 
> -Tuomas
> 
> From: linux-m68k-ow...@vger.kernel.org <linux-m68k-ow...@vger.kernel.org> on 
> behalf of Michael Schmitz <schmitz...@gmail.com>
> Sent: 12 March 2018 09:26:40
> To: linux-scsi@vger.kernel.org
> Cc: linux-m...@vger.kernel.org; da...@davemloft.net; ge...@linux-m68k.org; 
> fth...@telegraphics.com.au; Michael Schmitz; Michael Schmitz
> Subject: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c
> 
> From: Michael Schmitz <schm...@debian.org>
> 
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
> 
> Code largely based on board specific parts of the old drivers (blz1230.c,
> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
> after the 2.6 kernel series for lack of maintenance) with contributions
> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
> bugfix by use of PIO in extended message in transfer).
> 
> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
> driver included in this patch.
> 
> Use DMA transfers wherever possible, with board-specific DMA set-up
> functions copied from the old driver code. Three byte reselection messages
>  do appear to cause DMA timeouts. So wire up a PIO transfer routine for
> these instead.
> 
> PIO code taken from mac_esp.c where the reselection timeout issue was
> debugged and fixed first, with the following modifications:
> 
> esp_reselect_with_tag explicitly sets esp->cmd_block_dma as target address
> for the message bytes. Fixup to use kernel virtual address esp->cmd_block
> in PIO transfer if DMA address is esp->cmd_block_dma.
> 
> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
> 
> ---
> 
> Changes since v1:
> 
> Fixed issues raised by Finn Thain in initial code review:
> 
> - use KBUILD_MODNAME for driver name string.
> - use pr_fmt() for pr_* format prefix.
> - clean up DMA error reporting: clear error flag before each DMA
>   operation, set error flag on PIO error. Don't test phase in dma_err hook.
> - change confusing comment about semantics of read flag, add comments
>   indicating DMA direction to DMA setup hooks.
> - drop spurious braces around switch clauses.
> - lift cfreq setting out of ID switch clauses.
> - fix indentation.
> - fix error codes on probe fail.
> - drop check for board ID when unmapping DMA regs in error handling:
>   the ioaddr > 0xff test already catches all cases where the DMA
>   registers were ioremapped.
> - dynamically alloc zorro_private_data.
> - fix use of driver_data field (don't mix host and zorro_esp_priv
>   pointers). Note: require esp pointer in zorro_esp_priv to find host
>   pointer!
> - back out phase bits changes to pio_cmd !write branch introduced
>   to cope with ESP SELAS command. We don't use that code so keep
>   it in sync with Finn's version.
> - use esp_ops.dma_length_limit() to limit transfer size. After review
>   of old driver code, use 0xff max transfer size throughout.
> 
> Fixed issues raised by Geert Uytterhoven:
> 
> - dynamically alloc zorro_private_data, store as device drvdata.
> - store ctrl_data for CyberStormI in driver private data.
> - use dma_sync_single_for_device() instead of cache_push/clear.
> - handle case of duplicate board identity - check whether board is
>   Zorro III or Zorro II (use ROM resource data for this). Also fix
>   up DMA mask for Zorro II boards to 32 bits (these are really CPU
>   expansion slot boards).
> - remove zorro3 field from driver_data struct (now in private data).
> - add braces around ambiguous if - else construct.
> - use named structs instead of array for board config data.
> - use scsi_option driver data flag for boards with optional ESP.
> 
> Other improvements and bugfixes
> 
> - fix Zorro device table error (duplicate ID used, also raised
>   by Kars de Jong).
> - error code fixup in error handling path.
> - add separate DMA setup for Blizzard 1230 II board.
> - add support for Cyberstorm II board.
> - add register structs and DMA setup for Zorro III Fastlane board,
>   following logic from old fastlane.c driver. Wire up Fastlane DMA
>   and interrupt status routines, map the necessary low 24 bit board
>   address space used for DMA target address setting. Clean up DMA
>   register space ioremap() branch for Zorro III boards (currently
>   Fastlane only) to end confusion about what to do in e

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

thanks for the review - largely uncontroversial except for the volatile...

Am 14.03.2018 um 20:49 schrieb Geert Uytterhoeven:
>> +/* Blizzard 1230 DMA interface */
>> +
>> +struct blz1230_dma_registers {
>> +   volatile unsigned char dma_addr;/* DMA address  [0x] 
>> */
> 
> volatile considered harmful.

Yes, I saw that. I also saw gcc miscompile the DMA set-up (in
particular, the case where three bytes of the transfer address are
stuffed consecutively into the same DMA address register).

> If you would use proper *{read,write}*() accessors instead of direct
> assignments,
> you can drop the volatile's here.

Meaning writeb(val, reg) instead of reg = val?

#define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))

nicely hides the 'volatile' but suggests I also need to pass it a
pointer, so

writeb((addr >> 24) & 0xff, >dma_addr)

would do the same as

dregs->dma_addr  = (addr >> 24) & 0xff; ??

I'll have to compare the assembly generated by the two versions before I
dare test that, but I'll give that a try. Liberal use of wmb() did fix
the miscompile but that just looked too ugly.

>> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
>> +   { \
>> +   asm volatile ( \
>> +"1: moveb " operands "\n" \
>> +"   subqw #1,%1   \n" \
>> +"   jbne 1b   \n" \
>> +: "+a" (addr), "+r" (reg1) \
>> +: "a" (fifo)); \
>> +   }
> 
> Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
> the reviewer to notice they are used.

Yes, I can do that (meaning Finn would need to make the same change to
keep our versions in sync).

>> +   /* Switch to the correct the DMA routine and clock frequency. */
>> +   switch (ent->id) {
>> +   case ZORRO_PROD_PHASE5_BLIZZARD_2060:
>> +   zorro_esp_ops.send_dma_cmd = zorro_esp_send_blz2060_dma_cmd;
> 
> Please use function pointers in struct zorro_driver_data, so you don't need
> a switch() here (except for Fastlane vs. B1230II).

At that point, the Blizzard 1230 II zorro_driver_data has been replaced
by the Fastlane one so the correct function pointer would be used. I
didn't realize that also nicely solves my problem here.

Thanks!

Cheers,

Michael

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Finn,


Am 12.03.2018 um 22:04 schrieb Finn Thain:
>> +if (addr == esp->command_block_dma)
>> +addr = (u32) esp->command_block;
> 
> Since you've removed the alternative branch and phys_to_virt(), I suggest 
> you do this at function invocation... (see below)

Keeps it together with the phase and address tests, so easier to follow.


>> +static int zorro_esp_probe(struct zorro_dev *z,
>> +   const struct zorro_device_id *ent)
>> +{
>> +struct scsi_host_template *tpnt = _esp_template;
>> +struct Scsi_Host *host;
>> +struct esp *esp;
>> +struct zorro_driver_data *zdd;
>> +struct zorro_esp_priv *zep;
>> +unsigned long board, ioaddr, dmaaddr;
>> +int err;
>> +
>> +board = zorro_resource_start(z);
>> +zdd = (struct zorro_driver_data *)ent->driver_data;
>> +
>> +pr_info("%s found at address 0x%lx.\n", zdd->name, board);
>> +
>> +zep = kmalloc(sizeof(*zep), GFP_KERNEL);
>> +if (!zep) {
>> +pr_err("Can't allocate device private data!\n");
>> +return -ENOMEM;
>> +}
>> +
>> +/* let's figure out whether we have a Zorro II or Zorro III board */
>> +if ((z->rom.er_Type & ERT_TYPEMASK) == ERT_ZORROIII) {
>> +/* note this is a Zorro III board */
>> +if (board > 0xff)
>> +zep->zorro3 = 1;
> 
> The comment here seems to be redundant (?)

Not the only one.

> More importantly, I think you have to initialize zep->zorro3 = 0 in the 
> other branch, or else use kzalloc() instead of kmalloc() above.

Using kzalloc() saves the zep->error=0 initialization later on so it's a
win.

>> +case 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060:
>> +if (zep->zorro3) {
>> +/* Fastlane Zorro III board */
>> +/* map address space up to ESP address for DMA */
> 
> Same here?

I've left the comment explaining what the board low address space is
used for.

>> +zep->board_base = ioremap_nocache(board,
>> +FASTLANE_ESP_ADDR-1);
>> +if (!zep->board_base) {
>> +pr_err("Cannot allocate board address space\n");
>> +err = -ENOMEM;
>> +goto fail_free_host;
>> +}
>> +/* initialize DMA control shadow register */
>> +zep->ctrl_data = (FASTLANE_DMA_FCODE |
>> +  FASTLANE_DMA_EDI | FASTLANE_DMA_ESI);
>> +zorro_esp_ops.send_dma_cmd =
>> +zorro_esp_send_fastlane_dma_cmd;
>> +zorro_esp_ops.irq_pending  = fastlane_esp_irq_pending;
>> +zorro_esp_ops.dma_invalidate =
>> +fastlane_esp_dma_invalidate;
>> +} else {
>> +/* Blizzard 1230 II Zorro II board */

and that one - the board might be a Cyberstorm060 which would require
replacement of driver data, and replacement of the ESP register mapping.
Might need to clarify that here again...

>> +zorro_esp_ops.send_dma_cmd =
>> +zorro_esp_send_blz1230II_dma_cmd;

>> +//zorro_set_drvdata(z, host);
>> +
> 
> Can that be deleted?

Leftover from when private data were static, so can go now.


>> +static void zorro_esp_remove(struct zorro_dev *z)
>> +{
>> +/* equivalent to dev_get_drvdata(z->dev) */
> 
> If zorro_get_drvdata(z) needs a comment to explain it then why not just 
> write dev_get_drvdata(z->dev) instead?

dev_get_drvdata(>dev) (struct zorro device incorporates the whole
struct device, not a pointer), but yes.

>> +struct zorro_esp_priv *zep = zorro_get_drvdata(z);
>> +struct esp *esp = zep->esp;
>> +struct Scsi_Host *host = esp->host;
>> +
>> +scsi_esp_unregister(esp);
>> +
>> +/* Disable interrupts. Perhaps use disable_irq instead ... */
>> +
> 
> Can you clarify? free_irq() calls irq_shutdown()...

Which calls disable_irq() eventually if it's the last of shared
interrupts, true. disable_irq() would clearly be wrong here.

> 
> Thanks!
> 

My pleasure. I forgot to add your Reviewed-by tag - will do that for the
next version, OK?

Cheers,

Michael

[PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
 do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with the following modifications:

esp_reselect_with_tag explicitly sets esp->cmd_block_dma as target address
for the message bytes. Fixup to use kernel virtual address esp->cmd_block
in PIO transfer if DMA address is esp->cmd_block_dma. 

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

Issues raised by Finn Thain: 

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace related cleanup.

---
 drivers/scsi/Kconfig |   16 +
 drivers/scsi/Makefile|1 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn, Geert,

> +   /* We are passed DMA addresses i.e. physical addresses, but must 
> use
> +* kernel virtual addresses here, so remap to virtual. This is 
> easy
> +* enough for the case of residual bytes of an extended message in
> +* transfer - we know the address must be esp->command_block_dma.
> +* In other cases, hope that phys_to_virt() works ...
> +*/
> +   if (addr == esp->command_block_dma)
> +   addr = (u32) esp->command_block;
> +   else
> +   addr = (u32) phys_to_virt(addr);

 To avoid having a need for phys_to_virt(), you should remember the 
 addresses passed to/returned from dma_map_*().
>>>
>>> Interesting - can we be certain that only one mapping is being used at 
>>> any one time?

Tests have revealed that we can't rely on a single mapping active at any
one time (it's rare, but I get some messages printed when I log
zorro_esp_map_single() being called with a mapping already saved).

>>>
>>
>> I don't know how Geert's suggestion would work in this case. But I think 
> 
> I suppose storing the virtual address in the driver private data struct,
> and looking up that stored virtual address in the PIO transfer function.
> 
>> we covered this problem back in December: 
>> https://marc.info/?l=linux-m68k=151365452606870
>>
>> (That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.)
> 
> Yes, rereading that thread now suggests we had explored a few options
> but the issue of autosense commands was making it a little too complicated.

We have no need to find out the correct virtual address for a given DMA
mapping as long as we only use PIO for message byte transfer. I'll leave
finding a proper solution for a rainy day (or rather, a few rainy
weeks). Anyone familiar with Auckland will know what that means :-)

Cheers,

Michael

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Tuomas,


Am 06.03.2018 um 04:31 schrieb Tuomas Vainikka:
>>> I think you are talking about esp->regs? For esp->dma_regs, the
>>> ioremap is
>>> conditional on ent->id, but the unmap is not.
>> The details of the ioremap are conditional on the ID, but the fact
>> that the ioremap happens (and hence esp->dma_regs is an ioremapped
>> address) is not. All Zorro-3 boards have to have both their regs and
>> dma_regs remapped.
>>
>> What's confusing is that there is only a single Zorro-3 board
>> currently supported by the driver. Others will be added and I"ll use a
>> switch statement to pick the appropriate address based on the ID. That
>> might make it clearer.
> 
> Fastlane might be the only Z3 SCSI board that has the chip.

Good to know - I've rewritten the probe code to check for the type of
board (Z2 or Z3) based on the ROM data, and make the ioremap/iounmap
conditional on that.

Cheers,

Michael


> 
> -Tuomas
> 
>>
>> +}
>> +
>> +static void zorro_esp_remove_one(struct zorro_dev *z)
>> +{
>> + struct Scsi_Host *host = zorro_get_drvdata(z);
>> + struct esp *esp = shost_priv(host);
>> +
>> + scsi_esp_unregister(esp);
>> +
>> + /* Disable interrupts. Perhaps use disable_irq instead ... */
>> +
>> + free_irq(host->irq, esp);
>> + dma_free_coherent(esp->dev, 16,
>> +   esp->command_block,
>> +   esp->command_block_dma);
>> +
>> + if (host->base > 0xff) {
>> + iounmap(esp->dma_regs);
> Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
 I can't - ent->id is not available here...
>>> Maybe store ent->id in the private struct to get around that?
>> Yes, that could be done. I still think it's not needed.
>>
>> Cheers,
>>
>>Michael
>>
>>
>>> -- 
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

fine, I'll rely on the base address and the z->rom.er_Type value to
pick the correct config data.

Cheers,

  Michael


On Wed, Mar 7, 2018 at 9:06 PM, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Hi Michael,
>
> On Wed, Mar 7, 2018 at 8:55 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
>> OK, in that case I'll need to work out something similar to the test for
>> optional SCSI function on the Blizzard 1230/1260 to find out what board
>> I have when dealing with the duplicate Fastlane/Blizzard1230II ID.
>>
>> Is the board base address as returned by zorro_resource_start() reliable
>> to distinguish between Zorro II and Zorro III boards?
>
> The board base address is assigned by AmigaOS based on the values in the
> Expansion ROM (mainly ExpansionRom.er_Type) on the board.
> More specifically, AmigaOS creates struct ConfigDev from struct ExpansionRom.
> So yes, it must be reliable.
>
>> Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
>>> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitz...@gmail.com> 
>>> wrote:
>>>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
>>>> corrected that in the meantime.
>>>>
>>>> Fastlane / Blizzard 1230_II distinction is something I an not quite
>>>> sure about - does the probe function get called twice if the device
>>>> table contains the same ID twice but with different driver_data
>>>> contents?
>>>
>>> No, the probe function gets called on the first match only.
>>> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

OK, in that case I'll need to work out something similar to the test for
optional SCSI function on the Blizzard 1230/1260 to find out what board
I have when dealing with the duplicate Fastlane/Blizzard1230II ID.

Is the board base address as returned by zorro_resource_start() reliable
to distinguish between Zorro II and Zorro III boards?

Cheers,

Michael

Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
>> corrected that in the meantime.
>>
>> Fastlane / Blizzard 1230_II distinction is something I an not quite
>> sure about - does the probe function get called twice if the device
>> table contains the same ID twice but with different driver_data
>> contents?
> 
> No, the probe function gets called on the first match only.
> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

Am 07.03.2018 um 13:54 schrieb Finn Thain:
> On Wed, 7 Mar 2018, Michael Schmitz wrote:
> 
>> The major obstacle now seems to be dynamic allocation of the driver 
>> private data and storing a pointer to that in a way that it can be 
>> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
>> causes the module load to crash ...
> 
> I've just noticed that most esp drivers do this:
> 
> static int esp_foo_probe(struct platform_device *dev)
> {
>   ...
>   esp->dev = dev;
>   ...
> }
> 
> But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
> in a pointer to a struct platform_device being used where a pointer to a 
> struct device should be used (i.e. dma_*() calls). I will look into fixing 
> this up. sun_esp.c doesn't have this problem, but the other drivers do.
> 
> I don't think any of that applies to your zorro_esp code because the 
> version you sent does this,
> 
>   esp->dev = >dev;
> 
> which seems fine to me. But it could end up more convenient to use the 
> sun_esp approach and set esp->dev = z.

It just adds another dereferencing step in the dma_map functions which
we only need to do once here.

But sun_esp also does

dev_set_drvdata(>dev, esp);

i.e. not only does it store the struct device pointer in the esp struct
(by indirection through the platform device struct), but also the struct
esp pointer in struct device.

> I suspect that the problem with zorro_esp is that sometimes you use the 
> esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
> other times you use it for the struct zorro_driver_data pointer. (I think 
> I see now why you put the esp pointer in struct zorro_driver_data.)

The zorro_esp_priv pointer (zorro_esp_driver_data pointer are only used
to store board specific config data needed for probe), but yes, you've
found my error.

I overlooked that dev_set_drvdata() and zorro_set_drvdata() will store
their payload in the same struct device instance. Using one for struct
zorro_esp_priv and the other for struct Scsi_host is just asking for
trouble. Thanks for jogging my memory ...

Since there is no other place for me to put driver private data, I need
to change the use of that field to point to the current zorro_esp_priv
instance, and retrieve the struct esp pointer from there. I can retrieve
the host pointer from  struct esp so all should be well.

This is a little unusual so I better add a few comments to save the next
maintainer from unnecessary headache.

> If you like, email the current version to me or push it to a repo 
> somewhere and I'll take a look at it.

I'll take you up on that offer another time, but with the use of
dev->driver_data fixed, the driver no longer crashes. I shouldn't
hack kernel code in a rush...

Now on to mangle the rest of the issues raised in the review...

Cheers,

Michael

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

On Tue, Mar 6, 2018 at 8:48 PM, Geert Uytterhoeven  wrote:
> BTW, please call the probe/remove functions zorro_esp_probe() resp.
> zorro_esp_remove().

Fair enough.

 +   if (!host) {
 +   pr_err(PFX "No host detected; board configuration 
 problem?\n");
 +   goto out_free;
 +   }
>>
>> here. But I can add the err=-NOMEM here.
>
> After out_free it returns fixed -ENODEV ;-)

Fixed that in my tree already. But I now have six err=-ENOMEM peppered
all over the probe code. Still, if it makes it more readable ...

> Doing "err = -ENOMEM" here, and returning err at the end is better, as
> it propagates meaningful error codes.

Yes, I've belatedly realized that now.

The major obstacle now seems to be dynamic allocation of the driver
private data and storing a pointer to that in a way that it can be
retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep)
causes the module load to crash ...

Cheers,

  Michael


>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

I'll leave the unused !write branch in the original condition. I'd
rather keep the address conversion inside the PIO code than
duplicating it in each DMA setup routine though.

You had asked what manual I had used a while ago: it's
http://bitsavers.trailing-edge.com/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf

Cheers,

  Michael


On Tue, Mar 6, 2018 at 9:37 PM, Finn Thain <fth...@telegraphics.com.au> wrote:
> On Tue, 6 Mar 2018, Michael Schmitz wrote:
>
>> The whole !write branch will never be executed, and I could just omit it
>> entirely for now, or leave it as it was in the Mac driver.
>>
>
> We could make use of the !write branch in zorro_esp, even if it was only
> to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But
> let's keep them in sync.
>
> --
>
>> Cheers,
>>
>>   Michael
>>

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

thanks, will comment on a few points that were not already raised by Finn below.

On Tue, Mar 6, 2018 at 12:29 AM, Geert Uytterhoeven
 wrote:

>> +static struct zorro_driver_data {
>> +   const char *name;
>> +   unsigned long offset;
>> +   unsigned long dma_offset;
>> +   int absolute;
>> +   int zorro3; /* offset is absolute address */
>
> zorro3 is unused.

Fastlane support isn't yet included, I expect to use it there.

>
>> +} zorro_esp_driver_data[] = {
>> +   { .name = "CyberStormI", .offset = 0xf400, .dma_offset = 0xf800,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "CyberStormII", .offset = 0x1ff03, .dma_offset = 0x1ff43,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Blizzard 2060", .offset = 0x1ff00, .dma_offset = 0x1ffe0,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Blizzard 1230", .offset = 0x8000, .dma_offset = 0x1,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Blizzard 1230II", .offset = 0x1, .dma_offset = 
>> 0x10021,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Fastlane", .offset = 0x101, .dma_offset = 0x141,
>> + .absolute = 0, .zorro3 = 1 },
>> +   { 0 }
>
> I think it's better to not use an array here, but individual structs:
>
> static struct zorro_driver_data cyberstorm_data = ...
> static struct zorro_driver_data cyberstorm2_data = ...
> ...
>
> That makes it easier to review the references from zorro_esp_zorro_tbl[]
> below.

Might have avoided the error of using _esp_driver_data[0] twice ...

>> +static unsigned char ctrl_data;/* Keep backup of the stuff written
>> +* to ctrl_reg. Always write a copy
>> +* to this register when writing to
>> +* the hardware register!
>> +*/
>
> This should be part of the device's zorro_esp_priv.

It's only used in the cyber_dma_setup, and I not actually read
anywhere else. Might as well be on the stack instead of static...

>> +static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
>> +u32 dma_count, int write, u8 cmd)
>> +{
>> +   struct zorro_esp_priv *zep = ZORRO_ESP_GET_PRIV(esp);
>> +   u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
>> +   u8 phase = esp->sreg & ESP_STAT_PMASK;
>> +
>> +   cmd &= ~ESP_CMD_DMA;
>> +   zep->error = 0;
>> +
>> +   /* We are passed DMA addresses i.e. physical addresses, but must use
>> +* kernel virtual addresses here, so remap to virtual. This is easy
>> +* enough for the case of residual bytes of an extended message in
>> +* transfer - we know the address must be esp->command_block_dma.
>> +* In other cases, hope that phys_to_virt() works ...
>> +*/
>> +   if (addr == esp->command_block_dma)
>> +   addr = (u32) esp->command_block;
>> +   else
>> +   addr = (u32) phys_to_virt(addr);
>
> To avoid having a need for phys_to_virt(), you should remember the addresses
> passed to/returned from dma_map_*().

Interesting - can we be certain that only one mapping is being used at
any one time?

>
> if you assign the address to a different variable with the proper
> type, you don't
> need the cast below
>
> +   if (write) {
> +   u8 *dst = (u8 *)addr;
>
> ... here.
>
>> +   } else {
>
> The read case doesn't use addr?

It's hidden in ZORRO_ESP_PIO_LOOP and ZORRO_ESP_PIO_FILL, but it's
there. Might be the reason for the u32 type?

>> +static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
>> +   u32 esp_count, u32 dma_count, int write, u8 cmd)
>> +{
>> +   struct blz1230_dma_registers *dregs =
>> +   (struct blz1230_dma_registers *) (esp->dma_regs);
>> +   u8 phase = esp->sreg & ESP_STAT_PMASK;
>> +
>> +   if (phase == ESP_MIP) {
>> +   zorro_esp_send_pio_cmd(esp, addr, esp_count,
>> +   dma_count, write, cmd);
>> +   return;
>> +   }
>> +
>> +   BUG_ON(!(cmd & ESP_CMD_DMA));
>> +
>> +   if (write)
>> +   cache_clear(addr, esp_count);
>> +   else
>> +   cache_push(addr, esp_count);
>
> dma_sync_*()
>
>> +static int zorro_esp_init_one(struct zorro_dev *z,
>> +  const struct zorro_device_id *ent)
>> +{
>> +   struct scsi_host_template *tpnt = _esp_template;
>> +   struct Scsi_Host *host;
>> +   struct esp *esp;
>> +   struct zorro_driver_data *zdd;
>> +   struct zorro_esp_priv *zep;
>> +   unsigned long board, ioaddr, dmaaddr;
>> +   int err = -ENOMEM;
>
> Initialization not needed.

The initialized err variable is used when bailing out ..

>> +
>> +   board = zorro_resource_start(z);
>> +   

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Kars,

Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
corrected that in the meantime.

Fastlane / Blizzard 1230_II distinction is something I an not quite
sure about - does the probe function get called twice if the device
table contains the same ID twice but with different driver_data
contents?

Cheers,

  Michael


On Mon, Mar 5, 2018 at 9:02 PM, Kars de Jong <jo...@linux-m68k.org> wrote:
> 2018-03-04 0:54 GMT+01:00 Michael Schmitz <schmitz...@gmail.com>:
>> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
>> +   .driver_data = (unsigned long)_esp_driver_data[0],
>> +   },
>> +   {
>> +   .id = 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>> +   .driver_data = (unsigned long)_esp_driver_data[0],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
>> +   .driver_data = (unsigned long)_esp_driver_data[1],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
>> +   .driver_data = (unsigned long)_esp_driver_data[2],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
>> +   .driver_data = (unsigned long)_esp_driver_data[3],
>> +   },
>> +   {
>> +   .id = 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>> +   .driver_data = (unsigned long)_esp_driver_data[4],
>> +   },
>> +   { 0 }
>> +};
>> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
>
>
> The ID ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
> is used at index 1 and index 5.
> I'm guessing only the first one will be detected.
>
> Regards,
>
> Kars.

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

On Mon, Mar 5, 2018 at 2:01 PM, Finn Thain <fth...@telegraphics.com.au> wrote:
> On Mon, 5 Mar 2018, Michael Schmitz wrote:
>
>> >> +fail_unmap_dma_regs:
>> >> + if (ioaddr > 0xff)
>> >> + iounmap(esp->dma_regs);
>> >
>> > I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?
>>
>> On second thought - no, I don't. the ID check above only determines what
>> Zorro-3 address is ioremapped, but in each case where ioaddr > 0xff,
>> something will have been mapped at this point.
>>
>
> I think you are talking about esp->regs? For esp->dma_regs, the ioremap is
> conditional on ent->id, but the unmap is not.

The details of the ioremap are conditional on the ID, but the fact
that the ioremap happens (and hence esp->dma_regs is an ioremapped
address) is not. All Zorro-3 boards have to have both their regs and
dma_regs remapped.

What's confusing is that there is only a single Zorro-3 board
currently supported by the driver. Others will be added and I"ll use a
switch statement to pick the appropriate address based on the ID. That
might make it clearer.

>
>> >> +}
>> >> +
>> >> +static void zorro_esp_remove_one(struct zorro_dev *z)
>> >> +{
>> >> + struct Scsi_Host *host = zorro_get_drvdata(z);
>> >> + struct esp *esp = shost_priv(host);
>> >> +
>> >> + scsi_esp_unregister(esp);
>> >> +
>> >> + /* Disable interrupts. Perhaps use disable_irq instead ... */
>> >> +
>> >> + free_irq(host->irq, esp);
>> >> + dma_free_coherent(esp->dev, 16,
>> >> +   esp->command_block,
>> >> +   esp->command_block_dma);
>> >> +
>> >> + if (host->base > 0xff) {
>> >> + iounmap(esp->dma_regs);
>> >
>> > Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
>>
>> I can't - ent->id is not available here...
>
> Maybe store ent->id in the private struct to get around that?

Yes, that could be done. I still think it's not needed.

Cheers,

  Michael


>
> --

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

>> >> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
>> >> +  _esp_private_data[(esp->host->host_no)])
>> >> +
>> >
>> > How do you know that host_no won't exceed the array bounds?
>> > Why not use dev_{set,get}_drvdata(esp->dev)? -- much as mac_esp uses
>> > platform_{set,get}_drvdata() here.
>>
>> I don't think you can have more than 8 Zorro cards in an Amiga.
>
> The host_no range is not limited to just zorro cards. It's also libata
> hosts, USB Attached Storage hosts, scsi_debug etc.

Max. libata hosts is one, no USB host adapters I know of, leaves
scsi_debug. But I'll get rid of this if at all possible.

>> > I suspect that the mac_esp PIO code should work fine here unchanged.
>> > If so, let's avoid a new variation on that code if that's possible (?)
>>
>> The Mac PIO code did not work unchanged here, that's why I checked the
>> interrupt bit raised and changed the mask. But that was only for one
>> quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire
>> driver without DMA. Doesn't happen in DMA mode.
>>
>> You would have seen the error only with such a special target, and
>> probably not in PDMA mode...
>>
>
> The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how
> to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds
> like this bug would show up too given the right kind of target. If so, I
> will need to patch mac_esp too.

It did show up in PIO mode so it's a matter of having the right target
to trigger it.
dmesg  on elgar says:

[   26.96] scsi host1: esp
[   27.27] scsi 1:0:1:0: Direct-Access IBMRAID  DFHSS2F9337
  4I4I PQ: 0 ANSI: 2
[   27.28] scsi target1:0:1: Beginning Domain Validation
[   27.36] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15)
[   27.38] scsi target1:0:1: Domain Validation skipping write tests
[   27.39] scsi target1:0:1: Ending Domain Validation
[   27.45] scsi 1:0:2:0: CD-ROMSONY CD-ROM CDU-561
  1.9m PQ: 0 ANSI: 2 CCS
[   27.46] scsi target1:0:2: Beginning Domain Validation
[   27.58] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15)
[   27.60] scsi target1:0:2: Domain Validation skipping write tests
[   27.62] scsi target1:0:2: Ending Domain Validation
[   27.75] scsi host1: scsi scan: INQUIRY result too short (5), using 36

To the best of my knowledge, the CD-ROM triggered that error. Looking
at the NCR53CF94_96 databook, select with attention and stop is used
when multiple message bytes need to be transferred (example: sync
negotiation - that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used
in the ESP core which switches to using this selection mode).

One message byte is transferred before the ESP raises both
ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will
still be in message out phase at that stage.

u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ?
ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) );

would set the correct interrupt status mask to avoid detecting this
condition as error. Doing sync negotiation is fairly basic - I wonder
why you haven't seen this before. Ah - the Mac driver supports sync
transfers only when using PDMA, that's why.

The description of 'select with attention' also states the same two
interrupt bits are set, so maybe the bug is elsewhere. According to
the manual, only the first message byte should be loaded into the FIFO
when stopping for additional message bytes. We load the entire message
instead...

>
>> Regarding your other comment on this code: I'll have to review what
>> phase was present when the select with attention and stop condition
>> occurred,
>
> Thanks. That's what I'd prefer to use in mac_esp (if it works).
>
>> but setting the mask up front according to the bus phase would make a
>> lot of sense.
>>
>
> The idea is that ESP_INTR_FDONE is an error when the phase is not
> appropriate. In the version you sent, the loop permits ESP_INTR_BSERV |
> ESP_INTR_FDONE regardless of phase.

Deciding whether an error exists just based on the phase might be
impossible now I come to think of it - the combination of
ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection
commands. We might need to look at additional information to figure
out whether getting  ESP_INTR_FDONE in message out phase was an error.

This is getting a little academic for zorro_esp - if a particular
board needs to do PIO all the time, we need a way to set the
ESP_FLAG_USE_FIFO flag in the core driver instead. Much safer.

>
>> > WARN_ON is preferred. Please see Johannes Thumshirn's message from last
>> > week,
>> > https://www.spinics.net/lists/linux-scsi/msg117919.html
>>
>> Well, should we ever see this condition trigger (I have never!), I don't
>> think there is a way we can recover from it. We expect a DMA command
>> (DMA i.e. physical address), how can we reliably determine whether the
>> calling code just 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

>> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
>> + *
>> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
>> + *
>> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
>> + *   migration to ESP SCSI core
>
> You can blame me for some of this ;-)

I'll add a line here pointing out the source for the PIO code, so
others will have a chance to keep these in sync if needs be.


>> +fail_unmap_dma_regs:
>> + if (ioaddr > 0xff)
>> + iounmap(esp->dma_regs);
>
> I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?

On second thought - no, I don't. the ID check above only determines
what Zorro-3 address is ioremapped, but in each case where ioaddr >
0xff, something will have been mapped at this point.

>> +}
>> +
>> +static void zorro_esp_remove_one(struct zorro_dev *z)
>> +{
>> + struct Scsi_Host *host = zorro_get_drvdata(z);
>> + struct esp *esp = shost_priv(host);
>> +
>> + scsi_esp_unregister(esp);
>> +
>> + /* Disable interrupts. Perhaps use disable_irq instead ... */
>> +
>> + free_irq(host->irq, esp);
>> + dma_free_coherent(esp->dev, 16,
>> +   esp->command_block,
>> +   esp->command_block_dma);
>> +
>> + if (host->base > 0xff) {
>> + iounmap(esp->dma_regs);
>
> Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?

I can't - ent->id is not available here. But again, if host->base is
outside the Zorro-2 space, the DMA register mapping has been done
through ioremap() and needs to be undone here.

Cheers,

  Michael

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

thanks for your review!

Am 04.03.2018 um 15:55 schrieb Finn Thain:
> On Sun, 4 Mar 2018, Michael Schmitz wrote:
> 
>> From: Michael Schmitz <schm...@debian.org>
>>
>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>> m68k Amiga.
>>
> 
> Nice work!
> 
> Shouldn't both patches be combined into one? The first patch can't be used 
> without this one.

I can certainly do that, if that's the preferred way.

>> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
>> + *
>> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
>> + *
>> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
>> + *   migration to ESP SCSI core
> 
> You can blame me for some of this ;-)

Oops - did acknowledge you in the commit message but forgot to do it in
the code ... or did you mean something else?

>> +#define DRV_MODULE_NAME "zorro_esp"
>> +#define PFX DRV_MODULE_NAME ": "
> 
> This should be pr_fmt(). mac_esp used PFX because it is older than the 
> pr_*() stuff.
> 
> Also, KBUILD_MODNAME might be appropriate here? This idiom is common:
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

OK, I'll change the logging to current standard.

>> +/*
>> + * private data used for PIO
>> + */
>> +struct zorro_esp_priv {
>> +struct esp *esp;
> 
> You don't need this backpointer -- every time you use the zorro_esp_priv 
> struct, you already have the esp pointer.

True - not sure what I was anticipating to use that for anymore.

>> +int error;
>> +} zorro_esp_private_data[8];
>> +
> 
> Many of these 8 structs probably aren't going to get used, which seems 
> wasteful. I'd allocate the private struct dynamically, as mac_esp does.

I had tried that, but couldn't get it stored in either the esp or the
zorro_device structs.

>> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
>> +_esp_private_data[(esp->host->host_no)])
>> +
> 
> How do you know that host_no won't exceed the array bounds?
> Why not use dev_{set,get}_drvdata(esp->dev)? -- much as mac_esp uses
> platform_{set,get}_drvdata() here.

I don't think you can have more than 8 Zorro cards in an Amiga. And I
had tried zorro_{set,get}_drvdata() to no avail (the private data pinter
is used for something else there). Will try dev_{set,get}_drvdata() now.


>> +/* We are passed DMA addresses i.e. physical addresses, but must use
>> + * kernel virtual addresses here, so remap to virtual. This is easy
> 
> IMHO "convert" is probably more clear than "remap" here.

Right.


>> +esp->ireg = zorro_esp_read8(esp, ESP_INTRPT);
>> +/* This is tricky - ~ESP_INTR_BSERV is the wrong mask
>> + * a ESP_CMD_SELAS command which also generates
>> + * ESP_INTR_FDONE! Use ~(ESP_INTR_BSERV|ESP_INTR_FDONE)
>> + * since ESP_INTR_FDONE is not raised by any other
>> + * error condition. Unfortunately, this is still not
>> + * sufficient to make the single message byte transfer
>> + * of ESP_CMD_SELAS work ...
> 
> Is that comment correct? This is the !write branch, that is, read acccess 
> from memory. But tag bytes move in the other direction for ESP_CMD_SELAS.
> 
> I suspect that the mac_esp PIO code should work fine here unchanged. If 
> so, let's avoid a new variation on that code if that's possible (?)

The Mac PIO code did not work unchanged here, that's why I checked the
interrupt bit raised and changed the mask. But that was only for one
quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire
driver without DMA. Doesn't happen in DMA mode.

You would have seen the error only with such a special target, and
probably not in PDMA mode...

Regarding your other comment on this code: I'll have to review what
phase was present when the select with attention and stop condition
occurred, but setting the mask up front according to the bus phase would
make a lot of sense.

>> +BUG_ON(!(cmd & ESP_CMD_DMA));
> 
> WARN_ON is preferred. Please see Johannes Thumshirn's message from last 
> week,
> https://www.spinics.net/lists/linux-scsi/msg117919.html

Well, should we ever see this condition trigger (I have never!), I don't
think there is a way we can recover from it. We expect a DMA command
(DMA i.e. physical address), how can we reliably determine whether the
calling code just forgot to set the bit, or really passed us a kernel
virtual address?

Since I've never seen this trigger, may as well remove the test.

[PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

Code largely based on board specific parts of the old drivers (blz1230.c,
blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
after the 2.6 kernel series for lack of maintenance) with contributions
by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
bugfix by use of PIO in extended message in transfer).

Use DMA transfers wherever possible, with board-specific DMA set-up
functions copied from the old driver code. Three byte reselection messages
 do appear to cause DMA timeouts. So wire up a PIO transfer routine for
these instead.

PIO code taken from mac_esp.c where the reselection timeout issue was
debugged and fixed first, with the following modifications:

esp_reselect_with_tag explicitly sets esp->cmd_block_dma as target address
for the message bytes. Fixup to use kernel virtual address esp->cmd_block
in PIO transfer if DMA address is esp->cmd_block_dma. Use phys_to_virt(addr)
to determine kernel virtual address in all other cases (this works on m68k).

A 'select with attention and stop' command raises ESP_INTR_FDONE, so
adjust interrupt mask used to end PIO transfer accordingly.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
---
 drivers/scsi/zorro_esp.c |  785 ++
 1 files changed, 785 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/zorro_esp.c

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
new file mode 100644
index 000..03c4ad2
--- /dev/null
+++ b/drivers/scsi/zorro_esp.c
@@ -0,0 +1,785 @@
+/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
+ *
+ * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
+ *
+ * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
+ *   migration to ESP SCSI core
+ */
+/*
+ * ZORRO bus code from:
+ */
+/*
+ * Detection routine for the NCR53c710 based Amiga SCSI Controllers for Linux.
+ * Amiga MacroSystemUS WarpEngine SCSI controller.
+ * Amiga Technologies/DKB A4091 SCSI controller.
+ *
+ * Written 1997 by Alan Hourihane <al...@fairlite.demon.co.uk>
+ * plus modifications of the 53c7xx.c driver to support the Amiga.
+ *
+ * Rewritten to use 53c700.c by Kars de Jong <jo...@linux-m68k.org>
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "esp_scsi.h"
+
+#define DRV_MODULE_NAME "zorro_esp"
+#define PFX         DRV_MODULE_NAME ": "
+
+MODULE_AUTHOR("Michael Schmitz <schm...@debian.org>");
+MODULE_DESCRIPTION("Amiga Zorro NCR5C9x (ESP) driver");
+MODULE_LICENSE("GPL");
+
+#define DMA_WRITE 0x8000
+
+static struct zorro_driver_data {
+   const char *name;
+   unsigned long offset;
+   unsigned long dma_offset;
+   int absolute;
+   int zorro3; /* offset is absolute address */
+} zorro_esp_driver_data[] = {
+   { .name = "CyberStormI", .offset = 0xf400, .dma_offset = 0xf800,
+ .absolute = 0, .zorro3 = 0 },
+   { .name = "CyberStormII", .offset = 0x1ff03, .dma_offset = 0x1ff43,
+ .absolute = 0, .zorro3 = 0 },
+   { .name = "Blizzard 2060", .offset = 0x1ff00, .dma_offset = 0x1ffe0,
+ .absolute = 0, .zorro3 = 0 },
+   { .name = "Blizzard 1230", .offset = 0x8000, .dma_offset = 0x1,
+ .absolute = 0, .zorro3 = 0 },
+   { .name = "Blizzard 1230II", .offset = 0x1, .dma_offset = 0x10021,
+ .absolute = 0, .zorro3 = 0 },
+   { .name = "Fastlane", .offset = 0x101, .dma_offset = 0x141,
+ .absolute = 0, .zorro3 = 1 },
+   { 0 }
+};
+
+static struct zorro_device_id zorro_esp_zorro_tbl[] = {
+   {
+   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
+   .driver_data = (unsigned long)_esp_driver_data[0],
+   },
+   {
+   .id = 
ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
+   .driver_data = (unsigned long)_esp_driver_data[0],
+   },
+   {
+   .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
+   .driver_data = (unsigned long)_esp_driver_data[1],
+   },
+   {
+   .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
+   .driver_data = (unsigned long)_esp_driver_data[2],
+   },
+   {
+   .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
+   .driver_data = (unsigned long)_esp_driver_data[3],
+   },
+   {
+   .id = 
ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
+   .driver_data = (unsigned long)_esp_driver_data[4],
+   

[PATCH 1/2] m68k/amiga - Zorro ESP SCSI Makefile/Kconfig support

[Michael Schmitz]

From: Michael Schmitz <schm...@debian.org>

Add Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver (combining old blz1230, blz2060, cyberstorm and fastlane
SCSI drivers that were removed for lack of maintenance after 2.6)

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
---
 drivers/scsi/Kconfig  |   16 
 drivers/scsi/Makefile |1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 8a739b7..c7d337f 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1462,6 +1462,22 @@ config SCSI_ZORRO7XX
  accelerator card for the Amiga 1200,
- the SCSI controller on the GVP Turbo 040/060 accelerator.
 
+config SCSI_ZORRO_ESP
+   tristate "Zorro ESP SCSI support"
+   depends on ZORRO && SCSI
+   select SCSI_SPI_ATTRS
+   help
+ Support for various ESP-based SCSI controllers on Zorro
+ expansion boards for the Amiga.
+ This includes:
+   - the Amiga 4091 Zorro III SCSI-2 controller,
+   - the MacroSystem Development's WarpEngine Amiga SCSI-2 controller
+ (info at
+ <http://www.lysator.liu.se/amiga/ar/guide/ar310.guide?FEATURE5>),
+   - the SCSI controller on the Phase5 Blizzard PowerUP 603e+
+ accelerator card for the Amiga 1200,
+   - the SCSI controller on the GVP Turbo 040/060 accelerator.
+
 config ATARI_SCSI
tristate "Atari native SCSI support"
depends on ATARI && SCSI
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index fcfd28d..6f13c03 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_INFINIBAND_ISER) += libiscsi.o
 obj-$(CONFIG_ISCSI_BOOT_SYSFS) += iscsi_boot_sysfs.o
 obj-$(CONFIG_SCSI_A4000T)  += 53c700.o a4000t.o
 obj-$(CONFIG_SCSI_ZORRO7XX)+= 53c700.o zorro7xx.o
+obj-$(CONFIG_SCSI_ZORRO_ESP)   += esp_scsi.o   zorro_esp.o
 obj-$(CONFIG_A3000_SCSI)   += a3000.o  wd33c93.o
 obj-$(CONFIG_A2091_SCSI)   += a2091.o  wd33c93.o
 obj-$(CONFIG_GVP11_SCSI)   += gvp11.o  wd33c93.o
-- 
1.7.0.4

[PATCH 0/2] Revive Amiga Zorro ESP SCSI driver

[Michael Schmitz]

At long last, revive the driver for m68k Amiga Zorro boards based on
the Qlogic ESP SCSI chip, which were in-tree as separate drivers
(blz1230.c, blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c) until
the rewrite of the ESP core code after 2.6.x.

A previous rewrite of the Zorro ESP SCSI drivers did not cope with tagged
commands, and needed an ugly workaround to disable tagged queueing. After
much poking around with config2 and config3 registers to no avail, the   
driver was never submitted.

Inspired by Finn Thain's PDMA fixes to the Mac ESP driver, the same strategy
of using PIO transfer for the short extended messages used to transfer the
queue tag was used for the Zorro driver. Evidently, the DMA engine on the
Zorro boards cannot handle short transfers of a few bytes only, and use of
PIO for these is the only option. 

I have decided to ignore checkpatch warnings about long lines and the use
of volatile in this patch - the long lines are due to excessive length
macros for Zorro board IDs defined elsewhere in the kernel. I have decided
to keep   volatile qualifiers for DMA engine registers because I found gcc   
to miscompile the DMA setup routines (omitting multiple register stores)
otherwise.

Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup

[Michael Schmitz]

Ondrej,

could this be a partial write (target did not transfer the last byte)?

One would suppose the chip posts a phase mismatch in that case ...

Cheers,

Michael


Am 27.06.2017 um 18:28 schrieb Ondrej Zary:
> On Monday 26 June 2017, Ondrej Zary wrote:
>> On Monday 26 June 2017 09:30:33 Finn Thain wrote:
>>> Ondrej, would you please test this new series?
>>>
>>> Changed since v1:
>>> - PDMA transfer residual is calculated earlier.
>>> - End of DMA flag check is now polled (if there is any residual).
>>>
>>> Changed since v2:
>>> - Bail out of transfer loops when Gated IRQ gets asserted.
>>> - Make udelay conditional on board type.
>>> - Drop sg_tablesize patch due to performance regression.
>>>
>>>
>>> Finn Thain (1):
>>>   g_NCR5380: Cleanup comments and whitespace
>>>
>>> Ondrej Zary (3):
>>>   g_NCR5380: Fix PDMA transfer size
>>>   g_NCR5380: End PDMA transfer correctly on target disconnection
>>>   g_NCR5380: Re-work PDMA loops
>>>
>>>  drivers/scsi/g_NCR5380.c | 239
>>> +++ 1 file changed, 116
>>> insertions(+), 123 deletions(-)
> 
> BTW. I've probably found the DTC write corruption.
> Added the following check (13 is host buffer index register) - and it 
> triggers 
> sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't 
> see how the value could ever be odd. Looks like a bug in the chip.
> The index register corrupts during the transfer, not after IRQ or timeout. 
> The 
> same check at beginning of pwrite() did not trigger.
> 
> The index register is not writable so we must(?) reset the PDMA engine to 
> recover. However, this quick attempt to fix does not work, maybe we should 
> reload the block count and continue?
> 
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct 
> NCR5380_hostdata *hostdata,
> goto out_wait;
> }
> }
> -
> +   idx = NCR5380_read(13);
> +   if (idx != 0) {
> +   printk("host idx=%d, start=%d\n", idx, start);
> +   NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
> +   NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
> +   goto out_wait;
> +   }
> if (hostdata->io_port && hostdata->io_width == 2)
> outsw(hostdata->io_port + hostdata->c400_host_buf,
> src + start, 64);
> 
> 

Re: [PATCH] m68k/atari: atari_scsi.c - use m68k_realnum_memory for FastRAM test

[Michael Schmitz]

Hi Christian

Am 06.06.2017 um 07:17 schrieb Christian T. Steigies:
> On Mon, Jun 05, 2017 at 07:37:59PM +1200, Michael Schmitz wrote:
>> m68k_num_memory is unsuitable to test for the presence of FastRAM
>> on CT60 if the kernel is located in FastRAM: in arch/m68k/mm/motorola.c
>> the ST-RAM chunk is skipped and m68k_num_memory is decremented in this
>> case. m68k_realnum_memory still contains the actual number of RAM chunks
>> so use that.
>>
>> Tested by Christian Steiges on his Falcon with kernel loaded in FastRAM
>> - could you please reply with your Tested-by tag, Christian?
> 
> Sure, do I need to sign it or is this ok?
> 
> Tested-by: Christian T. Steigies <c...@debian.org>
>> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>

That's OK - we know it was you :-)

Martin has already applied it - thanks!

Cheers,

Michael

[PATCH] m68k/atari: atari_scsi.c - use m68k_realnum_memory for FastRAM test

[Michael Schmitz]

m68k_num_memory is unsuitable to test for the presence of FastRAM
on CT60 if the kernel is located in FastRAM: in arch/m68k/mm/motorola.c
the ST-RAM chunk is skipped and m68k_num_memory is decremented in this
case. m68k_realnum_memory still contains the actual number of RAM chunks
so use that.

Tested by Christian Steiges on his Falcon with kernel loaded in FastRAM
- could you please reply with your Tested-by tag, Christian?

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
---
 drivers/scsi/atari_scsi.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index f792420..a75feeb 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -776,7 +776,7 @@ static int __init atari_scsi_probe(struct platform_device 
*pdev)
 * from/to alternative Ram.
 */
if (ATARIHW_PRESENT(ST_SCSI) && !ATARIHW_PRESENT(EXTD_DMA) &&
-   m68k_num_memory > 1) {
+   m68k_realnum_memory > 1) {
atari_dma_buffer = atari_stram_alloc(STRAM_BUFFER_SIZE, "SCSI");
if (!atari_dma_buffer) {
pr_err(PFX "can't allocate ST-RAM double buffer\n");
-- 
1.7.0.4

Re: [PATCH] scsi/mac_esp: Replace bogus memory barrier with spinlock

[Michael Schmitz]

Martin,

looks good to me, so:

Reviewed-By: Michael Schmitz <schmitz...@gmail.com>


Am 25.04.2017 um 10:29 schrieb Martin K. Petersen:
> 
> Finn,
> 
>> Commit da244654c66e ("[SCSI] mac_esp: fix for quadras with two esp chips")
>> added mac_scsi_esp_intr() to handle the IRQ lines from a pair of on-board
>> ESP chips (a normal shared IRQ did not work).
>>
>> Proper mutual exclusion was missing from that patch. This patch fixes
>> race conditions between comparison and assignment of esp_chips[]
>> pointers.
> 
> Ondrej/Michael: Mind reviewing this change?
> 

Re: [PATCH] scsi/mac_esp: Replace bogus memory barrier with spinlock

[Michael Schmitz]

Hi Martin,

I must have missed that one - where was it posted to?

Cheers,

Michael


Am 25.04.2017 um 10:29 schrieb Martin K. Petersen:
> 
> Finn,
> 
>> Commit da244654c66e ("[SCSI] mac_esp: fix for quadras with two esp chips")
>> added mac_scsi_esp_intr() to handle the IRQ lines from a pair of on-board
>> ESP chips (a normal shared IRQ did not work).
>>
>> Proper mutual exclusion was missing from that patch. This patch fixes
>> race conditions between comparison and assignment of esp_chips[]
>> pointers.
> 
> Ondrej/Michael: Mind reviewing this change?
> 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Michael Schmitz]

Martin, Finn,

I'll get to that on the weekend - Auckland Anniversary Day coming up so
plenty of time.

Cheers,

Michael

Am 26.01.2017 um 12:25 schrieb Martin K. Petersen:
>> "Finn" == Finn Thain  writes:
> 
> Finn> Michael, Ondrej, can I get you to review/test please?
> 
> Pretty please!
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Finn,

Am 01.11.2016 um 12:47 schrieb Finn Thain:
> 
> On Tue, 1 Nov 2016, Michael Schmitz wrote:
> 
>>> I had tried to set that bit in zorro_esp_slave_configure but had not 
>>> done a proper job of it - I'd only set esp->config3 and forgot to set 
>>> tp->esp_config3. Time to retest this ...
>>
>> I don't think it's quite that easy - the ESP_CONFIG3_TENB bit needs to 
>> be set for all targets if at least one SCSI-2 target is on the bus and 
>> we allow dosconnecting, no?
> 
> I think ESP_CONFIG3_TENB is for FAS100A and FASHME. The bug here is on 
> ESP236 and FAS236, so ESP_CONFIG3_TBMS would be the relevant bit.

I stand corrected. Err... confused.

When setting ESP_CONFIG3_TBMS, should we set ESP_CONFIG3_GTM as well?

> The bit gets set when ESP_CONFIG2_SCSI2ENAB gets set (as in David's 
> patch) so we then need to avoid clobbering it, since ESP_CONFIG3_TBMS == 
> ESP_CONFIG3_EWIDE. I think we have to test for HME to avoid this clash.

I'd want to set these bits for ESP236 and FAS236 only, so no clash with
HME. As you found out, ESP_CONFIG3_TBMS aka ESP_CONFIG3_EWIDE gets
clobbered on bus reset cleanup unconditionally.

Cheers,

Michael

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Finn,

> I had tried to set that bit in zorro_esp_slave_configure but had not
> done a proper job of it - I'd only set esp->config3 and forgot to set
> tp->esp_config3. Time to retest this ...

I don't think it's quite that easy - the ESP_CONFIG3_TENB bit needs to
be set for all targets if at least one SCSI-2 target is on the bus and
we allow dosconnecting, no?

Adrian - any chance I can get access to elgar again for some more testing?

Cheers,

  Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Finn,

Am 30.10.2016 um 15:33 schrieb Finn Thain:
> 
> On Sat, 29 Oct 2016, I wrote:
> 
>>
>> On Sun, 13 Apr 2014, David Miller wrote:
>>
>>>
>>> But oddly in the NCR53CX docs:
>>>
>>> 
>>> http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/NCR53C9X.txt
>>>
>>> it speaks as if ESP_CONFIG3_TMS and ESP_CONFIG3_TENB are merely finer 
>>> grained versions of config2 register setting ESP_CONFIG2_SCSI2ENAB, 
>>> which enables both features.
>>
>> Yes, so setting the ESP_CONFIG2_SCSI2ENAB bit is correct. The only 
>> problem is, doesn't the ESP_CONFIG3_TMS bit get cleared again later, 
>> when the CONFIG3 register is written to?
>>
> 
> Nevermind my question. To falsify my own theory, I see that 
> ESP_CONFIG3_TMS == ESP_CONFIG3_FCLK, and thus esp_scsi does actually set 
> the relevant bit, and therefore "the FSC can receive 3-byte messages 
> during businitiated select with ATN."

>From my reading of the quoted docs,ESP_CONFIG3_TMS is valid for FAS100A
(and later?). What esp_scsi sets (for FAS236 chips) is fast SCSI clock
mode. The relevant bit for three byte message support for Mac and Amiga
ESP hardware should be ESP_CONFIG3_TENB.

I had tried to set that bit in zorro_esp_slave_configure but had not
done a proper job of it - I'd only set esp->config3 and forgot to set
tp->esp_config3. Time to retest this ...

Cheers,

Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/12] Fixes, cleanup and g_NCR5380_mmio/g_NCR5380 merger

[Michael Schmitz]

> This patch series has fixes for compatibility, reliability and
> performance issues and some cleanup. It also includes a new version
> of Ondrej Zary's patch that merges g_NCR5380_mmio into g_NCR5380.
> 
> I've tested this patch series on a Powerbook 180. If someone would
> test some of the other platforms that would be very helpful. All
> drivers were compile-tested.
> 
> Changes since v1:
> - rebased on 4.9/scsi-queue
> - added reviewed-by tags
> - tweaked the order of struct members in patch 7/12
> 
> 
> Finn Thain (12):
>   scsi/g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio drivers
>   scsi/cumana_1: Remove unused cumanascsi_setup() function
>   scsi/atari_scsi: Make device register accessors re-enterant
>   scsi/ncr5380: Simplify register polling limit
>   scsi/ncr5380: Increase register polling limit
>   scsi/ncr5380: Improve hostdata struct member alignment and
> cache-ability
>   scsi/ncr5380: Store IO ports and addresses in host private data
>   scsi/ncr5380: Use correct types for device register accessors
>   scsi/ncr5380: Pass hostdata pointer to register polling routines
>   scsi/ncr5380: Expedite register polling
>   scsi/ncr5380: Use correct types for DMA routines
>   scsi/ncr5380: Suppress unhelpful "interrupt without IRQ bit" message

Tested on Atari SCSI (Falcon) - no regressions.

Tested-by: Michael Schmitz <schmitz...@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: status of NCR5380-based ISA drivers

[Michael Schmitz]

Christoph,

I test the atari_scsi front end - haven't got any ISA cards handy.
Probably no ISA slots on my PC main board either ...

Cheers,

Michael

Am 12.09.2016 um 05:12 schrieb Christoph Hellwig:
> Hi all,
> 
> you seem to the currently active NCR580 cabal.  Which frontends to NCR5380
> do you test at the moment, or could you test?
> 
> NCR5380-based ISA drivers make up a significant part of those drivers still
> using scsi_module.c after it's 15 year deprecation period, and I'd like to
> either move them to this century or to the dust bin.
> 
> The drivers in question are: dtc, g_NCR5380, pas16 and t128.
> 
> Thanks,
>   Christoph
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/22] ncr5380: Eliminate macros, reduce code duplication, fix bugs etc

[Michael Schmitz]

Finn,

tested successfully on Atari Falcon, so:

Tested-by: Michael Schmitz <schmitz...@gmail.com>

Am 14.03.2016 um 17:27 schrieb Finn Thain:
> This patch series has more macro elimination and some tweaks to the
> DMA hooks so that all the wrapper drivers can share the same core
> DMA algorithm. This resolves the major discrepancies between the two
> core drivers, which relate to code conditional on the REAL_DMA and
> PSEUDO_DMA macros.
> 
> After all the wrapper drivers agree on the DMA hook api, the core driver
> fork gets resolved. NCR5380.c is adopted by atari_scsi and sun3_scsi and
> atari_NCR5380.c is then deleted.
> 
> Historically, the 5380 drivers suffered from over-use of conditional
> compilation, which caused the compile-time configuration space to explode,
> leading to core driver code that was practically untestable, unmaintainable
> and difficult to reason about. It also prevented driver modules from
> sharing object code.
> 
> Along with REAL_DMA, REAL_DMA_POLL and PSEUDO_DMA, most of the remaining
> macros are also eradicated, such as CONFIG_SCSI_GENERIC_NCR53C400,
> SUPPORT_TAGS, DONT_USE_INTR, AUTOPROBE_IRQ and BIOSPARAM.
> 
> Also in this patch series, some duplicated documentation is removed and
> the PDMA implementation in mac_scsi finally gets fixed.
> 
> This patch series was tested by exercising the dmx3191d and mac_scsi modules
> on suitable hardware. Help with driver testing on ISA and Atari hardware
> is sought as I don't have any (likewise RiscPC ecards and Sun 3 hardware).
> 
> ---
>  Documentation/scsi/g_NCR5380.txt   |   17 
>  Documentation/scsi/scsi-parameters.txt |   11 
>  drivers/scsi/Kconfig   |   11 
>  drivers/scsi/NCR5380.c |  661 
>  drivers/scsi/NCR5380.h |  145 -
>  drivers/scsi/arm/cumana_1.c|   25 
>  drivers/scsi/arm/oak.c |   22 
>  drivers/scsi/atari_NCR5380.c   | 2676 
> -
>  drivers/scsi/atari_scsi.c  |  142 -
>  drivers/scsi/dmx3191d.c|   10 
>  drivers/scsi/dtc.c |   27 
>  drivers/scsi/dtc.h |7 
>  drivers/scsi/g_NCR5380.c   |  143 -
>  drivers/scsi/g_NCR5380.h   |   26 
>  drivers/scsi/mac_scsi.c|  239 +-
>  drivers/scsi/pas16.c   |   27 
>  drivers/scsi/pas16.h   |5 
>  drivers/scsi/sun3_scsi.c   |   48 
>  drivers/scsi/t128.c|   19 
>  drivers/scsi/t128.h|7 
>  20 files changed, 636 insertions(+), 3632 deletions(-)
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 34/78] atari_NCR5380: Use arbitration timeout

[Michael Schmitz]

Hi Geert,

On Mon, Jan 25, 2016 at 9:05 PM, Geert Uytterhoeven
 wrote:

>>> Perhaps this is an ARAnyM quirk?
>
>> The MR_ARBITRATE bit should remain set until the driver clears it (or the
>> reset logic clears it). But it looks like aranym simply discards writes to
>> the mode register, such that reads always return 0.
>>
>> Compare
>>   http://sourceforge.net/p/aranym/code/ci/master/tree/src/ncr5380.cpp
>> with the MAME/MESS emulated device
>>   https://github.com/mamedev/mame/blob/master/src/devices/machine/ncr5380.cpp
>>
>> I don't know what the Hatari emulator does.
>>
>> In principle I think that Linux drivers should not carry workarounds for
>> emulators.
>
> Please consider ARAnyM is the current m68k workhorse, so it would be
> nice to handle this someway.
>
> Alternatively, we need to fix ARAnyM, or can make the creation of the
> atari_scsi platform device conditional on not running under ARAnyM.

Should be possible based on the machine cookie, unless that gets
munged by kernel startup code.

(Making the 5380 emulation a bit more complete such as in the MAME
source snippet would be my preference, too)

Cheers,

  Michael


>
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/77] More fixes, cleanup and modernization for NCR5380 drivers

[Michael Schmitz]

Hi Finn,

I've tested this series thoroughly on my Atari Falcon - no regressions,
runs stable and is quite responsive. No SCSI lock-ups that had plagued
the old driver (before your rewrites).

Please add my

Tested-by: Michael Schmitz <schmitz...@gmail.com>

Cheers,

Michael


Am 22.12.15 um 14:17 schrieb Finn Thain:
> Like my previous work on the NCR5380 drivers, this patch series has bug
> fixes, code cleanup and modernization. These drivers suffer from mistakes,
> poor style and neglect and this long series addresses the worst of it,
> covering all ten wrapper drivers and both of the core driver forks. The
> combined size of the drivers is reduced by over 700 LoC.
>
> This series continues to reduce divergence between the two core driver
> forks, often by copying a bug fix from one to the other. Most patches are
> larger for having to keep the two forks in sync. Making the same change to
> both is churn if one of them is to be removed but neither can be as yet.
> By the end of this series the diff between the two forks is minimal, so it
> becomes clear what caused the fork and what can be done about it.
>
> This patch series did benefit from scripts/checkpatch.pl but not too much.
> Decades ago, these drivers started out with 4-space tabs and if the 80
> column limit were to be strictly enforced now, it would require adding new
> functions and shortening identifiers. I would defer this sort of activity
> until after the fork has been resolved.
>
> All patches to all NCR5380 drivers (x86, ARM, m68k) have been compile-
> tested. The mac_scsi, dmx3191d, g_NCR5380 and atari_scsi modules were
> regression tested on suitable hardware.
>
> Changes since v1:
> - Patch 8 omits a pointless assignment.
> - Patch 10 gets a better error message.
> - Patch 20 drops the WQ_CPU_INTENSIVE flag.
> - Patch 21 adds timing calibration for the register polling loop.
> - Patch 49 is replaced by a new one that removes FLAG_DTC3181E.
> - Patch 72 by Ondrej Zary was added to fix pseudo DMA on 53C400.
>
> Changes since v2:
> - Patch 21 has better calibration with low HZ values.
> - Patch 57 no longer attempts to dereference a NULL pointer on Atari.
> - Patch 66 initializes max_sectors in the host templates.
> - Patches 73 thru 77 by Ondrej Zary were added with additional fixes for
>   53C400-compatible cards.
>
> ---
>  drivers/scsi/Kconfig |   17 
>  drivers/scsi/NCR5380.c   | 2868 
> +++
>  drivers/scsi/NCR5380.h   |   87 -
>  drivers/scsi/arm/cumana_1.c  |   31 
>  drivers/scsi/arm/oak.c   |   27 
>  drivers/scsi/atari_NCR5380.c | 2292 +++---
>  drivers/scsi/atari_scsi.c|  102 -
>  drivers/scsi/dmx3191d.c  |   33 
>  drivers/scsi/dtc.c   |  115 -
>  drivers/scsi/dtc.h   |   45 
>  drivers/scsi/g_NCR5380.c |  408 +++---
>  drivers/scsi/g_NCR5380.h |   66 
>  drivers/scsi/mac_scsi.c  |  117 -
>  drivers/scsi/pas16.c |  116 -
>  drivers/scsi/pas16.h |   40 
>  drivers/scsi/sun3_scsi.c |  141 --
>  drivers/scsi/t128.c  |  102 -
>  drivers/scsi/t128.h  |   39 
>  18 files changed, 2957 insertions(+), 3689 deletions(-)
>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 66/77] ncr5380: Fix soft lockups

[Michael Schmitz]

I'd like to think that, too - probably true for the Atari TT SCSI case
(can do scatter-gather, can do more than one command per LUN). Worse
for the Falcon SCSI which is the only one I can test (no
scatter-gather, one command per LUN, interrupt shared with IDE and IDE
driver locked out while SCSI command handled).

But that only affects balancing of I/O between IDE and SCSI drivers.
Is that what you are worried about, Alan?

Happy to test whether limiting max_sectors makes a difference in the DMA case.

Cheers,

  Michael



On Wed, Dec 23, 2015 at 2:47 AM, Finn Thain  wrote:
>
> On Tue, 22 Dec 2015, One Thousand Gnomes wrote:
>
>> On Tue, 22 Dec 2015 12:18:44 +1100 Finn Thain
>>  wrote:
>>
>> > Because of the rudimentary design of the chip, it is necessary to poll
>> > the SCSI bus signals during PIO and this tends to hog the CPU. The
>> > driver will accept new commands while others execute, and this causes
>> > a soft lockup because the workqueue item will not terminate until the
>> > issue queue is emptied.
>> >
>> > When exercising dmx3191d using sequential IO from dd, the driver is
>> > sent 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the
>> > rate is is only about 300 KiB/s, so these are long-running commands.
>> > And although PDMA may run at several MiB/s, interrupts are disabled
>> > for the duration of the transfer.
>> >
>> > Fix the unresponsiveness and soft lockup issues by calling
>> > cond_resched() after each command is completed and by limiting
>> > max_sectors for drivers that don't implement real DMA.
>>
>> Is there a reason for not doing some limiting in the DMA case too. A
>> 512K write command even with DMA on a low end 68K box introduces a
>> second of latency before another I/O can be scheduled ?
>
> The DMA case is the atari_scsi case. I'd like to think that atari_scsi
> would have only the latency issues that might be expected from any SCSI-2
> host adapter driver.
>
> Unlike PDMA, interrupts are not disabled for these DMA transfers. Note
> that this patch isn't really relevant to DMA, because the main loop
> iterates only when done == 0, that is, !hostdata->dmalen.
>
> --
>
>>
>> Alan
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/71] More fixes, cleanup and modernization for NCR5380 drivers

[Michael Schmitz]

Hi Finn,

>>>
>>> I have compile-tested all patches to all NCR5380 drivers (x86, ARM, 
>>> m68k) and regression tested mac_scsi and dmx3191d modules on suitable 
>>> hardware. Testing the mac_scsi and dmx3191d modules provides only 
>>> limited coverage. It would be good to see some testing of ISA cards 
>>> and Sun 3 and Atari hardware too (I don't have any).
>>
>> I have some NCR5380 ISA cards and can test them.
>>
> 
> Thanks Ondrej. I've no idea which ISA drivers are presently working in 
> mainline. Finding regressions may be more difficult than usual ;-)
> 
> Michael, Sam: only atari_scsi and sun3_scsi implement DMA support, so some
> testing of either driver would be helpful.

One way or another, I'll test this series or get someone to test a
kernel I built.

Cheers,

Michael



--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/71] atari_NCR5380: Remove RESET_BOOT, CONFIG_ATARI_SCSI_TOSHIBA_DELAY and CONFIG_ATARI_SCSI_RESET_BOOT

[Michael Schmitz]

Hi Finn,

Am 18.11.2015 um 21:35 schrieb Finn Thain:

> The bus reset may raise an interrupt. That would be new behaviour for
> atari_scsi only when CONFIG_ATARI_SCSI_RESET_BOOT=n. The ST DMA interrupt
> is not assigned to atari_scsi at this stage, so
> CONFIG_ATARI_SCSI_RESET_BOOT=y may well be problematic already.

I can confirm that the bus reset at boot has never been problematic in
the past. It's been enabled in my kernels as long as I've used the
driver (must be getting close to 20 years now).

Cheers,


Michael


> Regardless, do_reset() now raises and clears the interrupt within
> local_irq_save/restore which should avoid problems.
> 
> Signed-off-by: Finn Thain 
> 
> ---
>  drivers/scsi/Kconfig |   17 ---
>  drivers/scsi/NCR5380.c   |   17 +--
>  drivers/scsi/NCR5380.h   |1 
>  drivers/scsi/atari_NCR5380.c |   22 +-
>  drivers/scsi/atari_scsi.c|   60 +--
>  drivers/scsi/mac_scsi.c  |   65 
> ++-
>  drivers/scsi/sun3_scsi.c |   47 ---
>  7 files changed, 51 insertions(+), 178 deletions(-)
> 
> Index: linux/drivers/scsi/Kconfig
> ===
> --- linux.orig/drivers/scsi/Kconfig   2015-11-18 19:25:56.0 +1100
> +++ linux/drivers/scsi/Kconfig2015-11-18 19:33:15.0 +1100
> @@ -1624,23 +1624,6 @@ config ATARI_SCSI
> ST-DMA, replacing ACSI).  It does NOT support other schemes, like
> in the Hades (without DMA).
>  
> -config ATARI_SCSI_TOSHIBA_DELAY
> - bool "Long delays for Toshiba CD-ROMs"
> - depends on ATARI_SCSI
> - help
> -   This option increases the delay after a SCSI arbitration to
> -   accommodate some flaky Toshiba CD-ROM drives. Say Y if you intend to
> -   use a Toshiba CD-ROM drive; otherwise, the option is not needed and
> -   would impact performance a bit, so say N.
> -
> -config ATARI_SCSI_RESET_BOOT
> - bool "Reset SCSI-devices at boottime"
> - depends on ATARI_SCSI
> - help
> -   Reset the devices on your Atari whenever it boots.  This makes the
> -   boot process fractionally longer but may assist recovery from errors
> -   that leave the devices with SCSI operations partway completed.
> -
>  config MAC_SCSI
>   tristate "Macintosh NCR5380 SCSI"
>   depends on MAC && SCSI=y
> Index: linux/drivers/scsi/atari_NCR5380.c
> ===
> --- linux.orig/drivers/scsi/atari_NCR5380.c   2015-11-18 19:33:13.0 
> +1100
> +++ linux/drivers/scsi/atari_NCR5380.c2015-11-18 19:33:15.0 
> +1100
> @@ -674,13 +674,14 @@ static void prepare_info(struct Scsi_Hos
>"base 0x%lx, irq %d, "
>"can_queue %d, cmd_per_lun %d, "
>"sg_tablesize %d, this_id %d, "
> -  "flags { %s}, "
> +  "flags { %s%s}, "
>"options { %s} ",
>instance->hostt->name, instance->io_port, instance->n_io_port,
>instance->base, instance->irq,
>instance->can_queue, instance->cmd_per_lun,
>instance->sg_tablesize, instance->this_id,
>hostdata->flags & FLAG_TAGGED_QUEUING ? "TAGGED_QUEUING " : "",
> +  hostdata->flags & FLAG_TOSHIBA_DELAY  ? "TOSHIBA_DELAY "  : "",
>  #ifdef DIFFERENTIAL
>"DIFFERENTIAL "
>  #endif
> @@ -860,6 +861,7 @@ static int __init NCR5380_init(struct Sc
>  
>  static int NCR5380_maybe_reset_bus(struct Scsi_Host *instance)
>  {
> + struct NCR5380_hostdata *hostdata = shost_priv(instance);
>   int pass;
>  
>   for (pass = 1; (NCR5380_read(STATUS_REG) & SR_BSY) && pass <= 6; 
> ++pass) {
> @@ -878,6 +880,14 @@ static int NCR5380_maybe_reset_bus(struc
>   case 4:
>   shost_printk(KERN_ERR, instance, "bus busy, attempting 
> reset\n");
>   do_reset(instance);
> + /* Wait after a reset; the SCSI standard calls for
> +  * 250ms, we wait 500ms to be on the safe side.
> +  * But some Toshiba CD-ROMs need ten times that.
> +  */
> + if (hostdata->flags & FLAG_TOSHIBA_DELAY)
> + msleep(2500);
> + else
> + msleep(500);
>   break;
>   case 6:
>   shost_printk(KERN_ERR, instance, "bus locked solid\n");
> @@ -1493,12 +1503,10 @@ static int NCR5380_select(struct Scsi_Ho
>* a minimum so we'll udelay ceil(1.2)
>*/
>  
> -#ifdef CONFIG_ATARI_SCSI_TOSHIBA_DELAY
> - /* ++roman: But some targets (see above :-) seem to need a bit more... 
> */
> - udelay(15);
> -#else
> - udelay(2);
> -#endif
> + if 

Re: [PATCH 10/71] atari_NCR5380: Remove RESET_BOOT, CONFIG_ATARI_SCSI_TOSHIBA_DELAY and CONFIG_ATARI_SCSI_RESET_BOOT

[Michael Schmitz]

Hi Finn,

Am 19.11.2015 um 17:05 schrieb Finn Thain:
> w
> On Thu, 19 Nov 2015, Michael Schmitz wrote:
> 
>> Hi Finn,
>>
>> Am 18.11.2015 um 21:35 schrieb Finn Thain:
>>
>>> The bus reset may raise an interrupt. That would be new behaviour for 
>>> atari_scsi only when CONFIG_ATARI_SCSI_RESET_BOOT=n. The ST DMA 
>>> interrupt is not assigned to atari_scsi at this stage, so 
>>> CONFIG_ATARI_SCSI_RESET_BOOT=y may well be problematic already.
>>
>> I can confirm that the bus reset at boot has never been problematic in 
>> the past. It's been enabled in my kernels as long as I've used the 
>> driver (must be getting close to 20 years now).
> 
> That's good to know. I'm not sure why it was configurable in the first 
> place (long delays?). The new algorithm (the one I copied from NCR5380.c) 

The longer delays (and possibly a reset at boot) were only necessary for
certain CD-ROM drives. I don't think I have ever seen such a device, and
it's a bit unlikely any of these still survive. Reset at boot before
proper driver init can probably go away now.

> does not allow the user to prevent a possible scsi bus reset at driver 
> init time. The scsi bus reset only takes place if the driver discovers 
> that the bus was already wedged when it started. (It proved useful when I 
> was introducing faults for EH testing, BTW.)

Much saner approach, I'm sure. Don't forget the driver was written
before sophisticated error handling came along. Reset at boot and keep
your fingers crossed was the strategy in these days.

Cheers,

Michael

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions

[Michael Schmitz]

Hi Nicholas,

 The values for USLEEP_* are taken to be in units jiffies, according to
 comments in NCR5380.c. Replacing them by the msecs_to_jiffies conversion
 is in fact wrong.

 Please drop the changes to g_NCR5380.c for that reason.


 right the comment indicates it should be jiffies - my interpretation
 of that was that in NCR5380.c
 were  jiffies + (250 * HZ / 1000); constructs would be correctly
 converted to e.g. jiffies + msecs_to_jiffies(250)

No objection there.

 And defines like USLEEP_POLL are noted to be in jiffies:
 * USLEEP_SLEEP - amount of time, in jiffies, to sleep

 and then defined correctly as HZ indepenedent values:
 #define USLEEP_SLEEP (20*HZ/1000

 and thus should be the same as msecs_to_jiffies(20)

None here either.


 now g_NCR5380.c defines
 #define USLEEP_POLL 1
 #define USLEEP_SLEEP20
 #define USLEEP_WAITLONG 500

 for the DTC3181E card - but without the conversion to ms
 from the use in the code though (e.g NCR5380_set_timer) it
 seemed to me that it actually should be jiffeis equivalent ms and

We can't know that - it's a fair guess, but the way it is currently
defined will see these constants used as jiffies, not ms.

'git blame' is of little help here ...

^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700  59) /*
settings for DTC3181E card with only Mustek scanner attached */
^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700  60) #define
USLEEP_POLL 1
^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700  61) #define
USLEEP_SLEEP20
^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700  62) #define
USLEEP_WAITLONG 500

The DTC3181E part dates back to '97. Let's see whether Ronald stilll remembers.

HZ used to be set to 100 in those days, so USLEEP_POLL=1 equates to
10ms, not 1ms

 the intent was only to change the USLEEP_POLL and USLEEP_WAITLONG
 settings for the specific device but not the unit and thus it
 shuld have been converted by msecs_to_jiffies(1) resp.
 msecs_to_jiffies(500). The problem with this if it is left in its
 current form is that the timeouts would actually depend on the HZ
 setting of the system which is probably not the intent.

I think it's safe to assume the code in question predates the option
to configure the scheduling tick frequency, so yes, probably not
intended.

The problem with your replacing jiffies by ms is that this will change
the timing for this particular combination of hardware by an order of
magnitude. That large a change in timing would need to be backed up by
testing on the actual hardware.

Much safer to use

#define USLEEP_POLLmsecs_to_jiffies(10)
#define USLEEP_SLEEP   msecs_to_jiffies(200)
#define USLEEP_WAITLONGmsecs_to_jiffies(5000)

and make sure no change in timing is incurred at all.

Cheers,

  Michael



 Pleas do give this one more look if the argument above is
 not sound I appologize for the noise.

 thx!
 hofrat
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions

[Michael Schmitz]

Finn, Nicholas,


On Sat, 31 Jan 2015, Nicholas Mc Guire wrote:


This is only an API consolidation to make things more readable.

Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).


... and some instances of value are replaced by 
msecs_to_jiffies(value)

which seems to be completely wrong.


The values for USLEEP_* are taken to be in units jiffies, according to 
comments in NCR5380.c. Replacing them by the msecs_to_jiffies 
conversion is in fact wrong.


Please drop the changes to g_NCR5380.c for that reason.

Cheers,

  Michael






Signed-off-by: Nicholas Mc Guire der.h...@hofr.at

v2: the original patch was not taking care of all the dependencies
as reported by Finn Thain fth...@telegraphics.com.au - this
version now uses the suggested config to check the patch.


No. The original patch introduced compiler warnings. v2 changed the 
format

specifiers as advised by me. And it also changed g_NCR5380.c (for some
unstated reason).

The patch revision history should go after a --- cut line, as is
described in Documentation/SubmittingPatches.



Converting milliseconds to jiffies by val * HZ / 1000 is technically
ok but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API cleanup only.


Do these corner cases affect constants like 1, 20, 200, 250 or 500 ms?



This patch was only compile tested with i386_defconfig + CONFIG_ISA=y
as well as all dependent drivers enabled:
CONFIG_SCSI_LOWLEVEL=y, CONFIG_SCSI_GENERIC_NCR5380=m
CONFIG_SCSI_DMX3191D=m, CONFIG_SCSI_DTC3280=m
CONFIG_SCSI_GENERIC_NCR5380=m, CONFIG_SCSI_GENERIC_NCR5380_MMIO=m
CONFIG_SCSI_PAS16=m, CONFIG_SCSI_T128=m

Patch is against 3.19.0-rc6 -next-20150130

---
 drivers/scsi/NCR5380.c   |   10 +-
 drivers/scsi/g_NCR5380.c |6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 36244d6..35bb93b 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -474,11 +474,11 @@ static void NCR5380_print_phase(struct 
Scsi_Host *instance)

  */
 #ifndef USLEEP_SLEEP
 /* 20 ms (reasonable hard disk speed) */
-#define USLEEP_SLEEP (20*HZ/1000)
+#define USLEEP_SLEEP msecs_to_jiffies(20)
 #endif
 /* 300 RPM (floppy speed) */
 #ifndef USLEEP_POLL
-#define USLEEP_POLL (200*HZ/1000)
+#define USLEEP_POLL msecs_to_jiffies(200)
 #endif
 #ifndef USLEEP_WAITLONG
 /* RvC: (reasonable time to wait on select error) */
@@ -576,7 +576,7 @@ static int __init __maybe_unused 
NCR5380_probe_irq(struct Scsi_Host *instance,
 		if ((mask  possible)  (request_irq(i, probe_intr, 0, 
NCR-probe, NULL) == 0))

trying_irqs |= mask;

-   timeout = jiffies + (250 * HZ / 1000);
+   timeout = jiffies + msecs_to_jiffies(250);
probe_irq = NO_IRQ;

/*
@@ -634,7 +634,7 @@ static void prepare_info(struct Scsi_Host 
*instance)

 sg_tablesize %d, this_id %d, 
 flags { %s%s%s}, 
 #if defined(USLEEP_POLL)  defined(USLEEP_WAITLONG)
-USLEEP_POLL %d, USLEEP_WAITLONG %d, 
+USLEEP_POLL %lu, USLEEP_WAITLONG %lu, 
 #endif
 options { %s} ,
 	 instance-hostt-name, instance-io_port, 
instance-n_io_port,
@@ -1348,7 +1348,7 @@ static int NCR5380_select(struct Scsi_Host 
*instance, struct scsi_cmnd *cmd)

 * selection.
 */

-   timeout = jiffies + (250 * HZ / 1000);
+   timeout = jiffies + msecs_to_jiffies(250);

/*
 * XXX very interesting - we're seeing a bounce where the BSY we
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index f35792f..9f978ad 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -57,9 +57,9 @@
  */

 /* settings for DTC3181E card with only Mustek scanner attached */
-#define USLEEP_POLL1
-#define USLEEP_SLEEP   20
-#define USLEEP_WAITLONG500
+#define USLEEP_POLLmsecs_to_jiffies(1)
+#define USLEEP_SLEEP   msecs_to_jiffies(20)
+#define USLEEP_WAITLONGmsecs_to_jiffies(500)

 #define AUTOPROBE_IRQ




--


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 22/36] atari_scsi: Fix atari_scsi deadlocks on Falcon

[Michael Schmitz]

Hi Finn

Changes since v1:
- Use Geert's suggestion for simpler stdma_is_locked_by() implementation.

---
  arch/m68k/atari/stdma.c |   61 ++--
  arch/m68k/include/asm/atari_stdma.h |4 -
  drivers/scsi/atari_NCR5380.c|   35 +---
  drivers/scsi/atari_scsi.c   |   76 
+++-
  4 files changed, 69 insertions(+), 107 deletions(-)

Index: linux/arch/m68k/atari/stdma.c
===
--- linux.orig/arch/m68k/atari/stdma.c  2014-10-27 16:17:59.0 +1100
+++ linux/arch/m68k/atari/stdma.c   2014-10-27 16:25:45.0 +1100
@@ -59,6 +59,31 @@ static irqreturn_t stdma_int (int irq, v
  /* End of Prototypes **/
  
  
+/**

+ * stdma_try_lock - attempt to acquire ST DMA interrupt lock
+ * @handler: interrupt handler to use after acquisition
+ *
+ * Returns !0 if lock was acquired; otherwise 0.
+ */
+
+int stdma_try_lock(irq_handler_t handler, void *data)
+{
+   unsigned long flags;
+
+   local_irq_save(flags);
+   if (stdma_locked) {
+   local_irq_restore(flags);
+   return 0;
+   }
+
+   stdma_locked   = 1;
+   stdma_isr  = handler;
+   stdma_isr_data = data;
+   local_irq_restore(flags);
+   return 1;
+}
+EXPORT_SYMBOL(stdma_try_lock);
+
  
  /*

   * Function: void stdma_lock( isrfunc isr, void *data )
@@ -78,19 +103,10 @@ static irqreturn_t stdma_int (int irq, v
  
  void stdma_lock(irq_handler_t handler, void *data)

  {
-   unsigned long flags;
-
-   local_irq_save(flags);  /* protect lock */
-
/* Since the DMA is used for file system purposes, we
 have to sleep uninterruptible (there may be locked
 buffers) */
-   wait_event(stdma_wait, !stdma_locked);
-
-   stdma_locked   = 1;
-   stdma_isr  = handler;
-   stdma_isr_data = data;
-   local_irq_restore(flags);
+   wait_event(stdma_wait, stdma_try_lock(handler, data));
  }
  EXPORT_SYMBOL(stdma_lock);
  
@@ -122,22 +138,25 @@ void stdma_release(void)

  EXPORT_SYMBOL(stdma_release);
  
  
-/*

- * Function: int stdma_others_waiting( void )
- *
- * Purpose: Check if someone waits for the ST-DMA lock.
- *
- * Inputs: none
- *
- * Returns: 0 if no one is waiting, != 0 otherwise
+/**
+ * stdma_is_locked_by - allow lock holder to check whether it needs to release.
+ * @handler: interrupt handler previously used to acquire lock.
   *
+ * Returns !0 if locked for the given handler; 0 otherwise.
   */
  
-int stdma_others_waiting(void)

+int stdma_is_locked_by(irq_handler_t handler)
  {
-   return waitqueue_active(stdma_wait);
+   unsigned long flags;
+   int result;
+
+   local_irq_save(flags);
+   result = stdma_locked  (stdma_isr == handler);
+   local_irq_restore(flags);
+
+   return result;
  }
-EXPORT_SYMBOL(stdma_others_waiting);
+EXPORT_SYMBOL(stdma_is_locked_by);
  
  
  /*

Index: linux/arch/m68k/include/asm/atari_stdma.h
===
--- linux.orig/arch/m68k/include/asm/atari_stdma.h  2014-10-27 
16:17:59.0 +1100
+++ linux/arch/m68k/include/asm/atari_stdma.h   2014-10-27 16:25:45.0 
+1100
@@ -8,11 +8,11 @@
  
  /* Prototypes */
  
+int stdma_try_lock(irq_handler_t, void *);

  void stdma_lock(irq_handler_t handler, void *data);
  void stdma_release( void );
-int stdma_others_waiting( void );
  int stdma_islocked( void );
-void *stdma_locked_by( void );
+int stdma_is_locked_by(irq_handler_t);
  void stdma_init( void );
  
  /* End of Prototypes **/


Acked-by: Michael Schmitz schm...@debian.org


Index: linux/drivers/scsi/atari_NCR5380.c
===
--- linux.orig/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:36.0 
+1100
+++ linux/drivers/scsi/atari_NCR5380.c  2014-10-27 16:25:45.0 +1100
@@ -879,10 +879,10 @@ static void NCR5380_exit(struct Scsi_Hos
   *
   */
  
-static int NCR5380_queue_command_lck(struct scsi_cmnd *cmd,

- void (*done)(struct scsi_cmnd *))
+static int NCR5380_queue_command(struct Scsi_Host *instance,
+ struct scsi_cmnd *cmd)
  {
-   SETUP_HOSTDATA(cmd-device-host);
+   struct NCR5380_hostdata *hostdata = shost_priv(instance);
struct scsi_cmnd *tmp;
unsigned long flags;


Nitpick - why did this change set go into this particular patch? Because 
you are converting from NCR5380_queue_command_lck to NCR5380_queue_command?


  
@@ -893,7 +893,7 @@ static int NCR5380_queue_command_lck(str

printk(KERN_NOTICE scsi%d: WRITE attempted with NO_WRITE debugging 
flag set\n,
   H_NO(cmd));
cmd

Re: [PATCH v2 22/36] atari_scsi: Fix atari_scsi deadlocks on Falcon

[Michael Schmitz]

Finn,
  

Index: linux/drivers/scsi/atari_NCR5380.c
===
--- linux.orig/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:36.0
+1100
+++ linux/drivers/scsi/atari_NCR5380.c  2014-10-27 16:25:45.0
+1100
@@ -879,10 +879,10 @@ static void NCR5380_exit(struct Scsi_Hos
   *
   */
  -static int NCR5380_queue_command_lck(struct scsi_cmnd *cmd,
- void (*done)(struct scsi_cmnd *))
+static int NCR5380_queue_command(struct Scsi_Host *instance,
+ struct scsi_cmnd *cmd)
  {
-   SETUP_HOSTDATA(cmd-device-host);
+   struct NCR5380_hostdata *hostdata = shost_priv(instance);
   struct scsi_cmnd *tmp;
   unsigned long flags;
  

Nitpick - why did this change set go into this particular patch? Because you
are converting from NCR5380_queue_command_lck to NCR5380_queue_command?



That's right.
  


OK, I just wanted to understand why it's being combined with the stram.c 
patch (and the directly resulting changes in the SCSI driver).


The change to NCR5380_queue_command() can only now be done, as a result 
of the deadlock fixes in this patch, so it follows logically. Makes 
perfectly good sense to me now, thanks.




  @@ -893,7 +893,7 @@ static int NCR5380_queue_command_lck(str
printk(KERN_NOTICE scsi%d: WRITE attempted with NO_WRITE debugging flag
set\n,
   H_NO(cmd));
cmd-result = (DID_ERROR  16);
-   done(cmd);
+   cmd-scsi_done(cmd);
return 0;
}
  #endif /* (NDEBUG  NDEBUG_NO_WRITE) */
@@ -904,8 +904,6 @@ static int NCR5380_queue_command_lck(str
*/
  
  	SET_NEXT(cmd, NULL);

-   cmd-scsi_done = done;
-
   cmd-result = 0;
  
   /*
  

Ditto for these two.



Again, it follows from the differences in the formal parameters between 
NCR5380_queue_command_lck() and NCR5380_queue_command().
  


Yep, I got that bit. Thanks for the explanation, and apologies for the 
noise.


  

@@ -915,7 +913,6 @@ static int NCR5380_queue_command_lck(str
* sense data is only guaranteed to be valid while the condition exists.
*/
  - local_irq_save(flags);
   /* ++guenther: now that the issue queue is being set up, we can lock
   ST-DMA.
* Otherwise a running NCR5380_main may steal the lock.
* Lock before actually inserting due to fairness reasons explained in
@@ -928,11 +925,13 @@ static int NCR5380_queue_command_lck(str
* because also a timer int can trigger an abort or reset, which would
* alter queues and touch the lock.
*/
-   if (!IS_A_TT()) {
-   /* perhaps stop command timer here */
-   falcon_get_lock();
-   /* perhaps restart command timer here */
-   }
+   /* perhaps stop command timer here */
+   if (!falcon_get_lock())
+   return SCSI_MLQUEUE_HOST_BUSY;
+   /* perhaps restart command timer here */
+
  
The comments about stopping and restarting the command timer can be 
removed. In 2.4 kernels, the driver would tweak the timers and wait on 
the lock unconditionally, Can't ne done anymore, for so many reasons.



Well, you sent an acked-by for this patch, so I'm a bit confused. Do you 
want me to re-spin it?
  


Not at all - this was just something I wanted to raise for future 
patches, before it slips my mind, again. Sorry I did not make that clear 
at all.


I expect we will have an opportunity to address this when we embark on a 
wider cleanup of comments throughout the driver - most of these comments 
were for the 0.9 kernel series and might still be somewhat germane up to 
2.4. Might be totally  misleading now.


Not a priority at all - I know you have bigger fish to fry. I should not 
have brought it up in this context.


No problems at all with this patch (or indeed with the rest of the series).

Cheers,

   Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers

[Michael Schmitz]

Hi Finn,

 Leaves the current instability - I did some work on the CT60 accelerator
 (reflashed the firmware so I can use the CTPCI board). This might have
 caused the system to become more unstable. Needs more investigation.

 I gather from the emails we've exchanged that the current instability
 (data corruption) dates back to March or thereabouts, so it is unrelated
 to this patch series.

Absolutely right - meant to add that but got interrupted in that train
of thought :-(

 The lockups have been resolved, which leaves only the ST DMA FIFO issue,
 which seems to be an old problem. From the comments in atari_scsi, it
 doesn't look like this was ever resolved.

 /* If the DMA was active, but now bit 1 is not clear, it is some
  * other 5380 interrupt that finishes the DMA transfer. We have to
  * calculate the number of residual bytes and give a warning if
  * bytes are stuck in the ST-DMA fifo (there's no way to reach them!)
  */
 if (atari_dma_active  (dma_stat  0x02)) {
 unsigned long transferred;

 transferred = SCSI_DMA_GETADR() - atari_dma_startaddr;
 /* The ST-DMA address is incremented in 2-byte steps, but the
  * data are written only in 16-byte chunks. If the number of
  * transferred bytes is not divisible by 16, the remainder is
  * lost somewhere in outer space.
  */
 if (transferred  15)
 printk(KERN_ERR SCSI DMA error: %ld bytes lost in 
ST-DMA fifo\n, transferred  15);

 Presuming that this is an old issue (apparently timing sensitive), we can
 conclude that no regressions were observed in your tests. I'm content with
 this presumption. I gather that you are too, or you would not have sent a
 tested-by tag. Which seems to put the ball in Geert's court?

Right again - though it's probably not Geert but James who needs to
give the go-ahead.

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers

[Michael Schmitz]

David, Geert,

my Falcon has some variant of this clock patch installed - it may not
be precisely the one described but reasonably close. It also has one
of the old 030 accelerator tricks (clock doubling of the 030 if the
CPU does not do bus cycles - named Skunk) fitted; the clock patch was
installed at the same time.

The Falcon ran stable with just SCSI disks in use (so no locking
races) until a few years ago when I started to use it for kernel
hacking. There were the occasional SCSI lockups which I attributed to
the SCSI clock problem, for want of a better explanation. IIRC when
the clock signal to the 5380 becomes unstable, the chip locks up and
needs to be reset. With the old 2.4 kernels, recovery from reset
worked OK and I've not seen disk corruption or hard read errors.

2.6 kernels changed a lot of things around SCSI midlevel and error
handling. I could never make error recovery from these SCSI lockups
work in 2.6 or 3.x until Arnd's locking fixes (and my patch to defer
command queueing if the lock had been taken by IDE). The lockups did
not stop happening until Finn's patches - and I probably need to
emphasize again that they are gone entirely now. It does seem 2.4
suffered from similar race problems, the driver just managed to
recover from those.

Leaves the current instability - I did some work on the CT60
accelerator (reflashed the firmware so I can use the CTPCI board).
This might have caused the system to become more unstable. Needs more
investigation.

Cheers,

  Michael


On Wed, Nov 5, 2014 at 9:10 PM, Geert Uytterhoeven ge...@linux-m68k.org wrote:
 On Wed, Nov 5, 2014 at 8:56 AM, David Gálvez dgalve...@gmail.com wrote:
 Do you know about the Falcon's disturbance in the SDMA clock signal
 hardware problem?
 Most Falcons, specially those used in music studios, have a hardware
 patch to fix this, it's normally called SCSI patch.

 Some more info:

 http://didierm.pagesperso-orange.fr/doc/eng/c_0a.htm

 So this adds additional buffering to the clock.

 Note that input pins 3 and 5 of the 74HC04 are left floating!
 I recommend tying them to either GND (pin 7) or VCC (pin 14), to avoid
 them picking up high-frequency signals and consuming power.

 Gr{oetje,eeting}s,

 Geert

 --
 Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
 ge...@linux-m68k.org

 In personal conversations with technical people, I call myself a hacker. But
 when I'm talking to journalists I just say programmer or something like 
 that.
 -- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers

[Michael Schmitz]

Hi Finn,


 Thanks for testing.

 No regressions over v1 means no regressions, right?



 Well, what would I compare the driver performance to? With your patches to
 sort out locking races, the driver is more stable than I've ever seen it in
 years. That's a definite win. Big improvement over the driver in its current
 state in m68k and mainstream (which locks up quite reliably with even
 moderate concurrent IDE and SCSI I/O for me). No regression over v1 or
 patches that you sent for me to test off-list.

 On the other hand, I've seen warnings about lost bytes (stuck in the DMA
 fifo) for the first time _ever_ with the new driver - we've discussed that
 at length, and it is still unclear why these happen. This is a known NCR5380
 issue, and pretty much anything could have precipitated that. Must have
 happened for other Falcon users before in the past, because the interrupt
 handler explicitly checks for this condition.

I went back and checked test logs from driver tests before any of your
patches - no regressions over vanilla 3.17 or 3.16.

The 'lost bytes' error is rare enough that it would not have shown in
those tests. Vanilla kernels run into the ST-DMA locking race at about
7% of the total test set.

The last kernel that performed with a similar stability as with your
patches was 2.4.30 or thereabouts.

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers

[Michael Schmitz]

Finn,

This patch series has fixes for bugs and compiler warnings as well as code
cleanup and modernization. It covers all ten NCR5380 drivers and the three
core NCR5380 drivers so it's fairly large.

These patches remove a lot of duplicated code and C pre-processor abuse.
There are also patches for scsi_add_host() conversion for atari_scsi,
mac_scsi and sun3_scsi.

Some steps are taken toward re-unification of the NCR5380 core driver forks
by reducing divergence between them. Also, the atari_NCR5380.c core driver
is generalized so it can be used by sun3_scsi.c (and others).

I have compile-tested all of the NCR5380 drivers (x86, ARM and m68k) and
executed mac_scsi and dmx3191d on suitable hardware. I found no regressions
but the core NCR5380 drivers have bugs unrelated to these patches.

Testing mac_scsi and dmx3191d provides only limited code coverage for these
patches. Some testing on Sun 3, Atari ST and/or Atari TT would be nice
(I don't have the hardware).
  
Tested atari_scsi on Falcon / CT60 hardware - no regressions over v1 
that I've seen.


Tested-by: Michael Schmitz schmitz...@gmail.com


There are old bugs relating to exception handling and autosense in the
core NCR5380 drivers that can make testing difficult. I'm working on a
series of patches to address these bugs. Those patches are not yet ready
for submission but they were helpful for the testing I did and may be
helpful to other testers. Let me know if so.

Changes since v1:
- Re-based to v3.17.
- Addressed issues raised in code review (see relevant patches for details).
- Added patches 30 to 36, to remove sun3_NCR5380.c entirely and remove more
  static variables from atari_NCR5380.c.

This patch set stops short of parameterizing the drivers with platform_data
and/or ops struct. IMHO, it would be premature to do such refactoring before
drivers have been purged of static variables and certain #ifdefs. After
that is done, entire modules could be replaced with platform devices.

Several patches in this set address issues with the tagged command queueing
code (see patches 7, 33 and 34). I've since learned from recent discussions
on the linux-scsi list that use of the tag member in struct scsi_cmnd is
deprecated. If removal of the tag member is imminent then it may be better
to remove TCQ support from all of the NCR5380 drivers instead of this
cleanup. Or it could be done separately.

Removal of TCQ code might make re-unification easier, by bringing
atari_NCR5380.c closer to NCR5380.c and eliminating some #ifdefs.
Changes to the TCQ code would affect atari_scsi, being the only driver to
#define SUPPORT_TAGS.

---
 MAINTAINERS |1 
 arch/m68k/atari/config.c|   27 
 arch/m68k/atari/stdma.c |   61 
 arch/m68k/include/asm/atari_stdma.h |4 
 arch/m68k/include/asm/macintosh.h   |4 
 arch/m68k/mac/config.c  |  146 +
 arch/m68k/sun3/config.c |   60 
 drivers/scsi/Kconfig|2 
 drivers/scsi/NCR5380.c  |  295 +--
 drivers/scsi/NCR5380.h  |   78 
 drivers/scsi/arm/cumana_1.c |   18 
 drivers/scsi/arm/oak.c  |   23 
 drivers/scsi/atari_NCR5380.c|  981 +---

 drivers/scsi/atari_scsi.c   |  676 +++-
 drivers/scsi/atari_scsi.h   |   60 
 drivers/scsi/dmx3191d.c |   31 
 drivers/scsi/dtc.c  |   85 -
 drivers/scsi/dtc.h  |   26 
 drivers/scsi/g_NCR5380.c|  224 --
 drivers/scsi/g_NCR5380.h|   26 
 drivers/scsi/mac_scsi.c |  542 ++
 drivers/scsi/mac_scsi.h |   74 
 drivers/scsi/pas16.c|  106 -
 drivers/scsi/pas16.h|   21 
 drivers/scsi/sun3_NCR5380.c | 2933 

 drivers/scsi/sun3_scsi.c|  512 ++
 drivers/scsi/sun3_scsi.h|   84 -
 drivers/scsi/t128.c |   83 -
 drivers/scsi/t128.h |   23 
 29 files changed, 1745 insertions(+), 5461 deletions(-)





  


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 23/29] atari_scsi: Convert to platform device

[Michael Schmitz]

Finn,

On Mon, 20 Oct 2014, Michael Schmitz wrote:

  

Hi Finn,

not certain it is related to this exact patch - the driver crashes 
pretty much on the spot when selecting the first target on the bus.



If it isn't that exact patch then it will probably be one of the other 
atari_scsi patches. Most of the other relevant stuff was covered by my own 
testing.


Anyway, thanks for testing and for sending the backtrace. I'll figure out 
what went wrong and fix it before I send a new patch set.


  


Turns out I missed a reject in patch 29 of the series (hunk #4) which 
caused a local_irq_save(flags) to go AWOL. Both commenting out the 
resulting lone local_irq_restore(flags), as well as fixing the reject 
properly makes the driver behave as it should.


No locking races, and no more warnings about bytes stuck in the DMA 
fifo. We might be on to a winner here :-)


Cheers,

   Michael



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 23/29] atari_scsi: Convert to platform device

[Michael Schmitz]

Hi Finn,

not certain it is related to this exact patch - the driver crashes 
pretty much on the spot when selecting the first target on the bus.


Panic log:
Atari SCSI: resetting the SCSI bus... done
scsi host0: Atari native SCSI, io_port 0x0, n_io_port 0, base 0x0, irq 
0, can_queue 8, cmd_per_lun 1, sg_tablesize 0, this_id 7, options { 
REAL_DMA SUPPORT_TAGS }

blk_queue_max_segments: set to minimum 1
blk_queue_max_segments: set to minimum 1
blk_queue_max_segments: set to minimum 1
Unable to handle kernel NULL pointer dereference at virtual address 0158
Oops: 
Modules linked in:
PC: [000cd87e] do_coredump+0x3a/0xbe4
SR: 2204  SP: 20803c8c  a2: 00a7
d0: d1: 003cd2: 0006d3: 20803dbc
d4: 00025930d5: 20802000a0: 00b59c70a1: 20802000
Process kworker/0:1 (pid: 69, task=00a7)
Frame format=4 fault addr=0158 fslw=01050200
Stack from 20803cc8:
   0006 20803dbc 00025930 20802000 0002b9be 001f 20803dd0 
0002aa56
   00a3c540 00b59c70 0008d3e0 00a07cc0 003a8074 00c75000 0002a664 
0007
   00a7035c 20803dd0 20803e80     

   00a70374 0002a9d0 00c75000 2200 20803dd0 00a7 0007 
00a7035c
   0002a5fe 00a7 0002aa82 2200 20803dbc 20803dd0 0002aa56 
00a3c540
   0002c5da 00b59c70 0002c6c4 20803dd0 2200 20803f06 20803f00 


Call Trace: [00025930] do_group_exit+0x0/0x9e
[0002b9be] do_signal_stop+0x0/0x186
[0002aa56] dequeue_signal+0x0/0x118
[0008d3e0] kmem_cache_free+0x10a/0x128
[0002a664] __sigqueue_free+0x32/0x3a
[0002a9d0] __dequeue_signal+0xaa/0x130
[2200] do_one_initcall+0x18c/0x1d4
[0002a5fe] recalc_sigpending+0x6/0x1e
[0002aa82] dequeue_signal+0x2c/0x118
[2200] do_one_initcall+0x18c/0x1d4
[0002aa56] dequeue_signal+0x0/0x118
[0002c5da] get_signal+0x7e/0x4aa
[0002c6c4] get_signal+0x168/0x4aa
[2200] do_one_initcall+0x18c/0x1d4
[0019bc12] NCR5380_transfer_pio.isra.6+0x0/0x1da
[000308b6] move_linked_works+0x0/0x9c
[428c] do_signal+0x1a/0x1d8
[2200] do_one_initcall+0x18c/0x1d4
[0019d390] NCR5380_main+0x9c6/0xd98
[00046030] handle_simple_irq+0x36/0x56
[2c36] do_IRQ+0x26/0x32
[2200] do_one_initcall+0x18c/0x1d4
[2b1c] auto_irqhandler_fixup+0x4/0xc
[2200] do_one_initcall+0x18c/0x1d4
[4a1a] do_notify_resume+0x3a/0x44
[2ac8] do_signal_return+0x10/0x1a
[0019bc12] NCR5380_transfer_pio.isra.6+0x0/0x1da
[000308b6] move_linked_works+0x0/0x9c
[2200] do_one_initcall+0x18c/0x1d4
[0019d390] NCR5380_main+0x9c6/0xd98
[00030a2a] worker_enter_idle+0x0/0x146
[00032a08] process_one_work+0x0/0x2a8
[00032cb0] process_scheduled_works+0x0/0x30
[000308b6] move_linked_works+0x0/0x9c
[00032cb0] process_scheduled_works+0x0/0x30
[000308b6] move_linked_works+0x0/0x9c
[00081c00] change_protection_range+0x150/0x1cc
[00032b14] process_one_work+0x10c/0x2a8
[002254bc] schedule+0x0/0x58
[00032dea] worker_thread+0x10a/0x4d0
[0003ee30] __init_waitqueue_head+0x0/0xc
[00032ce0] worker_thread+0x0/0x4d0
[0003ee30] __init_waitqueue_head+0x0/0xc
[00036ee0] kthread+0xa4/0xc0
[00036e3c] kthread+0x0/0xc0
[299c] ret_from_kernel_thread+0xc/0x14

Code: 026c ffa4 206a 0354 2028 0170 2d40 ffac 202b 0158 2d40 ffb0 286b 
014c 4a8c 670c 4aac 0014 6706 e8c0 0782 660a 4cee 38fc

Disabling lock debugging due to kernel taint

Unable to handle kernel NULL pointer dereference at virtual address 0158
Oops: 
Modules linked in:
PC: [000372c2] kthread_data+0x8/0x10
SR: 2700  SP: 20803af0  a2: 00a7
d0: 0003d1: faf65c54d2: 00a7d3: 00a701e8
d4: 00025930d5: 20802000a0: a1: 00a90474
Process kworker/0:1 (pid: 69, task=00a7)
Frame format=4 fault addr=fff0 fslw=01050020
Stack from 20803b2c:
   00033976 00a7 002252a6 00a7  0001 20803b98 
00025930
   20802000 00a70164 001f 00a7 20803b98 00a6fff8 000255b8 
00a9
   00025526 01050200 20803dbc 00025930 20802000 0002b9be 001f 
20803c8c
   00223674 20803dd0 20803d6c 20803b98 20803b98 00a701ac 581c 
000b
   0007 0001 00274ae2 0026dce5 0026e477  20803c8c 
6806
   0026e477 20803c8c  0026e45f 0158 20803c8c 0002aa56 
0007

Call Trace: [00033976] wq_worker_sleeping+0xa/0x82
[002252a6] __schedule+0x21e/0x394
[00025930] do_group_exit+0x0/0x9e
[000255b8] do_exit+0x550/0x846
[00025526] do_exit+0x4be/0x846
[00025930] do_group_exit+0x0/0x9e
[0002b9be] do_signal_stop+0x0/0x186
[00223674] printk+0x0/0x24
[581c] bad_super_trap+0x0/0xb2
[6806] send_fault_sig+0xaa/0xea
[0002aa56] dequeue_signal+0x0/0x118
[5e5e] buserr_c+0x254/0x390
[0002aa56] dequeue_signal+0x0/0x118
[2950] buserr+0x20/0x28
[00025930] do_group_exit+0x0/0x9e
[00025930] do_group_exit+0x0/0x9e
[0002b9be] do_signal_stop+0x0/0x186
[0002aa56] dequeue_signal+0x0/0x118
[0008d3e0] kmem_cache_free+0x10a/0x128
[0002a664] __sigqueue_free+0x32/0x3a
[0002a9d0] __dequeue_signal+0xaa/0x130
[2200] 

Re: [PATCH 23/29] atari_scsi: Convert to platform device

[Michael Schmitz]

Hi Finn,

 not certain it is related to this exact patch - the driver crashes
 pretty much on the spot when selecting the first target on the bus.

 If it isn't that exact patch then it will probably be one of the other
 atari_scsi patches. Most of the other relevant stuff was covered by my own
 testing.

 Anyway, thanks for testing and for sending the backtrace. I'll figure out
 what went wrong and fix it before I send a new patch set.

Let me know what I can do to help - e.g. can the platform device patch
be backed out without upsetting the rest of the series?

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 23/29] atari_scsi: Convert to platform device

[Michael Schmitz]

Hi Finn,


Can these be handled through the platform_device's resources?




I don't know.
  
  
Should be possible - I've seen that used in the ISP116x HCD driver 
(arch-dependent post-register access delay function passed via platform 
data).



Yes, possible, but is it desirable?
  


Only long term, i.e. if we can find a way to fold all the separate 
*_NCR5380 drivers into a core one.





If there's no third configuration then I see little to be gained from 
completely parameterizing the driver using a bunch of resources.


What am I missing? Why would it be desirable to have bits of driver code 
in arch/m68k/ and other bits of the same driver kept elsewhere in the 
tree (in drivers/)?


  


Again, only makes sense if the goal is to reduce the driver to a core 
driver with implementation bits separate.



(and IRQ_NONE is wrong, you should use 0)




+   if (IS_A_TT()) {
  
  

Check for instance-irq instead?


Yes, you'd think so, but a later patch (not in this set) would have to 
change it back to IS_A_TT().


Further patches align atari_NCR5380.c with NCR5380.c, such that the 
core driver then checks host-irq to find out whether an IRQ is in 
use. For Atari ST, request_irq() is not called even though the ST DMA 
irq is in use.
  
  

That's why the IRQ is set to 0, I guess? Works for me.



My patch tests for IS_A_TT not 0 because 0 has a different meaning 
depending on which core driver you use. I don't want to support three 
forks of NCR5380.c. One of the objectives of this patch series is to try 
to move toward convergence on a common core driver.
  


I wasn't talking about the use of IS_A_TT() here, rather about what 
host-irq = 0 achieves. Anyway, in the context of atari_scsi.c, 
host-irq == 0 is synonymous with !IS_A_TT(). If
host-irq == 0 is problematic with the long term goal of a singe 5380 
core driver, we'll just have to pick a value which means 'driver uses 
IRQ but you're not to fiddle with it'.
The old code states that setting instance-irq = 0 keeps the midlevel 
from tampering with the SCSI IRQ during queuecmd which would interfere 
with IDE and floppy.



I don't know what you mean. AFAIK, the SCSI mid layer doesn't care about 
instance-irq.
  


You're right - that's another obsolete comment then. The midlevel uses 
spinlocks now for ages, this means we can set instance-irq to the 
actual IRQ used.


I guess this is still relevant - I would not have seen the ST-DMA locked 
by IDE during SCSI queuecmd otherwise.



Lock-ups were due to disabling local IRQs. Please see,
  


Absolutely right, but that's not what I meant. I assumed the midlevel 
disabled interrupts during queuecommand - it does not, so the point is 
moot. If using the actual IRQ for instance-irq helps at all, no 
objection to that.


SCSI commands can still be queued while IDE IO is in flight - that is 
what the 'ST-DMA locked by IDE' above refers to.


Cheers,

   Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 23/29] atari_scsi: Convert to platform device

[Michael Schmitz]

Finn,

On Fri, 3 Oct 2014, Geert Uytterhoeven wrote:

  

+   if (ATARIHW_PRESENT(TT_SCSI)) {
+   atari_scsi_reg_read  = atari_scsi_tt_reg_read;
+   atari_scsi_reg_write = atari_scsi_tt_reg_write;
+   } else if (ATARIHW_PRESENT(ST_SCSI)) {
+   atari_scsi_reg_read  = atari_scsi_falcon_reg_read;
+   atari_scsi_reg_write = atari_scsi_falcon_reg_write;
  

Can these be handled through the platform_device's resources?




I don't know.
  


Should be possible - I've seen that used in the ISP116x HCD driver 
(arch-dependent post-register access delay function passed via platform 
data).




+   /* Leave sg_tablesize at 0 on a Falcon! */
+   if (IS_A_TT()  setup_sg_tablesize = 0)
+   atari_scsi_template.sg_tablesize = setup_sg_tablesize;
  

I think the IS_A_TT() check can just be removed.



Well, ST DMA will break if scatter/gather is enabled by the user.
  


Concur - we'd have to make certain the SG buffers are contiguous in 
physical memory before allowing SG on Falcon.


  

+#ifdef REAL_DMA
+   /* If running on a Falcon and if there's TT-Ram (i.e., more than one
+* memory block, since there's always ST-Ram in a Falcon), then
+* allocate a STRAM_BUFFER_SIZE byte dribble buffer for transfers
+* from/to alternative Ram.
+*/
+   if (ATARIHW_PRESENT(ST_SCSI)  !ATARIHW_PRESENT(EXTD_DMA) 
+   m68k_num_memory  1) {
+   atari_dma_buffer = atari_stram_alloc(STRAM_BUFFER_SIZE, SCSI);
+   if (!atari_dma_buffer) {
+   pr_err(PFX can't allocate ST-RAM double buffer\n);
+   return -ENOMEM;
+   }
+   atari_dma_phys_buffer = atari_stram_to_phys(atari_dma_buffer);
+   atari_dma_orig_addr = 0;
+   }
+#endif
  

More platform data?



Perhaps.



+   if (IS_A_TT())
+   instance-irq = IRQ_TT_MFP_SCSI;
+   else
+   instance-irq = IRQ_NONE;
  

platform_device resource?



If I thought it possible to parameterize the driver such that it never had 
to test IS_A_TT(), I'd probably agree that this would be more elegant.


Otherwise I'd prefer not to have parts of the logic separated off into the 
platform resources while the remaining logic remains in the driver itself.


While I don't think platform resources are desirable in this driver, I 
would like to hear Michael's views.
  


The IRQ is a good candidate to be passed via platform data. Register 
access primitives can be done via platform data as well. Likewise, the 
ST-DMA locking primitives. That still leaves the DMA setup and 
completion code - Falcon and TT differ here in that they require a 
different order of DMA setup and NCR setup (the Falcon SCSI chip is 
hooked up via the ST-DMA, the TT one memory mapped so DMA setup must 
come last on Falcon, and DMA completion check first. TT has that 
reversed. That's a bit more hassle and might require  lib_NCR5380 
approach similar to the ESP SCSI driver.


Aside from TT and ST, is there a third configuration that might benefit 
from a more data driven configuration?
  


TT and Falcon, not sure any of the ST/STE series ever had a SCSI 
adapter. Medusa is TT compatible



(and IRQ_NONE is wrong, you should use 0)



+   if (IS_A_TT()) {
  

Check for instance-irq instead?



Yes, you'd think so, but a later patch (not in this set) would have 
to change it back to IS_A_TT().


Further patches align atari_NCR5380.c with NCR5380.c, such that the core 
driver then checks host-irq to find out whether an IRQ is in use. For 
Atari ST, request_irq() is not called even though the ST DMA irq is in 
use.
  
That's why the IRQ is set to 0, I guess? Works for me. The old code 
states that setting instance-irq = 0 keeps the midlevel from tampering 
with the SCSI IRQ during queuecmd which would interfere with IDE and 
floppy. I guess this is still relevant - I would not have seen the 
ST-DMA locked by IDE during SCSI queuecmd otherwise.


Won't be able to test any of this for a while, sorry.

Cheers,

   Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Geert,

 [sorry for the long delay]

Tell me about it :-) I haven't had a good idea how to address this
problem so rather kept mum about it.

 On Mon, Apr 14, 2014 at 10:51 AM, Michael Schmitz schmitz...@gmail.com 
 wrote:
 Not sure my patch had ever made it into Geert's m68k-queue - except for the
 patch in my previous mail, my zorro_esp.c is still the same as I got from
 you in October last year. The project has been on the back burner for too
 long ...

 I don't think it makes much sense to add it to my queue, as it's purely SCSI
 and not critical for current m68k.

Fine, Ill try and resolve that with Dave then. Tuomas will need to do
the testing (unless someone wants to drop off the necessary hardware
with me - unlikely).

Thanks,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: add an entry for all the NCR5380 drivers

[Michael Schmitz]

Acked-by: Michael Schmitz schmitz...@gmail.com

On Mon, May 5, 2014 at 5:35 PM, Finn Thain fth...@telegraphics.com.au wrote:

 Signed-off-by: Finn Thain fth...@telegraphics.com.au
 Cc: Michael Schmitz schmitz...@gmail.com

 ---

 As requested:
 http://marc.info/?l=linux-arm-kernelm=139853302724112w=2

 diff --git a/MAINTAINERS b/MAINTAINERS
 index e67ea24..60ea600 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -5996,6 +5996,28 @@ M:   Petr Vandrovec p...@vandrovec.name
  S: Odd Fixes
  F: fs/ncpfs/

 +NCR 5380 SCSI DRIVERS
 +M: Finn Thain fth...@telegraphics.com.au
 +M: Michael Schmitz schmitz...@gmail.com
 +L: linux-scsi@vger.kernel.org
 +S: Maintained
 +F: Documentation/scsi/g_NCR5380.txt
 +F: drivers/scsi/NCR5380.*
 +F: drivers/scsi/arm/cumana_1.c
 +F: drivers/scsi/arm/oak.c
 +F: drivers/scsi/atari_NCR5380.c
 +F: drivers/scsi/atari_scsi.*
 +F: drivers/scsi/dmx3191d.c
 +F: drivers/scsi/dtc.*
 +F: drivers/scsi/g_NCR5380.*
 +F: drivers/scsi/g_NCR5380_mmio.c
 +F: drivers/scsi/mac_scsi.*
 +F: drivers/scsi/pas16.*
 +F: drivers/scsi/sun3_NCR5380.c
 +F: drivers/scsi/sun3_scsi.*
 +F: drivers/scsi/sun3_scsi_vme.c
 +F: drivers/scsi/t128.*
 +
  NCR DUAL 700 SCSI DRIVER (MICROCHANNEL)
  M: James E.J. Bottomley james.bottom...@hansenpartnership.com
  L: linux-scsi@vger.kernel.org
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/12] scsi/NCR5380: fix debugging macros and #include structure

[Michael Schmitz]

Hi James,

Perhaps Michael and Sam would be interested in sharing the role, for atari
and sun3 NCR5380 drivers (?)

If you insist ...

(kidding - Im OK with it if James thinks it's worth it)

As long as you understand how it works and how to fix it, the more the
merrier.  It gives me more people to yell at if something goes wrong
with the driver.


I used to have a fair understanding of the Atari driver years back - 
just need to get my head around all the midlevel and error handling 
changes since.


You'll be yelling at me regarding any breakage in the Atari NCR5380 
driver anyway in the end - count me in.


Cheers,

Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/12] scsi/NCR5380: fix debugging macros and #include structure

[Michael Schmitz]

Finn,

On Tue, Apr 29, 2014 at 2:22 PM, Finn Thain fth...@telegraphics.com.au wrote:

 On Sat, 26 Apr 2014, James Bottomley wrote:

 OK, so this is a pretty big change to an unmaintained driver.  I'll take
 it if you're willing to maintain the driver afterwards ... in which case
 I need another patch to add you to the MAINTAINERS file.

 Sure, I'm happy to support these patches and future work I plan to do on
 the driver.

 What additional responsibilities would come with adding my name the
 MAINTAINERS file?

 Perhaps Michael and Sam would be interested in sharing the role, for atari
 and sun3 NCR5380 drivers (?)

If you insist ...

(kidding - Im OK with it if James thinks it's worth it)

Cheers,

  Michael


 I only have hardware to test mac_scsi but I can obtain a Domex DMX3191D
 PCI card on ebay.

 --
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/12] scsi/NCR5380: adopt NCR5380_dprint() and NCR5380_dprint_phase()

[Michael Schmitz]

Acked-by: Michael Schmitz schm...@debian.org


All NCR5380 drivers already include the NCR5380.h header. Better to
adopt those macros rather than have three variations on them.

Moreover, the macros in NCR5380.h are preferable anyway: the atari_NCR5380
and sun3_NCR5380 versions are inflexible. For example, they can't accomodate
NCR5380_dprint(NDEBUG_MAIN | NDEBUG_QUEUES, ...)

Replace the NCR_PRINT* macros from atari_NCR5380.h and sun3_NCR5380.h with
the equivalent macros from NCR5380.h.

Signed-off-by: Finn Thain fth...@telegraphics.com.au

---
  drivers/scsi/atari_NCR5380.c |   22 +++---
  drivers/scsi/sun3_NCR5380.c  |   22 +++---
  2 files changed, 22 insertions(+), 22 deletions(-)

Index: linux-m68k/drivers/scsi/atari_NCR5380.c
===
--- linux-m68k.orig/drivers/scsi/atari_NCR5380.c2014-03-19 
23:23:02.0 +1100
+++ linux-m68k/drivers/scsi/atari_NCR5380.c 2014-03-19 23:34:43.0 
+1100
@@ -739,8 +739,8 @@ static void NCR5380_print_status(struct
Scsi_Cmnd *ptr;
unsigned long flags;
  
-	NCR_PRINT(NDEBUG_ANY);

-   NCR_PRINT_PHASE(NDEBUG_ANY);
+   NCR5380_dprint(NDEBUG_ANY, instance);
+   NCR5380_dprint_phase(NDEBUG_ANY, instance);
  
  	hostdata = (struct NCR5380_hostdata *)instance-hostdata;
  
@@ -1268,7 +1268,7 @@ static irqreturn_t NCR5380_intr(int irq,

INT_PRINTK(scsi%d: BASR=%02x\n, HOSTNO, basr);
/* dispatch to appropriate routine if found and done=0 */
if (basr  BASR_IRQ) {
-   NCR_PRINT(NDEBUG_INTR);
+   NCR5380_dprint(NDEBUG_INTR, instance);
if ((NCR5380_read(STATUS_REG)  (SR_SEL|SR_IO)) == 
(SR_SEL|SR_IO)) {
done = 0;
ENABLE_IRQ();
@@ -1396,7 +1396,7 @@ static int NCR5380_select(struct Scsi_Ho
unsigned long flags;
  
  	hostdata-restart_select = 0;

-   NCR_PRINT(NDEBUG_ARBITRATION);
+   NCR5380_dprint(NDEBUG_ARBITRATION, instance);
ARB_PRINTK(scsi%d: starting arbitration, id = %d\n, HOSTNO,
   instance-this_id);
  
@@ -1617,7 +1617,7 @@ static int NCR5380_select(struct Scsi_Ho

printk(KERN_ERR scsi%d: weirdness\n, HOSTNO);
if (hostdata-restart_select)
printk(KERN_NOTICE \trestart select\n);
-   NCR_PRINT(NDEBUG_ANY);
+   NCR5380_dprint(NDEBUG_ANY, instance);
NCR5380_write(SELECT_ENABLE_REG, hostdata-id_mask);
return -1;
}
@@ -1742,7 +1742,7 @@ static int NCR5380_transfer_pio(struct S
/* Check for phase mismatch */
if ((tmp  PHASE_MASK) != p) {
PIO_PRINTK(scsi%d: phase mismatch\n, HOSTNO);
-   NCR_PRINT_PHASE(NDEBUG_PIO);
+   NCR5380_dprint_phase(NDEBUG_PIO, instance);
break;
}
  
@@ -1764,18 +1764,18 @@ static int NCR5380_transfer_pio(struct S

if (!(p  SR_IO)) {
if (!((p  SR_MSG)  c  1)) {
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | 
ICR_ASSERT_DATA);
-   NCR_PRINT(NDEBUG_PIO);
+   NCR5380_dprint(NDEBUG_PIO, instance);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE |
  ICR_ASSERT_DATA | ICR_ASSERT_ACK);
} else {
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE |
  ICR_ASSERT_DATA | ICR_ASSERT_ATN);
-   NCR_PRINT(NDEBUG_PIO);
+   NCR5380_dprint(NDEBUG_PIO, instance);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE |
  ICR_ASSERT_DATA | ICR_ASSERT_ATN 
| ICR_ASSERT_ACK);
}
} else {
-   NCR_PRINT(NDEBUG_PIO);
+   NCR5380_dprint(NDEBUG_PIO, instance);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | 
ICR_ASSERT_ACK);
}
  
@@ -1997,7 +1997,7 @@ static void NCR5380_information_transfer

phase = (tmp  PHASE_MASK);
if (phase != old_phase) {
old_phase = phase;
-   NCR_PRINT_PHASE(NDEBUG_INFORMATION);
+   NCR5380_dprint_phase(NDEBUG_INFORMATION, 
instance);
}
  
  			if (sink  (phase != PHASE_MSGOUT)) {

@@ -2451,7 +2451,7 @@ static void NCR5380_information_transfer
break;
default:
printk(scsi%d: unknown

Re: [PATCH v2 08/12] scsi/NCR5380: adopt dprintk()

[Michael Schmitz]

Acked-by: Michael Schmitz schm...@debian.org


All NCR5380 drivers already include the NCR5380.h header. Better to
adopt those macros rather than have three variations on them.

Moreover, the macros in NCR5380.h are preferable because the atari_NCR5380
and sun3_NCR5380 versions are inflexible. For example, they can't accomodate
dprintk(NDEBUG_MAIN | NDEBUG_QUEUES, ...)

Replace the *_PRINTK macros from atari_NCR5380.h and sun3_NCR5380.h with
the equivalent macros from NCR5380.h.

Signed-off-by: Finn Thain fth...@telegraphics.com.au

---
  drivers/scsi/atari_NCR5380.c |  130 
+--
  drivers/scsi/atari_scsi.c|   16 ++---
  drivers/scsi/sun3_NCR5380.c  |  122 
  3 files changed, 134 insertions(+), 134 deletions(-)

Index: linux-m68k/drivers/scsi/atari_NCR5380.c
===
--- linux-m68k.orig/drivers/scsi/atari_NCR5380.c2014-03-19 
23:34:43.0 +1100
+++ linux-m68k/drivers/scsi/atari_NCR5380.c 2014-03-19 23:34:44.0 
+1100
@@ -370,7 +370,7 @@ static int is_lun_busy(Scsi_Cmnd *cmd, i
return 0;
if (TagAlloc[cmd-device-id][cmd-device-lun].nr_allocated =
TagAlloc[cmd-device-id][cmd-device-lun].queue_size) {
-   TAG_PRINTK(scsi%d: target %d lun %d: no free tags\n,
+   dprintk(NDEBUG_TAGS, scsi%d: target %d lun %d: no free tags\n,
   H_NO(cmd), cmd-device-id, cmd-device-lun);
return 1;
}
@@ -394,7 +394,7 @@ static void cmd_get_tag(Scsi_Cmnd *cmd,
!setup_use_tagged_queuing || !cmd-device-tagged_supported) {
cmd-tag = TAG_NONE;
hostdata-busy[cmd-device-id] |= (1  cmd-device-lun);
-   TAG_PRINTK(scsi%d: target %d lun %d now allocated by untagged 
+   dprintk(NDEBUG_TAGS, scsi%d: target %d lun %d now allocated by 
untagged 
   command\n, H_NO(cmd), cmd-device-id, 
cmd-device-lun);
} else {
TAG_ALLOC *ta = TagAlloc[cmd-device-id][cmd-device-lun];
@@ -402,7 +402,7 @@ static void cmd_get_tag(Scsi_Cmnd *cmd,
cmd-tag = find_first_zero_bit(ta-allocated, MAX_TAGS);
set_bit(cmd-tag, ta-allocated);
ta-nr_allocated++;
-   TAG_PRINTK(scsi%d: using tag %d for target %d lun %d 
+   dprintk(NDEBUG_TAGS, scsi%d: using tag %d for target %d lun %d 

   (now %d tags in use)\n,
   H_NO(cmd), cmd-tag, cmd-device-id,
   cmd-device-lun, ta-nr_allocated);
@@ -420,7 +420,7 @@ static void cmd_free_tag(Scsi_Cmnd *cmd)
  
  	if (cmd-tag == TAG_NONE) {

hostdata-busy[cmd-device-id] = ~(1  cmd-device-lun);
-   TAG_PRINTK(scsi%d: target %d lun %d untagged cmd finished\n,
+   dprintk(NDEBUG_TAGS, scsi%d: target %d lun %d untagged cmd 
finished\n,
   H_NO(cmd), cmd-device-id, cmd-device-lun);
} else if (cmd-tag = MAX_TAGS) {
printk(KERN_NOTICE scsi%d: trying to free bad tag %d!\n,
@@ -429,7 +429,7 @@ static void cmd_free_tag(Scsi_Cmnd *cmd)
TAG_ALLOC *ta = TagAlloc[cmd-device-id][cmd-device-lun];
clear_bit(cmd-tag, ta-allocated);
ta-nr_allocated--;
-   TAG_PRINTK(scsi%d: freed tag %d for target %d lun %d\n,
+   dprintk(NDEBUG_TAGS, scsi%d: freed tag %d for target %d lun 
%d\n,
   H_NO(cmd), cmd-tag, cmd-device-id, 
cmd-device-lun);
}
  }
@@ -478,7 +478,7 @@ static void merge_contiguous_buffers(Scs
for (endaddr = virt_to_phys(cmd-SCp.ptr + cmd-SCp.this_residual - 1) 
+ 1;
 cmd-SCp.buffers_residual 
 virt_to_phys(sg_virt(cmd-SCp.buffer[1])) == endaddr;) {
-   MER_PRINTK(VTOP(%p) == %08lx - merging\n,
+   dprintk(NDEBUG_MERGING, VTOP(%p) == %08lx - merging\n,
   page_address(sg_page(cmd-SCp.buffer[1])), endaddr);
  #if (NDEBUG  NDEBUG_MERGING)
++cnt;
@@ -490,7 +490,7 @@ static void merge_contiguous_buffers(Scs
}
  #if (NDEBUG  NDEBUG_MERGING)
if (oldlen != cmd-SCp.this_residual)
-   MER_PRINTK(merged %d buffers from %p, new length %08x\n,
+   dprintk(NDEBUG_MERGING, merged %d buffers from %p, new length 
%08x\n,
   cnt, cmd-SCp.ptr, cmd-SCp.this_residual);
  #endif
  }
@@ -676,7 +676,7 @@ static inline void NCR5380_all_init(void
  {
static int done = 0;
if (!done) {
-   INI_PRINTK(scsi : NCR5380_all_init()\n);
+   dprintk(NDEBUG_INIT, scsi : NCR5380_all_init()\n);
done = 1;
}
  }
@@ -984,7 +984,7 @@ static int NCR5380_queue_command_lck(Scs
}
local_irq_restore(flags);
  
-	QU_PRINTK(scsi%d: command added

Re: [PATCH v2 10/12] scsi/NCR5380: remove unused macro definitions

[Michael Schmitz]

Acked-by: MIchael Schmitz schm...@debian.org


Remove the unused (and divergent) debugging macro definitions from
the sun3_NCR5380 and atari_NCR5380 drivers. These drivers have been
converted to use the common macros in NCR5380.h.

Signed-off-by: Finn Thain fth...@telegraphics.com.au

---
  drivers/scsi/atari_scsi.h |   93 ---
  drivers/scsi/sun3_scsi.h  |  181 
--
  2 files changed, 274 deletions(-)

Index: linux-m68k/drivers/scsi/atari_scsi.h
===
--- linux-m68k.orig/drivers/scsi/atari_scsi.h   2014-03-19 23:23:02.0 
+1100
+++ linux-m68k/drivers/scsi/atari_scsi.h2014-03-19 23:34:45.0 
+1100
@@ -80,99 +80,6 @@
  #define SCSI_RESET_HOST_RESET 0x200
  #define SCSI_RESET_ACTION   0xff
  
-/* Debugging printk definitions:

- *
- *  ARB  - arbitration
- *  ASEN - auto-sense
- *  DMA  - DMA
- *  HSH  - PIO handshake
- *  INF  - information transfer
- *  INI  - initialization
- *  INT  - interrupt
- *  LNK  - linked commands
- *  MAIN - NCR5380_main() control flow
- *  NDAT - no data-out phase
- *  NWR  - no write commands
- *  PIO  - PIO transfers
- *  PDMA - pseudo DMA (unused on Atari)
- *  QU   - queues
- *  RSL  - reselections
- *  SEL  - selections
- *  USL  - usleep cpde (unused on Atari)
- *  LBS  - last byte sent (unused on Atari)
- *  RSS  - restarting of selections
- *  EXT  - extended messages
- *  ABRT - aborting and resetting
- *  TAG  - queue tag handling
- *  MER  - merging of consec. buffers
- *
- */
-
-#define dprint(flg, format...) \
-({ \
-   if (NDEBUG  (flg)) \
-   printk(KERN_DEBUG format);  \
-})
-
-#define ARB_PRINTK(format, args...) \
-   dprint(NDEBUG_ARBITRATION, format , ## args)
-#define ASEN_PRINTK(format, args...) \
-   dprint(NDEBUG_AUTOSENSE, format , ## args)
-#define DMA_PRINTK(format, args...) \
-   dprint(NDEBUG_DMA, format , ## args)
-#define HSH_PRINTK(format, args...) \
-   dprint(NDEBUG_HANDSHAKE, format , ## args)
-#define INF_PRINTK(format, args...) \
-   dprint(NDEBUG_INFORMATION, format , ## args)
-#define INI_PRINTK(format, args...) \
-   dprint(NDEBUG_INIT, format , ## args)
-#define INT_PRINTK(format, args...) \
-   dprint(NDEBUG_INTR, format , ## args)
-#define LNK_PRINTK(format, args...) \
-   dprint(NDEBUG_LINKED, format , ## args)
-#define MAIN_PRINTK(format, args...) \
-   dprint(NDEBUG_MAIN, format , ## args)
-#define NDAT_PRINTK(format, args...) \
-   dprint(NDEBUG_NO_DATAOUT, format , ## args)
-#define NWR_PRINTK(format, args...) \
-   dprint(NDEBUG_NO_WRITE, format , ## args)
-#define PIO_PRINTK(format, args...) \
-   dprint(NDEBUG_PIO, format , ## args)
-#define PDMA_PRINTK(format, args...) \
-   dprint(NDEBUG_PSEUDO_DMA, format , ## args)
-#define QU_PRINTK(format, args...) \
-   dprint(NDEBUG_QUEUES, format , ## args)
-#define RSL_PRINTK(format, args...) \
-   dprint(NDEBUG_RESELECTION, format , ## args)
-#define SEL_PRINTK(format, args...) \
-   dprint(NDEBUG_SELECTION, format , ## args)
-#define USL_PRINTK(format, args...) \
-   dprint(NDEBUG_USLEEP, format , ## args)
-#define LBS_PRINTK(format, args...) \
-   dprint(NDEBUG_LAST_BYTE_SENT, format , ## args)
-#define RSS_PRINTK(format, args...) \
-   dprint(NDEBUG_RESTART_SELECT, format , ## args)
-#define EXT_PRINTK(format, args...) \
-   dprint(NDEBUG_EXTENDED, format , ## args)
-#define ABRT_PRINTK(format, args...) \
-   dprint(NDEBUG_ABORT, format , ## args)
-#define TAG_PRINTK(format, args...) \
-   dprint(NDEBUG_TAGS, format , ## args)
-#define MER_PRINTK(format, args...) \
-   dprint(NDEBUG_MERGING, format , ## args)
-
-/* conditional macros for NCR5380_print_{,phase,status} */
-
-#define NCR_PRINT(mask)\
-   ((NDEBUG  (mask)) ? NCR5380_print(instance) : (void)0)
-
-#define NCR_PRINT_PHASE(mask) \
-   ((NDEBUG  (mask)) ? NCR5380_print_phase(instance) : (void)0)
-
-#define NCR_PRINT_STATUS(mask) \
-   ((NDEBUG  (mask)) ? NCR5380_print_status(instance) : (void)0)
-
-
  #endif /* ndef ASM */
  #endif /* ATARI_SCSI_H */
  
Index: linux-m68k/drivers/scsi/sun3_scsi.h

===
--- linux-m68k.orig/drivers/scsi/sun3_scsi.h2014-03-19 23:34:41.0 
+1100
+++ linux-m68k/drivers/scsi/sun3_scsi.h 2014-03-19 23:34:45.0 +1100
@@ -182,188 +182,7 @@ struct sun3_udc_regs {
  
  #define VME_DATA24 0x3d00
  
-// debugging printk's, taken from atari_scsi.h

-/* Debugging printk definitions:
- *
- *  ARB  - arbitration
- *  ASEN - auto-sense
- *  DMA  - DMA
- *  HSH  - PIO handshake
- *  INF  - information transfer
- *  INI  - initialization
- *  INT  - interrupt
- *  LNK  - linked commands
- *  MAIN - NCR5380_main() control flow
- *  NDAT - no data-out phase
- *  NWR  - no write

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Tuomas,


My preference would be to set this one (named ESP_CONFIG3_TBMS). Your
opinion, Dave?

As seems to be agreed upon here, the SCSI2 bit in the CONFIG2 register
(ESP_CONFIG2_SCSI2ENAB) is only for when the chip is used in target
mode.  So it is not relevant for our discussion because this driver is
for initiator mode operation only.

But some pieces of documentation seem like they might not agree on
this point.

With respect to bit 3 in the config3 register, it can take on one of
two meaning depending upon chip revision.  As per  
ESP_CONFIG3_{TMS,FCLK}

it either controls fast SCSI clocking, or it enabled 3 byte message
recognition.

But oddly in the NCR53CX docs:

	http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/ 
NCR53C9X.txt


it speaks as if ESP_CONFIG3_TMS and ESP_CONFIG3_TENB are merely finer
grained versions of config2 register setting ESP_CONFIG2_SCSI2ENAB,
which enables both features.

Again I looked at the FreeBSD driver and for all chips after plain
esp100, they set ESP_CONFIG2_SCSI2ENAB.

Can we try testing the following patch?


esp_scsi: Set SCSI2 bit in config2 register.

This should allow proper recognition of 3 byte reselection
on all esp100a and later chips.

Reported-by: Kars de Jong jo...@linux-m68k.org
Reported-by: Michael Schmitz schmitz...@gmail.com
Signed-off-by: David S. Miller da...@davemloft.net

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 55548dc..16f69e0 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2160,7 +2160,7 @@ static void esp_get_revision(struct esp *esp)
 */
esp-rev = ESP100;
} else {
-   esp-config2 = 0;
+   esp-config2 = ESP_CONFIG2_SCSI2ENAB;
esp_set_all_config3(esp, 5);
esp-prev_cfg3 = 5;
esp_write8(esp-config2, ESP_CFG2);
@@ -2187,8 +2187,6 @@ static void esp_get_revision(struct esp *esp)
} else {
esp-rev = ESP236;
}
-   esp-config2 = 0;
-   esp_write8(esp-config2, ESP_CFG2);
}
}
  }


I'll test these out soon.

Michael, where can I pull the latest version of zorro_esp?



Not sure my patch had ever made it into Geert's m68k-queue - except for  
the patch in my previous mail, my zorro_esp.c is still the same as I  
got from you in October last year. The project has been on the back  
burner for too long ...


I'll look into setting up pull access to my repository. zorro_esp.c as  
of today attached for now.


Cheers,

Michael



 /* zorrro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
 *
 * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
 *
 * Copyright (C) 2011 Michael Schmitz (schm...@debian.org) for 
 *   migration to ESP SCSI core
 */
/*
 * ZORRO bus code from:
 */
/*
 * Detection routine for the NCR53c710 based Amiga SCSI Controllers for Linux.
 *  Amiga MacroSystemUS WarpEngine SCSI controller.
 *  Amiga Technologies/DKB A4091 SCSI controller.
 *
 * Written 1997 by Alan Hourihane al...@fairlite.demon.co.uk
 * plus modifications of the 53c7xx.c driver to support the Amiga.
 *
 * Rewritten to use 53c700.c by Kars de Jong jo...@linux-m68k.org
 */

#include linux/module.h
#include linux/init.h
#include linux/interrupt.h
#include linux/ratelimit.h
#include linux/dma-mapping.h
#include linux/scatterlist.h
#include linux/delay.h
#include linux/zorro.h
#include linux/slab.h


#include asm/page.h
#include asm/pgtable.h
#include asm/cacheflush.h
#include asm/amigahw.h
#include asm/amigaints.h

#include scsi/scsi_host.h
#include scsi/scsi_transport_spi.h
#include scsi/scsi_device.h
#include scsi/scsi_tcq.h

#include esp_scsi.h

MODULE_AUTHOR(Michael Schmitz schm...@debian.org);
MODULE_DESCRIPTION(Amiga Zorro NCR5C9x (ESP) driver);
MODULE_LICENSE(GPL);

#define DMA_WRITE 0x8000

static struct zorro_driver_data {
const char *name;
unsigned long offset;
unsigned long dma_offset;
int absolute;
int zorro3; /* offset is absolute address */
} zorro_esp_driver_data[] = {
{ .name = CyberStormI, .offset = 0xf400, .dma_offset = 0xf800, 
.absolute = 0, .zorro3 = 0 },
{ .name = CyberStormII, .offset = 0x1ff03, .dma_offset = 0x1ff43, 
.absolute = 0, .zorro3 = 0 },
{ .name = Blizzard 2060, .offset = 0x1ff00, .dma_offset = 0x1ffe0, 
.absolute = 0, .zorro3 = 0 },
{ .name = Blizzard 1230, .offset = 0x8000, .dma_offset = 0x1, 
.absolute = 0, .zorro3 = 0 },
{ .name = Blizzard 1230II, .offset = 0x1, .dma_offset = 0x10021, 
.absolute = 0, .zorro3 = 0 },
{ .name = Fastlane, .offset = 0x101, .dma_offset = 0x141, 
.absolute = 0, .zorro3 = 1 },
{ 0 }
};

static struct zorro_device_id zorro_esp_zorro_tbl[] = {
{
.id

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Dave,

When this bit is set, the 53CF94/96 can receive 3-byte messages  
during
bus-initiated Select With ATN. This feature is also enabled by  
setting

bit 3 in the Configuration 2 register.


My preference would be to set this one (named ESP_CONFIG3_TBMS). Your
opinion, Dave?


As seems to be agreed upon here, the SCSI2 bit in the CONFIG2 register
(ESP_CONFIG2_SCSI2ENAB) is only for when the chip is used in target
mode.  So it is not relevant for our discussion because this driver is
for initiator mode operation only.


Agreed. ESP_CONFIG2_SCSI2ENAB might do more than we intend, and have  
unintended side effects. It's just easier to test whether this fixes  
our problem.




But some pieces of documentation seem like they might not agree on
this point.

With respect to bit 3 in the config3 register, it can take on one of
two meaning depending upon chip revision.  As per  
ESP_CONFIG3_{TMS,FCLK}

it either controls fast SCSI clocking, or it enabled 3 byte message
recognition.

But oddly in the NCR53CX docs:

	http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/ 
NCR53C9X.txt


it speaks as if ESP_CONFIG3_TMS and ESP_CONFIG3_TENB are merely finer
grained versions of config2 register setting ESP_CONFIG2_SCSI2ENAB,
which enables both features.


That's what I understood from the bits Kars quoted, yes. And in the  
Amiga driver cases, it will always be the 3 byte message feature  
controlled by that bit, as far as I can see.




Again I looked at the FreeBSD driver and for all chips after plain
esp100, they set ESP_CONFIG2_SCSI2ENAB.

Can we try testing the following patch?


That would be even easier than setting it explicitly in the Zorro  
driver, thanks,


Michael





esp_scsi: Set SCSI2 bit in config2 register.

This should allow proper recognition of 3 byte reselection
on all esp100a and later chips.

Reported-by: Kars de Jong jo...@linux-m68k.org
Reported-by: Michael Schmitz schmitz...@gmail.com
Signed-off-by: David S. Miller da...@davemloft.net

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 55548dc..16f69e0 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2160,7 +2160,7 @@ static void esp_get_revision(struct esp *esp)
 */
esp-rev = ESP100;
} else {
-   esp-config2 = 0;
+   esp-config2 = ESP_CONFIG2_SCSI2ENAB;
esp_set_all_config3(esp, 5);
esp-prev_cfg3 = 5;
esp_write8(esp-config2, ESP_CFG2);
@@ -2187,8 +2187,6 @@ static void esp_get_revision(struct esp *esp)
} else {
esp-rev = ESP236;
}
-   esp-config2 = 0;
-   esp_write8(esp-config2, ESP_CFG2);
}
}
 }


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hi Kars,

thanks for the PDFs!

 Bit 2 SCSI-2

 Setting this bit allows the FSC to support two new features adopted in
 SCSI-2: the 3-byte message exchange for Tagged-Queueing and Group 2
 commands. These features can also be set independently in the Config 3
 register.

 Tagged-Queueing
 When this bit is set and the FSC is selected with ATN (Attention), it
 will request either one or three message bytes depending on whether
 ATN remains true or goes false. If ATN is still true after the first
 byte has been received, the FSC may request two more message bytes
 before switching to Command phase. If ATN goes false, it will switch
 to Command phase after the first message byte. When the bit is not set
 it will request a single message byte (as a target) when selected with
 ATN, and abort the selection sequence (as an initiator) if the target
 does not switch to Command phase after one message byte has been
 transferred.

That appears to be our problem if I recall correctly Tuomas' debugging
report. (reselection, not selection as initiator). As
esp_slave_configure() enables queue tags regardless of chip config,
we'd best make certain the chip is configured correctly.

The SCSI2 bit is used to test for presence of config register 2 in
esp_get_revision but later cleared in the same function. It appears
we'd need to set it after the call to scsi_esp_register() - can you
test whether that obsoletes the zorro_esp_slave_configure hack,
Tuomas?

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index 1a1eb95..b33c3b5 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -418,9 +418,6 @@ static int zorro_esp_init_one(struct zorro_dev *z,
return -EBUSY;
}

-   /* Fill in the required pieces of hostdata */
-   scsi_esp_template.slave_configure = zorro_esp_slave_configure;
-
host = scsi_host_alloc(tpnt, sizeof(struct esp));

if (!host) {
@@ -508,6 +505,10 @@ static int zorro_esp_init_one(struct zorro_dev *z,
if (err)
goto fail_free_irq;

+   esp-config2 = ESP_CONFIG2_SCSI2ENAB;
+   zorro_esp_write8(esp, esp-config2, ESP_CFG2);
+
+
zorro_set_drvdata(z, host);

return 0;


 Group 2 Commands
 (seems to only be relevant for target mode).

 And about the QTE bit:

 Bit 6 Queue Tag Enable

 When this bit is set, the 53CF94/96 can receive 3-byte messages during
 bus-initiated Select With ATN. This feature is also enabled by setting
 bit 3 in the Configuration 2 register.

My preference would be to set this one (named ESP_CONFIG3_TBMS). Your
opinion, Dave?

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hello Kars,


  I've never seen a formula for any ESP or FAS chip for the timeout
  other than the one mentioned in huge comment in
  esp_set_clock_params(), although I do see the 7668 instead of 8192
  factor being used in the old NCR53C9x driver.

 I haven't gone far enough back in the 53C9x revision history to be
 certain. but it would seem to me that Kars de Jong added that FAS
 special case.

 Can you confirm that, Kars? Any recollection as to the reason?

 That is the value that's in the data manual of the Symbios Logic
 SYM53CF94/96-2 (the actual chip that's in my Amiga SCSI controller).

 Funny, according to the QLogic FAS2x6 manual the value should be 7682
 for FAS216/216U/236/236U chips...

 I don't think it's all that important. It only means that the actual
 selection timeout used by the chip will be slightly shorter than it is
 supposed to be.

Thanks for checking that - I agree that it might not amount to much.

The more important issue is the one about the one-byte reconnect
message. Does the manual speak to that particular issue? Any hint on
how we could enable SCSI-2 features on chip init?

Can you point me to a source for the manuals if possible?

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216

[Michael Schmitz]

Hello Dave, Tuomas,

 Also, looking at the timeout formulae in the old NCR53C9x.c driver,
 the values would be different for FAS216. Why was this dropped from
 the modern esp_scsi?

 I've never seen a formula for any ESP or FAS chip for the timeout
 other than the one mentioned in huge comment in
 esp_set_clock_params(), although I do see the 7668 instead of 8192
 factor being used in the old NCR53C9x driver.

I haven't gone far enough back in the 53C9x revision history to be
certain. but it would seem to me that Kars de Jong added that FAS
special case.

Can you confirm that, Kars? Any recollection as to the reason?

Cheers,

  Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/4] m68k/atari - atari_scsi: use correct virt/phys translation for DMA buffer

[Michael Schmitz]

With the kernel running from FastRAM instead of ST-RAM, none of ST-RAM is
mapped by mem_init, and DMA-addressable buffer must be mapped by ioremap.

Use platform specific virt/phys translation helpers for this case.

Signed-off-by: Michael Schmitz schm...@debian.org
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_scsi.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 296c936..a8d721f 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -639,7 +639,7 @@ static int __init atari_scsi_detect(struct 
scsi_host_template *host)
double buffer\n);
return 0;
}
-   atari_dma_phys_buffer = virt_to_phys(atari_dma_buffer);
+   atari_dma_phys_buffer = atari_stram_to_phys(atari_dma_buffer);
atari_dma_orig_addr = 0;
}
 #endif
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

Finn,

nothings' been merged yet, still pending review from the SCSI team. 
Comments below.




This is a larger version of Michael's patch. It takes care of the 
header
files and comments and it addresses sun3_NCR5380 as well as 
atari_NCR5380.
This means that the initio.h include (!) can be dropped from 
sun3_scsi.h.


Signed-off-by: Finn Thain fth...@telegraphics.com.au

---

Michael, I'm assuming that your patch hasn't been merged already (I 
didn't
find it in any of the likely repos). Please go ahead and add your 
sign-off
on this version if it is satisfactory. I didn't test this patch: I 
presume
that sun3_NCR5380 would need the same abort/reset fixes that 
atari_NCR5380

needed...


diff --git a/drivers/scsi/atari_NCR5380.c 
b/drivers/scsi/atari_NCR5380.c

index 0f3cdbc..e4aaf9a 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
cmd-scsi_done(cmd);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
} else {
 /* local_irq_restore(flags); */
printk(scsi%d: abort of connected command failed!\n, 
HOSTNO);
-   return SCSI_ABORT_ERROR;
+   return FAILED;
}
}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 * yet... */
tmp-scsi_done(tmp);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}

@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
if (hostdata-connected) {
local_irq_restore(flags);
ABRT_PRINTK(scsi%d: abort failed, command connected.\n, 
HOSTNO);
-   return SCSI_ABORT_SNOOZE;
+   return FAILED;
}

/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
ABRT_PRINTK(scsi%d: aborting disconnected command.\n, 
HOSTNO);

if (NCR5380_select(instance, cmd, (int)cmd-tag))
-   return SCSI_ABORT_BUSY;
+   return FAILED;

ABRT_PRINTK(scsi%d: nexus reestablished.\n, HOSTNO);

@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
tmp-scsi_done(tmp);

falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 */
falcon_release_lock_if_possible(hostdata);

-   return SCSI_ABORT_NOT_RUNNING;
+   return FAILED;
 }


@@ -2825,7 +2825,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
  *
  * Purpose : reset the SCSI bus.
  *
- * Returns : SCSI_RESET_WAKEUP
+ * Returns : SUCCESS or FAILURE
  *
  */

@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
SETUP_HOSTDATA(cmd-device-host);
int i;
unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
Scsi_Cmnd *connected, *disconnected_queue;
 #endif

@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * through anymore ... */
(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);

-#if 1	/* XXX Should now be done by midlevel code, but it's broken XXX 
*/

+   /* MSch 20140115 - looking at the generic NCR5380 driver, all of this
+* should go.
+* Catch-22: if we don't clear all queues, the SCSI driver lock will
+* not be reset by atari_scsi_reset()!


Bugger - typo of mine, meant to say 'released' not 'reset'.


+*/
+
+#if defined(RESET_RUN_DONE)
+   /* XXX Should now be done by midlevel code, but it's broken XXX */
/* XXX see belowXXX */

 	/* MSch: old-style reset: actually abort all command processing here 
*/

@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * the midlevel code that the reset was SUCCESSFUL, and there is no
 * need to 'wake up' the commands by a request_sense
 */
-   return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #else /* 1 */

 	/* MSch: new-style reset handling: let the mid-level do what it can 
*/

@@ -2963,6 +2970,6 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
local_irq_restore(flags);

 	/* we did no complete reset of all commands, so a wakeup is required 
*/

-   return SCSI_RESET_WAKEUP | SCSI_RESET_BUS_RESET;

Re: [PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

Bugger - forgot to CC Sammy as well.

My only comment of substance is that the reset handling doesn't need to 
be done the same way as on Atari, as there's no special locking to 
account for.


Cheers,

Michael

Am 11.03.2014 um 21:28 schrieb Geert Uytterhoeven:


CC Sammy

On Tue, Mar 11, 2014 at 9:21 AM, Finn Thain 
fth...@telegraphics.com.au wrote:


This is a larger version of Michael's patch. It takes care of the 
header
files and comments and it addresses sun3_NCR5380 as well as 
atari_NCR5380.
This means that the initio.h include (!) can be dropped from 
sun3_scsi.h.


Signed-off-by: Finn Thain fth...@telegraphics.com.au

---

Michael, I'm assuming that your patch hasn't been merged already (I 
didn't
find it in any of the likely repos). Please go ahead and add your 
sign-off
on this version if it is satisfactory. I didn't test this patch: I 
presume
that sun3_NCR5380 would need the same abort/reset fixes that 
atari_NCR5380

needed...


diff --git a/drivers/scsi/atari_NCR5380.c 
b/drivers/scsi/atari_NCR5380.c

index 0f3cdbc..e4aaf9a 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
cmd-scsi_done(cmd);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
} else {
 /* local_irq_restore(flags); */
printk(scsi%d: abort of connected command 
failed!\n, HOSTNO);

-   return SCSI_ABORT_ERROR;
+   return FAILED;
}
}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 * yet... */
tmp-scsi_done(tmp);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}

@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
if (hostdata-connected) {
local_irq_restore(flags);
ABRT_PRINTK(scsi%d: abort failed, command 
connected.\n, HOSTNO);

-   return SCSI_ABORT_SNOOZE;
+   return FAILED;
}

/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
ABRT_PRINTK(scsi%d: aborting disconnected 
command.\n, HOSTNO);


if (NCR5380_select(instance, cmd, 
(int)cmd-tag))

-   return SCSI_ABORT_BUSY;
+   return FAILED;

ABRT_PRINTK(scsi%d: nexus reestablished.\n, 
HOSTNO);


@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
tmp-scsi_done(tmp);

falcon_release_lock_if_possible(hostdata);

-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 */
falcon_release_lock_if_possible(hostdata);

-   return SCSI_ABORT_NOT_RUNNING;
+   return FAILED;
 }


@@ -2825,7 +2825,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
  *
  * Purpose : reset the SCSI bus.
  *
- * Returns : SCSI_RESET_WAKEUP
+ * Returns : SUCCESS or FAILURE
  *
  */

@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
SETUP_HOSTDATA(cmd-device-host);
int i;
unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
Scsi_Cmnd *connected, *disconnected_queue;
 #endif

@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * through anymore ... */
(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);

-#if 1  /* XXX Should now be done by midlevel code, but it's broken 
XXX */
+   /* MSch 20140115 - looking at the generic NCR5380 driver, all 
of this

+* should go.
+* Catch-22: if we don't clear all queues, the SCSI driver 
lock will

+* not be reset by atari_scsi_reset()!
+*/
+
+#if defined(RESET_RUN_DONE)
+   /* XXX Should now be done by midlevel code, but it's broken 
XXX */
/* XXX see below
XXX */


/* MSch: old-style reset: actually abort all command 
processing here */

@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * the midlevel code that the reset was SUCCESSFUL, and there 
is no

 * need to 'wake up' the commands by a request_sense
 */
-   return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #else /* 1 */

/* MSch: new-style reset handling: let the mid-level do what 
it can */

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Hello Arnd,

On Thursday 27 February 2014, Michael Schmitz wrote:
  

Arnd Bergmann wrote:

  
  
Nack - the completion condition in the first hunk has its logic 
reversed. Try this instead (while() loops while condition true, do {} 
until () loops while condition false, no?)



Sorry about messing it up again. I though I had fixed it up the
way you commented when you said it worked.
 
  
I'm 99% confident I had tested your current version of the patch before 
and found it still attempts to schedule while in interrupt. I can retest 
if you prefer, but that'll have to wait a few days.



I definitely trust you to have the right version, since you did the
testing.
  


I'm glad I double checked, since there's one other error left in my 
correction to your patch below:


The in_irq() condition is not sufficient, we need in_interrupt() there. 
This has somehow slipped into a related patch sent to linux-scsi, so 
I'll have to refactor the lot. Bugger.


I'll resend the correct version via Geert.

  

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include linux/init.h
 #include linux/nvram.h
 #include linux/bitops.h
+#include linux/wait.h
 
 #include asm/setup.h

 #include asm/atarihw.h
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
 
local_irq_save(flags);
 
-   while (!in_irq()  falcon_got_lock  stdma_others_waiting())

-   sleep_on(falcon_fairness_wait);
+   wait_event_cmd(falcon_fairness_wait,
+   in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+   local_irq_restore(flags),
+   local_irq_save(flags));
 
while (!falcon_got_lock) {

if (in_irq())



Yes, by inspection your version looks correct and mine looks wrong.
I had figured this out before, just sent the wrong version.
  


These things happen if you bother fixing other people's weird code :-)
And as I mentioned above, I missed another detail myself

  

@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
falcon_trying_lock = 0;
wake_up(falcon_try_wait);
} else {
-   sleep_on(falcon_try_wait);
+   wait_event_cmd(falcon_try_wait,
+   falcon_got_lock  !falcon_trying_lock,
+   local_irq_restore(flags),
+   local_irq_save(flags));
}



I did correct this part compared to my first patch, but forgot
to change the other hunk.

Can you send your version of the patch to Geert for inclusion?
That way I don't have the danger of missing another negation.
This code is clearly too weird to rely on inspection alone and
we know that your version was working when you last tested it.
  


Will do - I'll CC: you in so you can ACK the patch if Geert needs 
convincing.


Cheers,

   Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

The abort/reset lowlevel return codes had changed with the new
error SCSI handling - update Atari NCR5380 driver to reflect this.

Change reset handling to clear queues only, do not attempt to
call done() on each command aborted by the reset. The EH code
should do that for us.
Queues _must_ be cleared, otherwise atari_scsi_bus_reset will not
release the ST-DMA lock, deadlocking further error recovery.

Signed-off-by: Michael Schmitz schm...@debian.org
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_NCR5380.c |   29 ++---
 drivers/scsi/atari_scsi.c|2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 0f3cdbc..465e63d 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
cmd-scsi_done(cmd);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
} else {
 /* local_irq_restore(flags); */
printk(scsi%d: abort of connected command failed!\n, 
HOSTNO);
-   return SCSI_ABORT_ERROR;
+   return FAILED;
}
}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 * yet... */
tmp-scsi_done(tmp);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
 
@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
if (hostdata-connected) {
local_irq_restore(flags);
ABRT_PRINTK(scsi%d: abort failed, command connected.\n, 
HOSTNO);
-   return SCSI_ABORT_SNOOZE;
+   return FAILED;
}
 
/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
ABRT_PRINTK(scsi%d: aborting disconnected command.\n, 
HOSTNO);
 
if (NCR5380_select(instance, cmd, (int)cmd-tag))
-   return SCSI_ABORT_BUSY;
+   return FAILED;
 
ABRT_PRINTK(scsi%d: nexus reestablished.\n, HOSTNO);
 
@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
tmp-scsi_done(tmp);

falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 */
falcon_release_lock_if_possible(hostdata);
 
-   return SCSI_ABORT_NOT_RUNNING;
+   return FAILED;
 }
 
 
@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
SETUP_HOSTDATA(cmd-device-host);
int i;
unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
Scsi_Cmnd *connected, *disconnected_queue;
 #endif
 
@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * through anymore ... */
(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-#if 1  /* XXX Should now be done by midlevel code, but it's broken XXX */
+   /* MSch 20140115 - looking at the generic NCR5380 driver, all of this
+* should go.
+* Catch-22: if we don't clear all queues, the SCSI driver lock will
+* not be reset by atari_scsi_reset()!
+*/
+
+#if defined(RESET_RUN_DONE)
+   /* XXX Should now be done by midlevel code, but it's broken XXX */
/* XXX see belowXXX */
 
/* MSch: old-style reset: actually abort all command processing here */
@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * the midlevel code that the reset was SUCCESSFUL, and there is no
 * need to 'wake up' the commands by a request_sense
 */
-   return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #else /* 1 */
 
/* MSch: new-style reset handling: let the mid-level do what it can */
@@ -2963,6 +2970,6 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
local_irq_restore(flags);
 
/* we did no complete reset of all commands, so a wakeup is required */
-   return SCSI_RESET_WAKEUP | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #endif /* 1 */
 }
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 296c936..4ae0c1a 100644
--- a/drivers/scsi

[PATCH 0/3] m68k Atari SCSI fixes

[Michael Schmitz]

All, 

please review the following patches to the m68k Atari NCR5380 SCSI code. 

The first patch is my corrected version of Arnd Bergmann's sleep_on removal RFC
series (2/16), tested on my Atari Falcon. Geert, please take this one through 
the   
linux-m68k tree so Arnd's sleep_on remove can go ahead. 

The second and third one are updates to the Atari NCR5380 drivers' error 
handling
(return codes updated) and queue_command() function (return 
SCSI_MLQEUE_HOST_BUSY  
if the lock for the SCSI/DMA hardware cannot be safely taken). Again, tested on 
my 
Falcon. These will need Ack from the SCSI maintainers but could be taken through
Geert's tree as well. 

This patch series supersedes a previous one of mine, due to a botch-up mingling 
parts
of patch 1 and 3. Apologies for spamming everyone again.

Please CC: me in on any replies, I am not subscribed to linux-scsi.

Thanks,

Michael Schmitz 

[PATCH 1/3] m68k/atari - convert atari_scsi falcon_get_lock() to use 
wait_event()
[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes
[PATCH 3/3] m68k/atari - atari_scsi: punt if deadlocked
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] m68k/atari - convert atari_scsi falcon_get_lock() to use wait_event()

[Michael Schmitz]

sleep_on is known broken and going away. The atari_scsi driver is one of
two remaining users in the falcon_get_lock() function, which is a rather
crazy piece of code. This does not attempt to fix the driver's locking
scheme in general, but at least prevents falcon_get_lock from going to
sleep when no other thread holds the same lock or tries to get it,
and we no longer schedule with irqs disabled.

MSch: fixed completion conditions missed in Arnds' original RFC patch.

Signed-off-by: Arnd Bergmann a...@arndb.de
Acked-by: Michael Schmitz schm...@debian.org
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_scsi.c |   12 +---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..296c936 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include linux/init.h
 #include linux/nvram.h
 #include linux/bitops.h
+#include linux/wait.h
 
 #include asm/setup.h
 #include asm/atarihw.h
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
 
local_irq_save(flags);
 
-   while (!in_irq()  falcon_got_lock  stdma_others_waiting())
-   sleep_on(falcon_fairness_wait);
+   wait_event_cmd(falcon_fairness_wait,
+   in_interrupt() || !falcon_got_lock || !stdma_others_waiting(),
+   local_irq_restore(flags),
+   local_irq_save(flags));
 
while (!falcon_got_lock) {
if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
falcon_trying_lock = 0;
wake_up(falcon_try_wait);
} else {
-   sleep_on(falcon_try_wait);
+   wait_event_cmd(falcon_try_wait,
+   falcon_got_lock  !falcon_trying_lock,
+   local_irq_restore(flags),
+   local_irq_save(flags));
}
}
 
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] m68k/atari - atari_scsi: punt if deadlocked

[Michael Schmitz]

In case a SCSI command is queued from softirq context, and another
driver currently holds the ST-DMA lock, tell the SCSI midlevel to
hold off queueing commands for now. Midlevel will resume play later.

Signed-off-by: Michael Schmitz schm...@debian.org
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_NCR5380.c |   12 +---
 drivers/scsi/atari_scsi.c|   24 
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 465e63d..90a90e8 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -967,9 +967,15 @@ static int NCR5380_queue_command_lck(Scsi_Cmnd *cmd, void 
(*done)(Scsi_Cmnd *))
 * alter queues and touch the lock.
 */
if (!IS_A_TT()) {
-   /* perhaps stop command timer here */
-   falcon_get_lock();
-   /* perhaps restart command timer here */
+   /* MSch 20140119: check whether obtaining the ST-DMA lock did
+* succeed.
+* If the lock could not be acquired without risking to
+* deadlock, i.e. from softirq context with ST-DMA currently
+* otherwise locked, defer queueing further commands.
+*/
+   if (falcon_get_lock()) {
+   return SCSI_MLQUEUE_HOST_BUSY;
+   }
}
if (!(hostdata-issue_queue) || (cmd-cmnd[0] == REQUEST_SENSE)) {
LIST(cmd, hostdata-issue_queue);
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 4ae0c1a..30c7385 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -197,7 +197,7 @@ static unsigned long atari_dma_xfer_len(unsigned long 
wanted_len,
 static irqreturn_t scsi_tt_intr(int irq, void *dummy);
 static irqreturn_t scsi_falcon_intr(int irq, void *dummy);
 static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata);
-static void falcon_get_lock(void);
+static int falcon_get_lock(void);
 #ifdef CONFIG_ATARI_SCSI_RESET_BOOT
 static void atari_scsi_reset_boot(void);
 #endif
@@ -541,12 +541,12 @@ static void falcon_release_lock_if_possible(struct 
NCR5380_hostdata *hostdata)
  * Complicated, complicated Sigh...
  */
 
-static void falcon_get_lock(void)
+static int falcon_get_lock(void)
 {
unsigned long flags;
 
if (IS_A_TT())
-   return;
+   return 0;
 
local_irq_save(flags);
 
@@ -557,8 +557,23 @@ static void falcon_get_lock(void)
 
while (!falcon_got_lock) {
if (in_irq())
-   panic(Falcon SCSI hasn't ST-DMA lock in interrupt);
+   panic(Falcon SCSI hasn't got ST-DMA lock in irq);
if (!falcon_trying_lock) {
+   if (in_interrupt()) {
+   if (stdma_islocked()) {
+   /* MSch 20140119: problem -
+* cannot schedule in interrupt!
+* Unless stdma_lock can be modified
+* to interruptible wait, this will
+* be needed to avoid deadlocking here!
+* Return error to indicate command
+* queueing would deadlock, and allow
+* queue_command() to stall queueing.
+*/
+   local_irq_restore(flags);
+   return 1;
+   }
+   }
falcon_trying_lock = 1;
stdma_lock(scsi_falcon_intr, NULL);
falcon_got_lock = 1;
@@ -575,6 +590,7 @@ static void falcon_get_lock(void)
local_irq_restore(flags);
if (!falcon_got_lock)
panic(Falcon SCSI: someone stole the lock :-(\n);
+   return 0;
 }
 
 
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/16] sleep_on removal, second try

[Michael Schmitz]

Arnd,



It's been a while since the first submission of these patches,
but a lot of them have made it into linux-next already, so here
is the stuff that is not merged yet, hopefully addressing all
the comments.

Geert and Michael: the I was expecting the ataflop and atari_scsi
patches to be merged already, based on earlier discussion.
Can you apply them to the linux-m68k tree, or do you prefer
them to go through the scsi and block maintainers?


Not sure what we decided to do - I'd prefer to double-check the latest 
ones first, but I'd be OK with these to go via m68k.


Maybe Geert waits for acks from linux-scsi and linux-block? (The rest 
of my patches to Atari SCSI still awaits comment there.)


Geert?

Regards,

Michael


Jens: I did not get any comments for the DAC960 and swim3 patches,
I assume they are good to go in. Please merge.

Hans and Mauro: As I commented on the old thread, I thought the
four media patches were on their way. I have addressed the one
comment that I missed earlier now, and used Hans' version for
the two patches he changed. Please merge or let me know the status
if you have already put them in some tree, but not yet into linux-next

Greg or Andrew: The parport subsystem is orphaned unfortunately,
can one of you pick up that patch?

Davem: The two ATM patches got acks, but I did not hear back from
Karsten regarding the ISDN patches. Can you pick up all six, or
should we wait for comments about the ISDN patches?

Arnd

Cc: Andrew Morton a...@osdl.org
Cc: David S. Miller da...@davemloft.net
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Ingo Molnar mi...@kernel.org
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: Jens Axboe ax...@kernel.dk
Cc: Karsten Keil i...@linux-pingi.de
Cc: Mauro Carvalho Chehab m.che...@samsung.com
Cc: Michael Schmitz schm...@biophys.uni-duesseldorf.de
Cc: Peter Zijlstra pet...@infradead.org
Cc: linux-atm-gene...@lists.sourceforge.net
Cc: linux-me...@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: net...@vger.kernel.org

Arnd Bergmann (16):
  ataflop: fix sleep_on races
  scsi: atari_scsi: fix sleep_on race
  DAC960: remove sleep_on usage
  swim3: fix interruptible_sleep_on race
  [media] omap_vout: avoid sleep_on race
  [media] usbvision: drop unused define USBVISION_SAY_AND_WAIT
  [media] radio-cadet: avoid interruptible_sleep_on race
  [media] arv: fix sleep_on race
  parport: fix interruptible_sleep_on race
  atm: nicstar: remove interruptible_sleep_on_timeout
  atm: firestream: fix interruptible_sleep_on race
  isdn: pcbit: fix interruptible_sleep_on race
  isdn: hisax/elsa: fix sleep_on race in elsa FSM
  isdn: divert, hysdn: fix interruptible_sleep_on race
  isdn: fix multiple sleep_on races
  sched: remove sleep_on() and friends

 Documentation/DocBook/kernel-hacking.tmpl| 10 --
 drivers/atm/firestream.c |  4 +--
 drivers/atm/nicstar.c| 13 
 drivers/block/DAC960.c   | 34 ++--
 drivers/block/ataflop.c  | 16 +-
 drivers/block/swim3.c| 18 ++-
 drivers/isdn/divert/divert_procfs.c  |  7 +++--
 drivers/isdn/hisax/elsa.c|  9 --
 drivers/isdn/hisax/elsa_ser.c|  3 +-
 drivers/isdn/hysdn/hysdn_proclog.c   |  7 +++--
 drivers/isdn/i4l/isdn_common.c   | 13 +---
 drivers/isdn/pcbit/drv.c |  6 ++--
 drivers/media/platform/arv.c |  6 ++--
 drivers/media/platform/omap/omap_vout_vrfb.c |  3 +-
 drivers/media/radio/radio-cadet.c| 46 


 drivers/media/usb/usbvision/usbvision.h  |  8 -
 drivers/parport/share.c  |  3 +-
 drivers/scsi/atari_scsi.c| 12 ++--
 include/linux/wait.h | 11 ---
 kernel/sched/core.c  | 46 


 20 files changed, 113 insertions(+), 162 deletions(-)

--
1.8.3.2


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd Bergmann wrote:

sleep_on is known broken and going away. The atari_scsi driver is one of
two remaining users in the falcon_get_lock() function, which is a rather
crazy piece of code. This does not attempt to fix the driver's locking
scheme in general, but at least prevents falcon_get_lock from going to
sleep when no other thread holds the same lock or tries to get it,
and we no longer schedule with irqs disabled.

Signed-off-by: Arnd Bergmann a...@arndb.de
Cc: Michael Schmitz schmitz...@gmail.com
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_scsi.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..b33ce34 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include linux/init.h
 #include linux/nvram.h
 #include linux/bitops.h
+#include linux/wait.h
 
 #include asm/setup.h

 #include asm/atarihw.h
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
 
 	local_irq_save(flags);
 
-	while (!in_irq()  falcon_got_lock  stdma_others_waiting())

-   sleep_on(falcon_fairness_wait);
+   wait_event_cmd(falcon_fairness_wait,
+  !in_irq()  falcon_got_lock  stdma_others_waiting(),
+  local_irq_restore(flags),
+  local_irq_save(flags));
 
 	while (!falcon_got_lock) {

if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
falcon_trying_lock = 0;
wake_up(falcon_try_wait);
} else {
-   sleep_on(falcon_try_wait);
+   wait_event_cmd(falcon_try_wait,
+  falcon_got_lock  !falcon_trying_lock,
+  local_irq_restore(flags),
+  local_irq_save(flags));
}
}
 
  
Nack - the completion condition in the first hunk has its logic 
reversed. Try this instead (while() loops while condition true, do {} 
until () loops while condition false, no?)


I'm 99% confident I had tested your current version of the patch before 
and found it still attempts to schedule while in interrupt. I can retest 
if you prefer, but that'll have to wait a few days.


diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
#include linux/init.h
#include linux/nvram.h
#include linux/bitops.h
+#include linux/wait.h

#include asm/setup.h
#include asm/atarihw.h
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)

   local_irq_save(flags);

-   while (!in_irq()  falcon_got_lock  stdma_others_waiting())
-   sleep_on(falcon_fairness_wait);
+   wait_event_cmd(falcon_fairness_wait,
+   in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+   local_irq_restore(flags),
+   local_irq_save(flags));

   while (!falcon_got_lock) {
   if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
   falcon_trying_lock = 0;
   wake_up(falcon_try_wait);
   } else {
-   sleep_on(falcon_try_wait);
+   wait_event_cmd(falcon_try_wait,
+   falcon_got_lock  !falcon_trying_lock,
+   local_irq_restore(flags),
+   local_irq_save(flags));
   }
   }


Cheers,

   Michael

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] m68k/atari - atari_scsi lock fixes: punt if deadlocked

[Michael Schmitz]

In case a SCSI command is queued from softirq context, and another
driver currently holds the ST-DMA lock, tell the SCSI midlevel to
hold off queueing commands for now. Something will hopefully resume
play later.

Signed-off-by: Michael Schmitz schm...@debian.org
---
 drivers/scsi/atari_NCR5380.c |   12 +---
 drivers/scsi/atari_scsi.c|   26 +-
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 465e63d..90a90e8 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -967,9 +967,15 @@ static int NCR5380_queue_command_lck(Scsi_Cmnd *cmd, void 
(*done)(Scsi_Cmnd *))
 * alter queues and touch the lock.
 */
if (!IS_A_TT()) {
-   /* perhaps stop command timer here */
-   falcon_get_lock();
-   /* perhaps restart command timer here */
+   /* MSch 20140119: check whether obtaining the ST-DMA lock did
+* succeed.
+* If the lock could not be acquired without risking to
+* deadlock, i.e. from softirq context with ST-DMA currently
+* otherwise locked, defer queueing further commands.
+*/
+   if (falcon_get_lock()) {
+   return SCSI_MLQUEUE_HOST_BUSY;
+   }
}
if (!(hostdata-issue_queue) || (cmd-cmnd[0] == REQUEST_SENSE)) {
LIST(cmd, hostdata-issue_queue);
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 5e19509..2b62ce2 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -197,7 +197,7 @@ static unsigned long atari_dma_xfer_len(unsigned long 
wanted_len,
 static irqreturn_t scsi_tt_intr(int irq, void *dummy);
 static irqreturn_t scsi_falcon_intr(int irq, void *dummy);
 static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata);
-static void falcon_get_lock(void);
+static int falcon_get_lock(void);
 #ifdef CONFIG_ATARI_SCSI_RESET_BOOT
 static void atari_scsi_reset_boot(void);
 #endif
@@ -541,24 +541,39 @@ static void falcon_release_lock_if_possible(struct 
NCR5380_hostdata *hostdata)
  * Complicated, complicated Sigh...
  */
 
-static void falcon_get_lock(void)
+static int falcon_get_lock(void)
 {
unsigned long flags;
 
if (IS_A_TT())
-   return;
+   return 0;
 
local_irq_save(flags);
 
wait_event_cmd(falcon_fairness_wait,
-   in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+   in_interrupt() || !falcon_got_lock || !stdma_others_waiting(),
local_irq_restore(flags),
local_irq_save(flags));
 
while (!falcon_got_lock) {
if (in_irq())
-   panic(Falcon SCSI hasn't ST-DMA lock in interrupt);
+   panic(Falcon SCSI hasn't got ST-DMA lock in irq);
if (!falcon_trying_lock) {
+   if (in_interrupt()) {
+   if (stdma_islocked()) {
+   /* MSch 20140119: problem -
+* cannot schedule in interrupt!
+* Unless stdma_lock can be modified
+* to interruptible wait, this will
+* be needed to avoid deadlocking here!
+* Return error to indicate command
+* queueing would deadlock, and allow
+* queue_command() to stall queueing.
+*/
+   local_irq_restore(flags);
+   return 1;
+   }
+   }
falcon_trying_lock = 1;
stdma_lock(scsi_falcon_intr, NULL);
falcon_got_lock = 1;
@@ -575,6 +590,7 @@ static void falcon_get_lock(void)
local_irq_restore(flags);
if (!falcon_got_lock)
panic(Falcon SCSI: someone stole the lock :-(\n);
+   return 0;
 }
 
 
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

The abort/reset lowlevel return codes had changed with the new
error SCSI handling - update Atari NCR5380 driver to reflect this.

Change reset handling to clear queues only, do not attempt to
call done() on each command aborted by the reset. The EH code
should do that for us.
Queues _must_ be cleared, otherwise atari_scsi_bus_reset will not
release the ST-DMA lock, deadlocking further error recovery.

Signed-off-by: Michael Schmitz schm...@debian.org
---
 drivers/scsi/atari_NCR5380.c |   29 ++---
 drivers/scsi/atari_scsi.c|2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 0f3cdbc..465e63d 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
cmd-scsi_done(cmd);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
} else {
 /* local_irq_restore(flags); */
printk(scsi%d: abort of connected command failed!\n, 
HOSTNO);
-   return SCSI_ABORT_ERROR;
+   return FAILED;
}
}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 * yet... */
tmp-scsi_done(tmp);
falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
 
@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
if (hostdata-connected) {
local_irq_restore(flags);
ABRT_PRINTK(scsi%d: abort failed, command connected.\n, 
HOSTNO);
-   return SCSI_ABORT_SNOOZE;
+   return FAILED;
}
 
/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
ABRT_PRINTK(scsi%d: aborting disconnected command.\n, 
HOSTNO);
 
if (NCR5380_select(instance, cmd, (int)cmd-tag))
-   return SCSI_ABORT_BUSY;
+   return FAILED;
 
ABRT_PRINTK(scsi%d: nexus reestablished.\n, HOSTNO);
 
@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
local_irq_restore(flags);
tmp-scsi_done(tmp);

falcon_release_lock_if_possible(hostdata);
-   return SCSI_ABORT_SUCCESS;
+   return SUCCESS;
}
}
}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 */
falcon_release_lock_if_possible(hostdata);
 
-   return SCSI_ABORT_NOT_RUNNING;
+   return FAILED;
 }
 
 
@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
SETUP_HOSTDATA(cmd-device-host);
int i;
unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
Scsi_Cmnd *connected, *disconnected_queue;
 #endif
 
@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * through anymore ... */
(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-#if 1  /* XXX Should now be done by midlevel code, but it's broken XXX */
+   /* MSch 20140115 - looking at the generic NCR5380 driver, all of this
+* should go.
+* Catch-22: if we don't clear all queues, the SCSI driver lock will
+* not be reset by atari_scsi_reset()!
+*/
+
+#if defined(RESET_RUN_DONE)
+   /* XXX Should now be done by midlevel code, but it's broken XXX */
/* XXX see belowXXX */
 
/* MSch: old-style reset: actually abort all command processing here */
@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 * the midlevel code that the reset was SUCCESSFUL, and there is no
 * need to 'wake up' the commands by a request_sense
 */
-   return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #else /* 1 */
 
/* MSch: new-style reset handling: let the mid-level do what it can */
@@ -2963,6 +2970,6 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
local_irq_restore(flags);
 
/* we did no complete reset of all commands, so a wakeup is required */
-   return SCSI_RESET_WAKEUP | SCSI_RESET_BUS_RESET;
+   return SUCCESS;
 #endif /* 1 */
 }
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index cc1b013..5e19509 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -827,7 +827,7 @@ static int atari_scsi_bus_reset(Scsi_Cmnd *cmd)
} else

[PATCH 0/3] m68k/atari - Atari NCR5380 SCSI driver fixes (resent)

[Michael Schmitz]

Geert,

this patch series brings the Atari NCR5380 driver up to date with current 3.x
(or perhaps even 2.6) series SCSI midlevel and error handling changes.

The first patch fixes completion condition errors contained in Arnd Bergmann's
patch ([PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race). The locking
scheme still isn't 100% race free that way, but much improved, and I've seen
the driver successfully handle error recovery from aborting a disconnected
command via bus reset with all three patches applied (never managed to do that
since 2.4 days). 
Not sure whether this one should be taken through your tree or handled by Arnd
rather.

The other two are a rehash of old patches I had prepared somewhere along the
2.6 series - somehow or other these were lost when one of my old git trees
went pear shaped. 

Abort and reset handlers nowadays return SUCCESS or FAILED only, and the SCSI
midlevel now queues commands (in particular error handling ones) from soft
interrupt context - the Falcon locking scheme still is ill equipped to handle
that. Return codes have been changed and the reset handler in particular will
leave running command completion handlers to the midlevel now. 

If a SCSI command is queued while IDE or floppy have reserved the shared DMA
and interrupt, queue_command() may not wait for lock release and instead will
return SCSI_MLQUEUE_HOST_BUSY to indicate that. 

CC: to linux-scsi and the SCSI maintainer added as per your request. 

Cheers,

Michael
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] m68k/atari - atari_scsi: fix wait_event completion conditions

[Michael Schmitz]

Fix patch by ArndB changing falcon_get_lock to use wait_event.
Some of the completion conditions had been missed when converting
from while() {} to do {} until() logic.

Signed-off-by: Michael Schmitz schm...@debian.org
---
 drivers/scsi/atari_scsi.c |   14 +++---
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 1986ecb..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -550,10 +550,10 @@ static void falcon_get_lock(void)
 
local_irq_save(flags);
 
-   wait_event_cmd(falcon_fairness_wait,
-  !in_irq()  falcon_got_lock  stdma_others_waiting(),
-  local_irq_restore(flags),
-  local_irq_save(flags));
+   wait_event_cmd(falcon_fairness_wait,
+   in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+   local_irq_restore(flags),
+   local_irq_save(flags));
 
while (!falcon_got_lock) {
if (in_irq())
@@ -566,9 +566,9 @@ static void falcon_get_lock(void)
wake_up(falcon_try_wait);
} else {
wait_event_cmd(falcon_try_wait,
-  !falcon_got_lock  !falcon_trying_lock,
-  local_irq_restore(flags),
-  local_irq_save(flags));
+   falcon_got_lock  !falcon_trying_lock,
+   local_irq_restore(flags),
+   local_irq_save(flags));
}
}
 
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: esp_scsi QTAG in FAS216 (was Re: [PATCH 0/2] Experimental Amiga Zorro ESP driver)

[Michael Schmitz]

Tuomas,

I might still have a copy of the manual somewhere from way back, if you 
haven't found it anywhere. Would be on an old disk or even hardcopy in 
storage, so please confirm you still need it.


Cheers,

 Michael



Am 13.09.2013 um 03:36 schrieb Tuomas Vainikka:


On 09/11/2013 05:48 PM, Geert Uytterhoeven wrote:
On Wed, Sep 11, 2013 at 12:12 PM, Michael Schmitz 
schmitz...@gmail.com wrote:
problem. Using PIO, only the first byte of the tag message comes 
through. It
might not be esp_scsi's fault, but there seems to be an assumption 
that all
devices support TCQ. Also, no SCSI-2 features seem to be enabled in 
the chip

by esp_scsi.
I'd have to check with DaveM, but such an assumption might in fact 
exist.

BTW, it would be nice to start CCing linux-scsi@vger.kernel.org and
David S. Miller da...@davemloft.net on future discussions.

Gr{oetje,eeting}s,

 Geert



Hello,

Does anyone have the register descriptions for the FAS216 chip? It 
would seem that receiving only one byte during reconnect is perfectly 
normal [1] unless SCSI-2 features are explicitly enabled (which 
esp_scsi doesn't seem to be doing).


Also, looking at the timeout formulae in the old NCR53C9x.c driver, 
the values would be different for FAS216. Why was this dropped from 
the modern esp_scsi?


1. Page 2 in 
http://www.datasheetarchive.com/dl/Scans-030/ScansU9X12569.pdf


-Tuomas


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Remove of old NCR53C9x/esp family of drivers

[Michael Schmitz]

Hi all,

  Anybody who can look into converting the m68k NCR53C9x drivers and has
  hardware to test (some of) them? I don't think we can afford losing one
  third of our SCSI drivers...

I can look into converting some (having worked on the m68k Mac ESP driver
in the past - I do recall the Mac driver needs special hacks so it won't
be the easiest one to convert). I have no hardware to test these on,
however.

  You can use the following as guidance:
 
  commit 5ff263667798946abc15314eae3f341345877d7a
  Author: Thomas Bogendoerfer [EMAIL PROTECTED]
  Date:   Tue May 22 17:03:44 2007 -0700
 
  [SCSI] jazz_esp: Converted to use esp_core.
 
  Use new esp_scsi for JAZZ SCSI host adapter driver

Hasn't this come up before on linux-m68k? Someone asked me for information
when converting the Mac driver, I think. That could be a good start.

 I can also offer help to anyone who tries this.  It's also a good
 opportunity to let die drivers that have no committed users.

I'll contact you about this, then.

 Just to be clear on why we're doing this:  the NCR53C9x driver on which
 these are all based is in a pretty horrendous state of repair.  The
 esp_scsi one is much nicer, actually nicely tested and has a host of
 features the old driver didn't.  However, the principle driving force is
 the conversion of the SCSI subsystem to the sg_list accessors.  esp_scsi
 is already coverted.  NCR53C9x looks like a nasty job.  Thus, the moment
 the conversion patch goes in, all your drivers will break.  However,

Hmm, does that also affect another of the m68k drivers, the 5380 one? More
headache for me...

Michael


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html