| Message ID | 20200331094054.24441-1-w@1wt.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | Floppy driver cleanups | expand |
Hi Willy, given that you are actively maintaining the floppy driver now, any chance I could trick you into proper highmem handling? I've been trying to phase out block layer bounce buffering, and any help from a competent maintainer to move their drivers to properly support highmem by kmapping for PIO/MMIO I/O would be very helpful.
Hi Christoph, On Tue, Mar 31, 2020 at 03:10:19AM -0700, Christoph Hellwig wrote: > Hi Willy, > > given that you are actively maintaining the floppy driver now, No no no I'm not! Denis is :-) Really, I mean I just proposed some help to clean up this mess after being tricked into not believing a bug report just because the code was too confusing. > any > chance I could trick you into proper highmem handling? I've been trying > to phase out block layer bounce buffering, and any help from a competent > maintainer to move their drivers to properly support highmem by kmapping > for PIO/MMIO I/O would be very helpful. I'm not sure what this implies regarding this code, to be honest. It's very tricky and implements sort of a state machine using function pointers within its interrupt handler so you never know exactly what accesses what, and quite a part of it remains obscure to me :-/ I can accept to help, I can even run tests since I still have running hardware, but I'd at least need some guidance. And probably Denis would know better than me there. Also I doubt we'd get sufficient testing on less common archs. While I do have sparc64/parisc/alpha available, I haven't booted a recent kernel on any of them for a while (2.4 used to be the last ones), and I'm not sure it's reasonable to go into such changes without proper testing. What do you think ? Willy
On Tue, Mar 31, 2020 at 01:01:36PM +0200, Willy Tarreau wrote: > I'm not sure what this implies regarding this code, to be honest. It's > very tricky and implements sort of a state machine using function pointers > within its interrupt handler so you never know exactly what accesses what, > and quite a part of it remains obscure to me :-/ I can accept to help, I > can even run tests since I still have running hardware, but I'd at least > need some guidance. And probably Denis would know better than me there. > Also I doubt we'd get sufficient testing on less common archs. While I > do have sparc64/parisc/alpha available, I haven't booted a recent kernel > on any of them for a while (2.4 used to be the last ones), and I'm not > sure it's reasonable to go into such changes without proper testing. The basic change is that instead of using bio_data() or page_address all pages coming from the block layer need to be properly kmap()ed. I'll try to cook something up an will send it to Denis and you for review and testing. The code things about sparc64/parisc/alpha is that they all don't have highmem, so these changes should be effective no-ops for them.
On Tue, Mar 31, 2020 at 08:28:12AM -0700, Christoph Hellwig wrote: > On Tue, Mar 31, 2020 at 01:01:36PM +0200, Willy Tarreau wrote: > > I'm not sure what this implies regarding this code, to be honest. It's > > very tricky and implements sort of a state machine using function pointers > > within its interrupt handler so you never know exactly what accesses what, > > and quite a part of it remains obscure to me :-/ I can accept to help, I > > can even run tests since I still have running hardware, but I'd at least > > need some guidance. And probably Denis would know better than me there. > > Also I doubt we'd get sufficient testing on less common archs. While I > > do have sparc64/parisc/alpha available, I haven't booted a recent kernel > > on any of them for a while (2.4 used to be the last ones), and I'm not > > sure it's reasonable to go into such changes without proper testing. > > The basic change is that instead of using bio_data() or page_address > all pages coming from the block layer need to be properly kmap()ed. > I'll try to cook something up an will send it to Denis and you for > review and testing. The code things about sparc64/parisc/alpha is > that they all don't have highmem, so these changes should be effective > no-ops for them. OK, thanks for the explanation! Willy
On 3/31/20 3:40 AM, Willy Tarreau wrote: > This series applies a second batch of cleanups to the floppy driver and > its multiple arch-specific parts. Here the focus was on getting rid of > hard-coded registers and flags values to switch to their symbolic > definitions instead, and on making use of the global current_fdc variable > much more explicit throughout the code to reduce the risk of accidental > misuse as was the case with the most recently fixed bug. > > Note that this code base is very old and the purpose is not to rewrite > nor reorganize the driver at all, but instead to make certain things > more obvious while keeping changes reviewable. It does not even address > style issues that make checkpatch continue to complain a little bit (15 > total warnings which were already there and don't seem worth addressing > without more careful testing). Some comments were added to document a > few non-obvious assumptions though. > > This series was rediffed against today's master (458ef2a25e0c) which > contains the first series. The changes were tested on x86 with real > hardware, and was build-tested on ARM. I'll be happy to queue these up for 5.8 when ready. Would be handy if you could resend a v2 patchset with the extra patches, makes my life so much easier...
Hi Jens, On Mon, Apr 13, 2020 at 04:46:41PM -0600, Jens Axboe wrote: > I'll be happy to queue these up for 5.8 when ready. Would be handy > if you could resend a v2 patchset with the extra patches, makes my > life so much easier... Sure, will do once Denis confirms he's done with the review and is OK with the series. Thanks! Willy
Hi, On 4/14/20 8:31 AM, Willy Tarreau wrote: > Hi Jens, > > On Mon, Apr 13, 2020 at 04:46:41PM -0600, Jens Axboe wrote: >> I'll be happy to queue these up for 5.8 when ready. Would be handy >> if you could resend a v2 patchset with the extra patches, makes my >> life so much easier... > > Sure, will do once Denis confirms he's done with the review and is > OK with the series. > > Thanks! > Willy > I can see no new issues, respecting that the initial version was sent privately and additional [24-27] fixups. [+] eye checked the changes [+] compile tested the patches on x86, arm, powerpc, sparc64, m68k (forced ARCH_MAY_HAVE_PC_FDC by removing BROKEN) sparc64 showed a couple of warnings in printks I was expecting that some of the arch maintainers will at least ack the patches. [+] tested on real hardware for x86 [+] local syzkaller fuzzing reveals no new issues Willy, could you please resend the patchset with printks fix for sparc64? Or if Jens don't mind and you don't want to send 30 patches again you can resend only sparc64 patch and I will reapply it and send everything to Jens with merge request. I applied your patches a couple of days ago here https://github.com/evdenis/linux-floppy/ to cleanups branch. I also faced minor ubsan warning in setup_rw_floppy that is not related to these patches. It's false alarm of cross-boundary access of cmd, reply_count, reply in floppy_raw_cmd. This access is intentional. I will send a patch on top of your patchset. Thanks, Denis
Hi Denis, On Tue, Apr 14, 2020 at 01:29:02PM +0300, Denis Efremov wrote: > I was expecting that some of the arch maintainers will at least > ack the patches. TBH, floppy is probably very low on any arch maintainer's priority list. > Willy, could you please resend the patchset with printks fix for sparc64? > Or if Jens don't mind and you don't want to send 30 patches again you can > resend only sparc64 patch and I will reapply it and send everything to Jens > with merge request. I applied your patches a couple of days ago here > https://github.com/evdenis/linux-floppy/ to cleanups branch. Then I'll redo this one only and directly send it to you as I really hate spamming innocents with patches. This will also help Jens in that you've already recomposed the whole series for him. > I also faced minor ubsan warning in setup_rw_floppy that is not related > to these patches. It's false alarm of cross-boundary access of cmd, > reply_count, reply in floppy_raw_cmd. This access is intentional. > I will send a patch on top of your patchset. OK, cool! Thanks! Willy
Hi, On 4/14/20 7:12 PM, Willy Tarreau wrote> > Then I'll redo this one only and directly send it to you as I really hate > spamming innocents with patches. This will also help Jens in that you've > already recomposed the whole series for him. > Applied https://github.com/evdenis/linux-floppy/tree/cleanups With your warnings fix for sparc64 here: https://github.com/evdenis/linux-floppy/commit/9c51b73efc71afd52c4eb00d93fc09d3c95010c8 I've sent small fix on top of your patches to linux-block. If everything is ok with my part I will send all patches to Jens for 5.8 in a week. Thanks, Denis
This series applies a second batch of cleanups to the floppy driver and its multiple arch-specific parts. Here the focus was on getting rid of hard-coded registers and flags values to switch to their symbolic definitions instead, and on making use of the global current_fdc variable much more explicit throughout the code to reduce the risk of accidental misuse as was the case with the most recently fixed bug. Note that this code base is very old and the purpose is not to rewrite nor reorganize the driver at all, but instead to make certain things more obvious while keeping changes reviewable. It does not even address style issues that make checkpatch continue to complain a little bit (15 total warnings which were already there and don't seem worth addressing without more careful testing). Some comments were added to document a few non-obvious assumptions though. This series was rediffed against today's master (458ef2a25e0c) which contains the first series. The changes were tested on x86 with real hardware, and was build-tested on ARM. Willy Tarreau (23): floppy: split the base port from the register in I/O accesses floppy: add references to 82077's extra registers floppy: use symbolic register names in the m68k port floppy: use symbolic register names in the parisc port floppy: use symbolic register names in the powerpc port floppy: use symbolic register names in the sparc32 port floppy: use symbolic register names in the sparc64 port floppy: use symbolic register names in the x86 port floppy: cleanup: make twaddle() not rely on current_{fdc,drive} anymore floppy: cleanup: make reset_fdc_info() not rely on current_fdc anymore floppy: cleanup: make show_floppy() not rely on current_fdc anymore floppy: cleanup: make wait_til_ready() not rely on current_fdc anymore floppy: cleanup: make output_byte() not rely on current_fdc anymore floppy: cleanup: make result() not rely on current_fdc anymore floppy: cleanup: make need_more_output() not rely on current_fdc anymore floppy: cleanup: make perpendicular_mode() not rely on current_fdc anymore floppy: cleanup: make fdc_configure() not rely on current_fdc anymore floppy: cleanup: make fdc_specify() not rely on current_{fdc,drive} anymore floppy: cleanup: make check_wp() not rely on current_{fdc,drive} anymore floppy: cleanup: make next_valid_format() not rely on current_drive anymore floppy: cleanup: make get_fdc_version() not rely on current_fdc anymore floppy: cleanup: do not iterate on current_fdc in DMA grab/release functions floppy: cleanup: add a few comments about expectations in certain functions arch/alpha/include/asm/floppy.h | 4 +- arch/arm/include/asm/floppy.h | 8 +- arch/m68k/include/asm/floppy.h | 27 +- arch/mips/include/asm/mach-generic/floppy.h | 8 +- arch/mips/include/asm/mach-jazz/floppy.h | 8 +- arch/parisc/include/asm/floppy.h | 19 +- arch/powerpc/include/asm/floppy.h | 19 +- arch/sparc/include/asm/floppy_32.h | 50 +-- arch/sparc/include/asm/floppy_64.h | 59 ++-- arch/x86/include/asm/floppy.h | 19 +- drivers/block/floppy.c | 330 ++++++++++---------- include/uapi/linux/fdreg.h | 16 +- 12 files changed, 299 insertions(+), 268 deletions(-) Cc: "David S. Miller" <davem@davemloft.net> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Helge Deller <deller@gmx.de> Cc: Ian Molton <spyro@f2s.com> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru> Cc: Matt Turner <mattst88@gmail.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Russell King <linux@armlinux.org.uk> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: x86@kernel.org