- 13 May, 2020 2 commits
-
-
Guoqing Jiang authored
Since the purpose of call flush_workqueue in new_dev_store is to ensure md_delayed_delete() has completed, so we should check rdev->del_work is pending or not. To suppress lockdep warning, we have to check mddev->del_work while md_delayed_delete is attached to rdev->del_work, so it is not aligned to the purpose of flush workquee. So a new workqueue is needed to avoid the awkward situation, and introduce a new func flush_rdev_wq to flush the new workqueue after check if there was pending work. Also like new_dev_store, ADD_NEW_DISK ioctl has the same purpose to flush workqueue while it holds bdev->bd_mutex, so make the same change applies to the ioctl to avoid similar lock issue. And md_delayed_delete actually wants to delete rdev, so rename the function to rdev_delayed_delete. Signed-off-by:
Guoqing Jiang <guoqing.jiang@cloud.ionos.com> Signed-off-by:
Song Liu <songliubraving@fb.com>
-
Guoqing Jiang authored
Coly reported possible circular locking dependencyi with LOCKDEP enabled, quote the below info from the detailed report [1]. [ 1607.673903] Chain exists of: [ 1607.673903] kn->count#256 --> (wq_completion)md_misc --> (work_completion)(&rdev->del_work) [ 1607.673903] [ 1607.827946] Possible unsafe locking scenario: [ 1607.827946] [ 1607.898780] CPU0 CPU1 [ 1607.952980] ---- ---- [ 1608.007173] lock((work_completion)(&rdev->del_work)); [ 1608.069690] lock((wq_completion)md_misc); [ 1608.149887] lock((work_completion)(&rdev->del_work)); [ 1608.242563] lock(kn->count#256); [ 1608.283238] [ 1608.283238] *** DEADLOCK *** [ 1608.283238] [ 1608.354078] 2 locks held by kworker/5:0/843: [ 1608.405152] #0: ffff8889eecc9948 ((wq_completion)md_misc){+.+.}, at: process_one_work+0x42b/0xb30 [ 1608.512399] #1: ffff888a1d3b7e10 ((work_completion)(&rdev->del_work)){+.+.}, at: process_one_work+0x42b/0xb30 [ 1608.632130] Since works (rdev->del_work and mddev->del_work) are queued in md_misc_wq, then lockdep_map lock is held if either of them are running, then both of them try to hold kernfs lock by call kobject_del. Then if new_dev_store or array_state_store are triggered by write to the related sysfs node, so the write operation gets kernfs lock, but need the lockdep_map because all of them would trigger flush_workqueue(md_misc_wq) finally, then the same lockdep_map lock is needed. To suppress the lockdep warnning, we should flush the workqueue in case the related work is pending. And several works are attached to md_misc_wq, so we need to check which work should be checked: 1. for __md_stop_writes, the purpose of call flush workqueue is ensure sync thread is started if it was starting, so check mddev->del_work is pending or not since md_start_sync is attached to mddev->del_work. 2. __md_stop flushes md_misc_wq to ensure event_work is done, check the event_work is enough. Assume raid_{ctr,dtr} -> md_stop -> __md_stop doesn't need the kernfs lock. 3. both new_dev_store (holds kernfs lock) and ADD_NEW_DISK ioctl (holds the bdev->bd_mutex) call flush_workqueue to ensure md_delayed_delete has completed, this case will be handled in next patch. 4. md_open flushes workqueue to ensure the previous md is disappeared, but it holds bdev->bd_mutex then try to flush workqueue, so it is better to check mddev->del_work as well to avoid potential lock issue, this will be done in another patch. [1]: https://marc.info/?l=linux-raid&m=158518958031584&w=2 Cc: Coly Li <colyli@suse.de> Reported-by:Coly Li <colyli@suse.de> Signed-off-by:
-
- 12 May, 2020 32 commits
-
-
https://github.com/evdenis/linux-floppyJens Axboe authored
Floppy patches for 5.8 Cleanups: - symbolic register names for x86,sparc64,sparc32,powerpc,parisc,m68k - split of local/global variables for drive,fdc - UBSAN warning suppress in setup_rw_floppy() Changes were compile tested on arm, sparc64, powerpc, m68k. Many patches introduce no binary changes by using defines instead of magic numbers. The patches were also tested with syzkaller and simple write/read/format tests on real hardware. Signed-off-by:
Denis Efremov <efremov@linux.com> Signed-off-by:
Jens Axboe <axboe@kernel.dk> * tag 'floppy-for-5.8' of https://github.com/evdenis/linux-floppy: (31 commits) floppy: suppress UBSAN warning in setup_rw_floppy() floppy: add defines for sizes of cmd & reply buffers of floppy_raw_cmd floppy: add FD_AUTODETECT_SIZE define for struct floppy_drive_params floppy: use print_hex_dump() in setup_DMA() floppy: cleanup: make set_fdc() always set current_drive and current_fd floppy: cleanup: get rid of current_reqD in favor of current_drive floppy: make sure to reset all FDCs upon resume() floppy: cleanup: do not iterate on current_fdc in do_floppy_init() floppy: cleanup: add a few comments about expectations in certain functions floppy: cleanup: do not iterate on current_fdc in DMA grab/release functions floppy: cleanup: make get_fdc_version() not rely on current_fdc anymore floppy: cleanup: make next_valid_format() not rely on current_drive anymore floppy: cleanup: make check_wp() not rely on current_{fdc,drive} anymore floppy: cleanup: make fdc_specify() not rely on current_{fdc,drive} anymore floppy: cleanup: make fdc_configure() not rely on current_fdc anymore floppy: cleanup: make perpendicular_mode() not rely on current_fdc anymore floppy: cleanup: make need_more_output() not rely on current_fdc anymore floppy: cleanup: make result() not rely on current_fdc anymore floppy: cleanup: make output_byte() not rely on current_fdc anymore floppy: cleanup: make wait_til_ready() not rely on current_fdc anymore ...
-
Denis Efremov authored
UBSAN: array-index-out-of-bounds in drivers/block/floppy.c:1521:45 index 16 is out of range for type 'unsigned char [16]' Call Trace: ... setup_rw_floppy+0x5c3/0x7f0 floppy_ready+0x2be/0x13b0 process_one_work+0x2c1/0x5d0 worker_thread+0x56/0x5e0 kthread+0x122/0x170 ret_from_fork+0x35/0x40 From include/uapi/linux/fd.h: struct floppy_raw_cmd { ... unsigned char cmd_count; unsigned char cmd[16]; unsigned char reply_count; unsigned char reply[16]; ... } This out-of-bounds access is intentional. The command in struct floppy_raw_cmd may take up the space initially intended for the reply and the reply count. It is needed for long 82078 commands such as RESTORE, which takes 17 command bytes. Initial cmd size is not enough and since struct setup_rw_floppy is a part of uapi we check that cmd_count is in [0:16+1+16] in raw_cmd_copyin(). The patch adds union with original cmd,reply_count,reply fields and fullcmd field of equivalent size. The cmd accesses are turned to fullcmd where appropriate to suppress UBSAN warning. Link: https://lore.kernel.org/r/20200501134416.72248-5-efremov@linux.comReviewed-by:Christoph Hellwig <hch@lst.de> Signed-off-by:
-
Denis Efremov authored
Use FD_RAW_CMD_SIZE, FD_RAW_REPLY_SIZE defines instead of magic numbers for cmd & reply buffers of struct floppy_raw_cmd. Remove local to floppy.c MAX_REPLIES define, as it is now FD_RAW_REPLY_SIZE. FD_RAW_CMD_FULLSIZE added as we allow command to also fill reply_count and reply fields. Link: https://lore.kernel.org/r/20200501134416.72248-4-efremov@linux.comReviewed-by:
-
Denis Efremov authored
Use FD_AUTODETECT_SIZE for autodetect buffer size in struct floppy_drive_params instead of a magic number. Link: https://lore.kernel.org/r/20200501134416.72248-3-efremov@linux.comReviewed-by:
-
Denis Efremov authored
Remove pr_cont() and use print_hex_dump() in setup_DMA() to print the contents of the cmd buffer. Link: https://lore.kernel.org/r/20200501134416.72248-2-efremov@linux.comSuggested-by:
Joe Perches <joe@perches.com> Reviewed-by:
-
Willy Tarreau authored
When called with a negative drive value, set_fdc() would stick to the current fdc (which was assumed to reflect the current_drive's FDC). We do not need this anymore as the last call place with a negative value was just addressed. Let's make this function always set both current_fdc and current_drive so that there's no more ambiguity. A few comments stating this were added to a few non-obvious places. Link: https://lore.kernel.org/r/20200410101904.14652-3-w@1wt.euSigned-off-by:
Willy Tarreau <w@1wt.eu> Signed-off-by:
-
Willy Tarreau authored
This macro equals -1 and is used as an alternative for current_drive when calling reschedule_timeout(), which in turn needs to remap it. This only adds obfuscation, let's simply use current_drive. Link: https://lore.kernel.org/r/20200410101904.14652-2-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
In floppy_resume() we don't properly reinitialize all FDCs, instead we reinitialize the current FDC once per available FDC because value -1 is passed to user_reset_fdc(). Let's simply save the current drive and properly reinitialize each FDC. Link: https://lore.kernel.org/r/20200410101904.14652-1-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
There's no need to iterate on current_fdc in do_floppy_init() anymore, in the first case it's only used as an array index to access fdc_state[], so let's get rid of this confusing assignment. The second case is a bit trickier because user_reset_fdc() needs to already know current_fdc when called with drive==-1 due to this call chain: user_reset_fdc() lock_fdc() set_fdc() drive<0 ==> new_fdc = current_fdc Note that current_drive is not used in this code part and may even not match a unit belonging to current_fdc. Instead of passing -1 we can simply pass the first drive of the FDC being initialized, which is even cleaner as it will allow the function chain above to consistently assign both variables. Link: https://lore.kernel.org/r/20200410093023.14499-1-w@1wt.euSigned-off-by: -
Willy Tarreau authored
The locking in the driver is far from being obvious, with unlocking automatically happening at end of operations scheduled by interrupt, especially for the error paths where one does not necessarily expect that such an interrupt may be triggered. Let's add a few comments about what to expect at certain places to avoid misdetecting bugs which are not. Link: https://lore.kernel.org/r/20200331094054.24441-24-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Both floppy_grab_irq_and_dma() and floppy_release_irq_and_dma() used to iterate on the global variable while setting up or freeing resources. Now that they exclusively rely on functions which take the fdc as an argument, so let's not touch the global one anymore. Link: https://lore.kernel.org/r/20200331094054.24441-23-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-22-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the drive is passed in argument so that the function does not use current_drive anymore. Link: https://lore.kernel.org/r/20200331094054.24441-21-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc and drive are passed in argument so that the function does not use current_fdc nor current_drive anymore. Link: https://lore.kernel.org/r/20200331094054.24441-20-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc and drive are passed in argument so that the function does not use current_fdc nor current_drive anymore. Link: https://lore.kernel.org/r/20200331094054.24441-19-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-18-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. It's worth noting that there's still a single raw_cmd pointer specific to the current fdc. It may make sense to have one per fdc in the future. In addition, cont->done() still relies on the current drive and current raw_cmd. Link: https://lore.kernel.org/r/20200331094054.24441-17-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-16-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. It's worth noting that there's still a single reply_buffer[] which will store the result for the current fdc. It may or may not make sense to implement one buffer per fdc in the future. Link: https://lore.kernel.org/r/20200331094054.24441-15-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-14-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-13-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-12-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc is passed in argument so that the function does not use current_fdc anymore. Link: https://lore.kernel.org/r/20200331094054.24441-11-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now the fdc and drive are passed in argument so that the function does not use current_fdc nor current_drive anymore. Link: https://lore.kernel.org/r/20200331094054.24441-10-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Now we can use FD_STATUS and FD_DATA instead of 4 or 5, let's do this, and also use STATUS_DMA and STATUS_READY for the status bits. Link: https://lore.kernel.org/r/20200331094054.24441-9-w@1wt.eu Cc: x86@kernel.org Signed-off-by:
-
Willy Tarreau authored
Now by splitting the base address from the register index we can use the symbolic register names instead of the hard-coded numeric values. Link: https://lore.kernel.org/r/20200331094054.24441-8-w@1wt.eu Cc: "David S. Miller" <davem@davemloft.net> [willy: fix printk warnings s/%lx/%x/g in sun_82077_fd_{inb,outb}()] Signed-off-by:
-
Willy Tarreau authored
The sparc port used to be forced to rely on numeric register indexes with their equivalent in comments. Now that they don't depend on the IO port we can use their symbolic names. Link: https://lore.kernel.org/r/20200331094054.24441-7-w@1wt.eu Cc: "David S. Miller" <davem@davemloft.net> Signed-off-by:
-
Willy Tarreau authored
Now we can use FD_STATUS and FD_DATA instead of 4 or 5, let's do this, and also use STATUS_DMA and STATUS_READY for the status bits. Link: https://lore.kernel.org/r/20200331094054.24441-6-w@1wt.eu Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Signed-off-by:
-
Willy Tarreau authored
Now we can use FD_STATUS and FD_DATA instead of 4 or 5, let's do this, and also use STATUS_DMA and STATUS_READY for the status bits. Link: https://lore.kernel.org/r/20200331094054.24441-5-w@1wt.eu Cc: Helge Deller <deller@gmx.de> Signed-off-by:
-
Willy Tarreau authored
Now we can use FD_STATUS and FD_DATA instead of 4 or 5, let's do this, and also use STATUS_DMA and STATUS_READY for the status bits. Link: https://lore.kernel.org/r/20200331094054.24441-4-w@1wt.eu Cc: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by:
-
Willy Tarreau authored
This controller provides extra status registers SRA and SRB as well as a tape drive register (TDR) and a data rate select register (DSR), which are referenced in the sparc port, so let's have their symbolic definitions centralized. Link: https://lore.kernel.org/r/20200331094054.24441-3-w@1wt.euSigned-off-by:
-
Willy Tarreau authored
Currently we have architecture-specific fd_inb() and fd_outb() functions or macros, taking just a port which is in fact made of a base address and a register. The base address is FDC-specific and derived from the local or global "fdc" variable through the FD_IOPORT macro used in the base address calculation. This change splits this by explicitly passing the FDC's base address and the register separately to fd_outb() and fd_inb(). It affects the following archs: - x86, alpha, mips, powerpc, parisc, arm, m68k: simple remap of port -> base+reg - sparc32: use of reg only, since the base address was already masked out and the FDC controller is known from a static struct. - sparc64: like x86 for PCI, like sparc32 for 82077 Some archs use inline functions and others macros. This was not unified in order to minimize the number of changes to review. For the same reason checkpatch still spews a few warnings about things that were already there before. The parisc still uses hard-coded register values and could be cleaned up by taking the register definitions. The sparc per-controller inb/outb functions could further be refined to explicitly take an FDC register instead of a port in argument but it was not needed yet and may be cleaned later. Link: https://lore.kernel.org/r/20200331094054.24441-2-w@1wt.eu Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru> Cc: Richard Henderson <rth@twiddle.net> Cc: Matt Turner <mattst88@gmail.com> Cc: Ian Molton <spyro@f2s.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Helge Deller <deller@gmx.de> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: x86@kernel.org Signed-off-by:
-
- 09 May, 2020 6 commits
-
-
Keith Busch authored
Improve code readability by defining the specification's constants that the driver is using when decoding identification payloads. Signed-off-by:
Keith Busch <kbusch@kernel.org> Reviewed-by:
Bart van Assche <bvanassche@acm.org> Reviewed-by:
Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> Acked-by:
Sagi Grimberg <sagi@grimberg.me> Signed-off-by:
-
Chaitanya Kulkarni authored
With reference to the NVMeOF Specification (page 44, Figure 38) discovery log page entry provides address family field. We do set the transport type field but the adrfam field is not set when using loop transport and also it doesn't have support in the nvme-cli. So when reading discovery log page with a loop transport it leads to confusing output. As per the spec for adrfam value 254 is reserved for Intra Host Transport i.e. loopback), we add a required macro in the protocol header file, set default port disc addr entry's adrfam to NVMF_ADDR_FAMILY_MAX, and update nvmet_addr_family configfs array for show/store attribute. Without this patch, setting adrfam to (ipv4/ipv6/ib/fc/loop/" ") we get following output for nvme discover command from nvme-cli which is confusing. trtype: loop adrfam: ipv4 trtype: loop adrfam: ipv6 trtype: loop adrfam: infiniband trtype: loop adrfam: fibre-channel trtype: loop # ${CFGFS_HOME}/nvmet/ports/1/addr_adrfam = loop adrfam: pci # <----- pci for loop trtype: loop # ${CFGFS_HOME}/nvmet/ports/1/addr_adrfam = " " adrfam: pci # <----- pci for unrecognized This patch fixes above output :- trtype: loop adrfam: ipv4 trtype: loop adrfam: ipv6 trtype: loop adrfam: infiniband trtype: loop adrfam: fibre-channel trtype: loop # ${CFGFS_HOME}/nvmet/ports/1/addr_adrfam = loop adrfam: loop # <----- loop for loop trtype: loop # ${CFGFS_HOME}/config/nvmet/ports/adrfam = " " adrfam: unrecognized # <----- unrecognized when invalid value Signed-off-by: -
Chaitanya Kulkarni authored
The configfs attributes which are supposed to set when port is disable such as addr[addrfam|portid|traddr|treq|trsvcid|inline_data_size|trtype] has repetitive check and generic error message printing. This patch creates centralize helper to check and print an error message that also accepts caller as a parameter. This makes error message easy to parse for the user, removes the duplicate code and makes it available for futures such scenarios. Signed-off-by:
-
Chaitanya Kulkarni authored
Currently nvmet_addr_treq_[store|show]() uses switch and if else ladder for address transport requirements to string and reverse mapping. With addtion of the generic nvmet_type_name_map structure we can get rid of the switch and if else ladder with string duplication. Signed-off-by:
-
Chaitanya Kulkarni authored
Now that we have a generic type to name map for configfs, get rid of the nvmet_ana_state_names structure and replace it with newly added nvmet_type_name_map. Signed-off-by:
-
Chaitanya Kulkarni authored
Right now nvmet_addr_adrfam_[store|show]() uses switch and if else ladder for address family to string and reverse mapping which also repeats the strings in show and store function. With addition of generic nvmet_type_name_map structure we can now get rid of the switch and if else ladder and string duplication. Also, we add a newline in before found label in nvmet_addr_trtype_store() which keeps goto label code consistent with nvmet_allowed_hosts_drop_link(), nvmet_port_subsys_drop_link() and nvmet_ana_group_ana_state_store(). Signed-off-by:
-
