Re: [PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers
Hi Finn,
On Fri, Nov 7, 2014 at 3:34 AM, Finn Thain wrote:
>> it's probably not Geert but James who needs to give the go-ahead.
>
> Given Geert's objections to the changes under arch/m68k in v1, I'm hoping
> for an acked-by from Geert for v2...
I'm happy with the arch/m68k changes in v2 (modulo the PAGE_SIZE
comment on Sun-3), so please add my
Acked-by: Geert Uytterhoeven
after fixing that one.
As I'm not afraid of merge conflicts, I think this can go in through the SCSI
tree? James?
Note that there's still room for improvement in the individual drivers, using
more modern infrastructure (e.g. using devm_*() calls, and getting rid of the
multiple ATARIHW_PRESENT() checks using e.g. platform_data or regmap).
But this is already a giant step forward.
Thanks a lot for doing this cleanup!
Removing ca. 3700 LoC deserves you a spot in the LoC removal Hall of Fame!
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
On Fri, 7 Nov 2014, Michael Schmitz wrote: > it's probably not Geert but James who needs to give the go-ahead. Given Geert's objections to the changes under arch/m68k in v1, I'm hoping for an acked-by from Geert for v2... -- -- 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
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
On Thu, 6 Nov 2014, Michael Schmitz wrote:
> 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.
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?
--
--
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
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 wrote:
> On Wed, Nov 5, 2014 at 8:56 AM, David Gálvez 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
On Wed, Nov 5, 2014 at 8:56 AM, David Gálvez 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
2014-11-05 0:36 GMT+01:00 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. > 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 -- 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
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
On Sun, 2 Nov 2014, Michael Schmitz wrote: > Tested atari_scsi on Falcon / CT60 hardware - no regressions over v1 > that I've seen. > > Tested-by: Michael Schmitz > Thanks for testing. "No regressions over v1" means "no regressions", right? -- -- 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
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 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
[PATCH v2 00/36] Fixes, cleanups and modernization for NCR5380 drivers
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). 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
