* Re: [PATCH v4] mmc: sdio: check the buffer address for sdio API
From: Jens Axboe @ 2017-02-14 16:18 UTC (permalink / raw)
To: Ulf Hansson, Shawn Lin
Cc: linux-mmc@vger.kernel.org, Russell King, Christoph Hellwig,
linux-block
In-Reply-To: <CAPDyKFqS+OgmweTySyer422TfkWZrXMrQeuiymQ0HjqiZAdJzg@mail.gmail.com>
On 02/14/2017 02:17 AM, Ulf Hansson wrote:
> +Russell, Jens, Christoph, linux-block (asking for help in review)
>
> On 7 February 2017 at 01:54, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> It's fine if the host driver use PIO mode to transfer the
>> vmalloc area buffer but not for DMA mode. The sdio APIs haven't
>> provide the capability to tell the caller whether it will use DMA
>> to finish the IO transfer or not, so don't give the randomly
>> insmoded sdio function driver the possibility to break the kernel.
>> Also the APIs shouldn't take the liberty to do a copy for these
>> cases and just kick out these requests should be enough.
>>
>> This issue is observed by doing insmod a downloaded wifi module
>> driver and the kernel panic right away. Unfortunately we don't have
>> the source code but adding this patch that it proves that the module
>> driver was passing on a vmalloc area buffer for sdio APIs.
>>
>> It could very easy to be reproduced by writing a simple function
>> module driver and insmod it, and panic log looks like:
>>
>> unsigned u8 __aligned(32) buf[PAGE_SIZE];
>>
>> static int wifi_probe(struct sdio_func *func, const struct
>> sdio_device_id *id)
>> {
>> sdio_claim_host(func);
>> sdio_enable_func(func);
>> sdio_memcpy_toio(func, 0x0, buf, 0x200);
>> }
>>
>> [ 236.748210] wifi_probe: buf = 0xffffff8000a40b80
>> [ 236.748258] swiotlb_tbl_map_single: orig_addr = 0xfffffffff8c40b80,
>> tlb_addr = 0xf7eae000 //I added log here
>> [ 236.748276] Unable to handle kernel paging request at virtual address
>> fffffffff8c40b80
>> [ 236.776486] pgd = ffffffc0b3417000
>> [ 236.776789] [fffffffff8c40b80] *pgd=00000000b3427003,
>> *pud=00000000b3427003, *pmd=0000000000000000
>> [ 236.777601] Internal error: Oops: 96000005 [#1] PREEMPT SMP
>> [ 236.778093] Modules linked in: drvtst(O+)
>> [ 236.778463] CPU: 0 PID: 1918 Comm: insmod Tainted: G O
>> 4.4.36 #25
>> [ 236.779096] Hardware name: Rockchip RK3399 Evaluation Board v3 edp
>> (Android) (DT)
>> [ 236.779753] task: ffffffc0e3db0c40 ti: ffffffc0b342c000 task.ti:
>> ffffffc0b342c000
>> [ 236.780418] PC is at __memcpy+0x100/0x180
>> [ 236.780777] LR is at swiotlb_tbl_map_single+0x254/0x274
>> [ 236.781237] pc : [<ffffff80083579c0>] lr : [<ffffff800837cf70>]
>> ...
>>
>> [ 236.941392] f460: 3d20726464615f67 6666666666783020
>> [ 236.941826] [<ffffff80083579c0>] __memcpy+0x100/0x180
>> [ 236.942274] [<ffffff800837dc18>] swiotlb_map_sg_attrs+0xa8/0x170
>> [ 236.942810] [<ffffff800809359c>] __swiotlb_map_sg_attrs+0x24/0x8c
>> [ 236.943353] [<ffffff800871113c>]
>> dw_mci_pre_dma_transfer.isra.16+0xf0/0x11c
>> [ 236.943967] [<ffffff80087129cc>] __dw_mci_start_request+0x17c/0x4d0
>> [ 236.944520] [<ffffff80087132e0>] dw_mci_request+0xb8/0xf0
>> [ 236.945002] [<ffffff80086f8b44>] __mmc_start_request+0x9c/0xc0
>> [ 236.945520] [<ffffff80087ae0e8>]
>> mmc_start_request.part.17+0x100/0x11c
>> [ 236.946097] [<ffffff80086f9c74>] mmc_wait_for_req+0x78/0x1a8
>> [ 236.946600] [<ffffff800870421c>] mmc_io_rw_extended+0x27c/0x2fc
>> [ 236.947124] [<ffffff8008705800>] sdio_io_rw_ext_helper+0x1e4/0x238
>> [ 236.947670] [<ffffff8008705954>] sdio_memcpy_toio+0x24/0x2c
>> [ 236.948169] [<ffffff8000a40108>] wifi_probe+0xa8/0x198 [drvtst]
>> [ 236.948708] [<ffffff8008704734>] sdio_bus_probe+0xb0/0x140
>> [ 236.949195] [<ffffff80084bfd6c>] driver_probe_device+0x118/0x2b0
>> [ 236.949724] [<ffffff80084bff68>] __driver_attach+0x64/0x90
>> [ 236.950212] [<ffffff80084bee18>] bus_for_each_dev+0x80/0xb0
>> [ 236.950706] [<ffffff80084bf8ac>] driver_attach+0x20/0x28
>> [ 236.951176] [<ffffff80084bf45c>] bus_add_driver+0xe8/0x1ec
>> [ 236.951661] [<ffffff80084c0afc>] driver_register+0x98/0xe4
>> [ 236.952147] [<ffffff8008704580>] sdio_register_driver+0x24/0x2c
>> [ 236.952675] [<ffffff8000a40228>] wifi_sdio_init+0x30/0x68 [drvtst]
>> [ 236.953241] [<ffffff8000a4400c>] wifi_drv_init+0xc/0x38 [drvtst]
>> [ 236.953789] [<ffffff8008082ba4>] do_one_initcall+0x17c/0x198
>> [ 236.954293] [<ffffff800815e564>] do_init_module+0x60/0x1b8
>> [ 236.954779] [<ffffff80081150f0>] load_module+0x1660/0x1a50
>> [ 236.955264] [<ffffff800811562c>] SyS_init_module+0x14c/0x180
>> [ 236.955765] [<ffffff80080826f0>] el0_svc_naked+0x24/0x28
>>
>> So the kernel crash since the vmalloc area buffer, on-stack buffer or
>> the on-module buffer aren't physical continuous and swiotlb couldn't find
>> the correct mapping address in fact. Anyway, don't give the chance to panic
>> kernel by using sdio APIs.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v4:
>> - fix build warning by gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>>
>> Changes in v3:
>> - fix build issue reported by Kbuild Robot
>>
>> Changes in v2:
>> - add new function to check the addr of vmalloc/module/stack
>>
>> drivers/mmc/core/sdio_io.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
>> index 406e5f0..4df7c6f 100644
>> --- a/drivers/mmc/core/sdio_io.c
>> +++ b/drivers/mmc/core/sdio_io.c
>> @@ -10,6 +10,7 @@
>> */
>>
>> #include <linux/export.h>
>> +#include <linux/mm.h>
>> #include <linux/mmc/host.h>
>> #include <linux/mmc/card.h>
>> #include <linux/mmc/sdio.h>
>> @@ -298,6 +299,19 @@ unsigned int sdio_align_size(struct sdio_func *func, unsigned int sz)
>> }
>> EXPORT_SYMBOL_GPL(sdio_align_size);
>>
>> +static int sdio_io_addr_sanity_check(void *buf)
>> +{
>> +#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
>> + unsigned long addr = (unsigned long)buf;
>> +
>> + if ((addr >= MODULES_VADDR && addr < MODULES_END) ||
>> + object_is_on_stack(buf))
>> + return 1;
>> +#endif
>> + return (is_vmalloc_addr(buf) || object_is_on_stack(buf));
>> +
>> +}
>> +
>> /* Split an arbitrarily sized data transfer into several
>> * IO_RW_EXTENDED commands. */
>> static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>> @@ -307,7 +321,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>> unsigned max_blocks;
>> int ret;
>>
>> - if (!func || (func->num > 7))
>> + if (!func || (func->num > 7) || sdio_io_addr_sanity_check(buf))
>> return -EINVAL;
>>
>> /* Do the bulk of the transfer using block mode (if supported). */
>> --
>> 1.9.1
>>
>>
>
> I somewhat understand the issue you are trying to solve when
> validating the buffer for DMA. However, I don't know what a proper fix
> should be. Especially since I have seen nothing similar in the kernel
> so far.
>
> Therefore I have looped in Russell King, Jens Axboe, Christoph Hellwig
> and linux-block to ask for help in reviewing this. Hopefully we can
> get some advise here.
>
> To give some more background to the folkz above, the SDIO func API
> (part of the MMC subsystem) is intended to be used by for example WLAN
> drivers, which may be built as kernel modules. Part of SDIO func API
> is about writing/reading buffers to/from an SDIO card. That may
> involve doing DMA transactions, depending on what the corresponding
> MMC/SDIO host controller/driver supports.
The current situation seems like a bit of a mess. Why don't you have two
entry points, one for DMA and one for PIO. If the caller doesn't know if
he can use DMA, he'd better call the PIO variant. Either that, or audit
all callers and ensure they do the right thing wrt having a dma capable
buffer.
A check for can-do-dma should be restricted to size/alignment
constraints based on the hardware, not try and catch all weird cases of
whether or not that buffer is on the stack, heap, etc.
--
Jens Axboe
^ permalink raw reply
* Re: Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: hch @ 2017-02-14 16:34 UTC (permalink / raw)
To: Dexuan Cui
Cc: hch@lst.de, Jens Axboe, Bart Van Assche, hare@suse.com,
hare@suse.de, Martin K. Petersen, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, jth@kernel.org, Nick Meier,
Alex Ng (LIS), Long Li, Adrian Suhov (Cloudbase Solutions SRL),
Chris Valean (Cloudbase Solutions SRL)
In-Reply-To: <MWHPR03MB266957B2E9075ACDB829FD21BF580@MWHPR03MB2669.namprd03.prod.outlook.com>
> I tested today's linux-next (next-20170214) + the 2 patches just now and got
> a weird result:
> sometimes the VM stills hung with a new calltrace (BUG: spinlock bad
> magic) , but sometimes the VM did boot up despite the new calltrace!
>
> Attached is the log of a "good" boot.
>
> It looks we have a memory corruption issue somewhere...
Yes.
> Actually previously I saw the "BUG: spinlock bad magic" message once, but I
> couldn't repro it later, so I didn't mention it to you.
Interesting.
>
> The good news is that now I can repro the "spinlock bad magic" message
> every time.
> I tried to dig into this by enabling Kernel hacking -> Memory debugging,
> but didn't find anything abnormal.
> Is it possible that the SCSI layer passes a wrong memory address?
It's possible, but this looks like it might be a different issue.
A few questions on the dmesg:
[ 6.208794] sd 2:0:0:0: [storvsc] Sense Key : Illegal Request [current]
[ 6.209447] sd 2:0:0:0: [storvsc] Add. Sense: Invalid command operation code
[ 6.210043] sd 3:0:0:0: [storvsc] Sense Key : Illegal Request [current]
[ 6.210618] sd 3:0:0:0: [storvsc] Add. Sense: Invalid command operation code
[ 6.212272] sd 2:0:0:0: [storvsc] Sense Key : Illegal Request [current]
[ 6.212897] sd 2:0:0:0: [storvsc] Add. Sense: Invalid command operation code
[ 6.213474] sd 3:0:0:0: [storvsc] Sense Key : Illegal Request [current]
[ 6.214051] sd 3:0:0:0: [storvsc] Add. Sense: Invalid command operation code
I didn't see anything like this in the other logs. Are these messages
something usual on HyperV VMs?
[ 6.358405] XFS (sdb1): Mounting V5 Filesystem
[ 6.404478] XFS (sdb1): Ending clean mount
[ 7.535174] BUG: spinlock bad magic on CPU#0, swapper/0/0
[ 7.536807] lock: host_ts+0x30/0xffffffffffffe1a0 [hv_utils], .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
[ 7.538436] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.10.0-rc8-next-20170214+ #1
[ 7.539142] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090006 04/28/2016
[ 7.539142] Call Trace:
[ 7.539142] <IRQ>
[ 7.539142] dump_stack+0x63/0x82
[ 7.539142] spin_dump+0x78/0xc0
[ 7.539142] do_raw_spin_lock+0xfd/0x160
[ 7.539142] _raw_spin_lock_irqsave+0x4c/0x60
[ 7.539142] ? timesync_onchannelcallback+0x153/0x220 [hv_utils]
[ 7.539142] timesync_onchannelcallback+0x153/0x220 [hv_utils]
Can you resolve this address using gdb to a line of code? Once inside
gdb do:
l *(timesync_onchannelcallback+0x153)
^ permalink raw reply
* Re: [dm-devel] v4.9, 4.4-final: 28 bioset threads on small notebook, 36 threads on cellphone
From: Mikulas Patocka @ 2017-02-14 16:34 UTC (permalink / raw)
To: Kent Overstreet
Cc: Mike Snitzer, oleg.drokin, ming.l, andreas.dilger,
martin.petersen, minchan, jkosina, ming.lei, kernel list, jim,
pjk1939, axboe, geoff, dm-devel, linux-block, dpark, Pavel Machek,
ngupta, hch, agk
In-Reply-To: <20170209212523.lgrgvk2squoo6x6f@moria.home.lan>
On Thu, 9 Feb 2017, Kent Overstreet wrote:
> On Wed, Feb 08, 2017 at 11:34:07AM -0500, Mike Snitzer wrote:
> > On Tue, Feb 07 2017 at 11:58pm -0500,
> > Kent Overstreet <kent.overstreet@gmail.com> wrote:
> >
> > > On Tue, Feb 07, 2017 at 09:39:11PM +0100, Pavel Machek wrote:
> > > > On Mon 2017-02-06 17:49:06, Kent Overstreet wrote:
> > > > > On Mon, Feb 06, 2017 at 04:47:24PM -0900, Kent Overstreet wrote:
> > > > > > On Mon, Feb 06, 2017 at 01:53:09PM +0100, Pavel Machek wrote:
> > > > > > > Still there on v4.9, 36 threads on nokia n900 cellphone.
> > > > > > >
> > > > > > > So.. what needs to be done there?
> > > > >
> > > > > > But, I just got an idea for how to handle this that might be halfway sane, maybe
> > > > > > I'll try and come up with a patch...
> > > > >
> > > > > Ok, here's such a patch, only lightly tested:
> > > >
> > > > I guess it would be nice for me to test it... but what it is against?
> > > > I tried after v4.10-rc5 and linux-next, but got rejects in both cases.
> > >
> > > Sorry, I forgot I had a few other patches in my branch that touch
> > > mempool/biosets code.
> > >
> > > Also, after thinking about it more and looking at the relevant code, I'm pretty
> > > sure we don't need rescuer threads for block devices that just split bios - i.e.
> > > most of them, so I changed my patch to do that.
> > >
> > > Tested it by ripping out the current->bio_list checks/workarounds from the
> > > bcache code, appears to work:
> >
> > Feedback on this patch below, but first:
> >
> > There are deeper issues with the current->bio_list and rescue workqueues
> > than thread counts.
> >
> > I cannot help but feel like you (and Jens) are repeatedly ignoring the
> > issue that has been raised numerous times, most recently:
> > https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html
> >
> > FYI, this test (albeit ugly) can be used to check if the dm-snapshot
> > deadlock is fixed:
> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
> >
> > This situation is the unfortunate pathological worst case for what
> > happens when changes are merged and nobody wants to own fixing the
> > unforseen implications/regressions. Like everyone else in a position
> > of Linux maintenance I've tried to stay away from owning the
> > responsibility of a fix -- it isn't working. Ok, I'll stop bitching
> > now.. I do bear responsibility for not digging in myself. We're all
> > busy and this issue is "hard".
>
> Mike, it's not my job to debug DM code for you or sift through your bug reports.
> I don't read dm-devel, and I don't know why you think I that's my job.
>
> If there's something you think the block layer should be doing differently, post
> patches - or at the very least, explain what you'd like to be done, with words.
> Don't get pissy because I'm not sifting through your bug reports.
So I post this patch for that bug.
Will any of the block device maintainers respond to it?
From: Mikulas Patocka <mpatocka@redhat.com>
Date: Tue, 27 May 2014 11:03:36 -0400
Subject: block: flush queued bios when process blocks to avoid deadlock
The block layer uses per-process bio list to avoid recursion in
generic_make_request. When generic_make_request is called recursively,
the bio is added to current->bio_list and generic_make_request returns
immediately. The top-level instance of generic_make_request takes bios
from current->bio_list and processes them.
The problem is that this bio queuing on current->bio_list creates an
artifical locking dependency - a bio further on current->bio_list depends
on any locks that preceding bios could take. This could result in a
deadlock.
Commit df2cb6daa4 ("block: Avoid deadlocks with bio allocation by
stacking drivers") created a workqueue for every bio set and code
in bio_alloc_bioset() that tries to resolve some low-memory deadlocks by
redirecting bios queued on current->bio_list to the workqueue if the
system is low on memory. However another deadlock (see below **) may
happen, without any low memory condition, because generic_make_request
is queuing bios to current->bio_list (rather than submitting them).
Fix this deadlock by redirecting any bios on current->bio_list to the
bio_set's rescue workqueue on every schedule call. Consequently, when
the process blocks on a mutex, the bios queued on current->bio_list are
dispatched to independent workqueus and they can complete without
waiting for the mutex to be available.
Also, now we can remove punt_bios_to_rescuer() and bio_alloc_bioset()'s
calls to it because bio_alloc_bioset() will implicitly punt all bios on
current->bio_list if it performs a blocking allocation.
** Here is the dm-snapshot deadlock that was observed:
1) Process A sends one-page read bio to the dm-snapshot target. The bio
spans snapshot chunk boundary and so it is split to two bios by device
mapper.
2) Device mapper creates the first sub-bio and sends it to the snapshot
driver.
3) The function snapshot_map calls track_chunk (that allocates a structure
dm_snap_tracked_chunk and adds it to tracked_chunk_hash) and then remaps
the bio to the underlying device and exits with DM_MAPIO_REMAPPED.
4) The remapped bio is submitted with generic_make_request, but it isn't
issued - it is added to current->bio_list instead.
5) Meanwhile, process B (dm's kcopyd) executes pending_complete for the
chunk affected be the first remapped bio, it takes down_write(&s->lock)
and then loops in __check_for_conflicting_io, waiting for
dm_snap_tracked_chunk created in step 3) to be released.
6) Process A continues, it creates a second sub-bio for the rest of the
original bio.
7) snapshot_map is called for this new bio, it waits on
down_write(&s->lock) that is held by Process B (in step 5).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1267650
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Depends-on: df2cb6daa4 ("block: Avoid deadlocks with bio allocation by stacking drivers")
Cc: stable@vger.kernel.org
---
block/bio.c | 77 +++++++++++++++++++------------------------------
include/linux/blkdev.h | 24 ++++++++++-----
kernel/sched/core.c | 7 +---
3 files changed, 50 insertions(+), 58 deletions(-)
Index: linux-4.10-rc2/block/bio.c
===================================================================
--- linux-4.10-rc2.orig/block/bio.c
+++ linux-4.10-rc2/block/bio.c
@@ -357,35 +357,37 @@ static void bio_alloc_rescue(struct work
}
}
-static void punt_bios_to_rescuer(struct bio_set *bs)
+/**
+ * blk_flush_bio_list
+ * @tsk: task_struct whose bio_list must be flushed
+ *
+ * Pop bios queued on @tsk->bio_list and submit each of them to
+ * their rescue workqueue.
+ *
+ * If the bio doesn't have a bio_set, we leave it on @tsk->bio_list.
+ * If the bio is allocated from fs_bio_set, we must leave it to avoid
+ * deadlock on loopback block device.
+ * Stacking bio drivers should use bio_set, so this shouldn't be
+ * an issue.
+ */
+void blk_flush_bio_list(struct task_struct *tsk)
{
- struct bio_list punt, nopunt;
struct bio *bio;
+ struct bio_list list = *tsk->bio_list;
+ bio_list_init(tsk->bio_list);
- /*
- * In order to guarantee forward progress we must punt only bios that
- * were allocated from this bio_set; otherwise, if there was a bio on
- * there for a stacking driver higher up in the stack, processing it
- * could require allocating bios from this bio_set, and doing that from
- * our own rescuer would be bad.
- *
- * Since bio lists are singly linked, pop them all instead of trying to
- * remove from the middle of the list:
- */
-
- bio_list_init(&punt);
- bio_list_init(&nopunt);
-
- while ((bio = bio_list_pop(current->bio_list)))
- bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
-
- *current->bio_list = nopunt;
-
- spin_lock(&bs->rescue_lock);
- bio_list_merge(&bs->rescue_list, &punt);
- spin_unlock(&bs->rescue_lock);
+ while ((bio = bio_list_pop(&list))) {
+ struct bio_set *bs = bio->bi_pool;
+ if (unlikely(!bs) || bs == fs_bio_set) {
+ bio_list_add(tsk->bio_list, bio);
+ continue;
+ }
- queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ spin_lock(&bs->rescue_lock);
+ bio_list_add(&bs->rescue_list, bio);
+ queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ spin_unlock(&bs->rescue_lock);
+ }
}
/**
@@ -425,7 +427,6 @@ static void punt_bios_to_rescuer(struct
*/
struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
{
- gfp_t saved_gfp = gfp_mask;
unsigned front_pad;
unsigned inline_vecs;
struct bio_vec *bvl = NULL;
@@ -459,23 +460,11 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
* reserve.
*
* We solve this, and guarantee forward progress, with a rescuer
- * workqueue per bio_set. If we go to allocate and there are
- * bios on current->bio_list, we first try the allocation
- * without __GFP_DIRECT_RECLAIM; if that fails, we punt those
- * bios we would be blocking to the rescuer workqueue before
- * we retry with the original gfp_flags.
+ * workqueue per bio_set. If an allocation would block (due to
+ * __GFP_DIRECT_RECLAIM) the scheduler will first punt all bios
+ * on current->bio_list to the rescuer workqueue.
*/
-
- if (current->bio_list && !bio_list_empty(current->bio_list))
- gfp_mask &= ~__GFP_DIRECT_RECLAIM;
-
p = mempool_alloc(bs->bio_pool, gfp_mask);
- if (!p && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- p = mempool_alloc(bs->bio_pool, gfp_mask);
- }
-
front_pad = bs->front_pad;
inline_vecs = BIO_INLINE_VECS;
}
@@ -490,12 +479,6 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
unsigned long idx = 0;
bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- if (!bvl && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- }
-
if (unlikely(!bvl))
goto err_free;
Index: linux-4.10-rc2/include/linux/blkdev.h
===================================================================
--- linux-4.10-rc2.orig/include/linux/blkdev.h
+++ linux-4.10-rc2/include/linux/blkdev.h
@@ -1267,6 +1267,22 @@ static inline bool blk_needs_flush_plug(
!list_empty(&plug->cb_list));
}
+extern void blk_flush_bio_list(struct task_struct *tsk);
+
+static inline void blk_flush_queued_io(struct task_struct *tsk)
+{
+ /*
+ * Flush any queued bios to corresponding rescue threads.
+ */
+ if (tsk->bio_list && !bio_list_empty(tsk->bio_list))
+ blk_flush_bio_list(tsk);
+ /*
+ * Flush any plugged IO that is queued.
+ */
+ if (blk_needs_flush_plug(tsk))
+ blk_schedule_flush_plug(tsk);
+}
+
/*
* tag stuff
*/
@@ -1921,16 +1937,10 @@ static inline void blk_flush_plug(struct
{
}
-static inline void blk_schedule_flush_plug(struct task_struct *task)
+static inline void blk_flush_queued_io(struct task_struct *tsk)
{
}
-
-static inline bool blk_needs_flush_plug(struct task_struct *tsk)
-{
- return false;
-}
-
static inline int blkdev_issue_flush(struct block_device *bdev, gfp_t gfp_mask,
sector_t *error_sector)
{
Index: linux-4.10-rc2/kernel/sched/core.c
===================================================================
--- linux-4.10-rc2.orig/kernel/sched/core.c
+++ linux-4.10-rc2/kernel/sched/core.c
@@ -3441,11 +3441,10 @@ static inline void sched_submit_work(str
if (!tsk->state || tsk_is_pi_blocked(tsk))
return;
/*
- * If we are going to sleep and we have plugged IO queued,
+ * If we are going to sleep and we have queued IO,
* make sure to submit it to avoid deadlocks.
*/
- if (blk_needs_flush_plug(tsk))
- blk_schedule_flush_plug(tsk);
+ blk_flush_queued_io(tsk);
}
asmlinkage __visible void __sched schedule(void)
@@ -5068,7 +5067,7 @@ long __sched io_schedule_timeout(long ti
long ret;
current->in_iowait = 1;
- blk_schedule_flush_plug(current);
+ blk_flush_queued_io(current);
delayacct_blkio_start();
rq = raw_rq();
^ permalink raw reply
* Re: v4.9, 4.4-final: 28 bioset threads on small notebook, 36 threads on cellphone
From: Mike Snitzer @ 2017-02-14 17:33 UTC (permalink / raw)
To: Kent Overstreet
Cc: Pavel Machek, kernel list, axboe, hch, neilb, martin.petersen,
dpark, ming.l, dm-devel, ming.lei, agk, jkosina, geoff, jim,
pjk1939, minchan, ngupta, oleg.drokin, andreas.dilger,
linux-block
In-Reply-To: <20170209212523.lgrgvk2squoo6x6f@moria.home.lan>
On Thu, Feb 09 2017 at 4:25pm -0500,
Kent Overstreet <kent.overstreet@gmail.com> wrote:
> On Wed, Feb 08, 2017 at 11:34:07AM -0500, Mike Snitzer wrote:
> > On Tue, Feb 07 2017 at 11:58pm -0500,
> > Kent Overstreet <kent.overstreet@gmail.com> wrote:
> >
> > > On Tue, Feb 07, 2017 at 09:39:11PM +0100, Pavel Machek wrote:
> > > > On Mon 2017-02-06 17:49:06, Kent Overstreet wrote:
> > > > > On Mon, Feb 06, 2017 at 04:47:24PM -0900, Kent Overstreet wrote:
> > > > > > On Mon, Feb 06, 2017 at 01:53:09PM +0100, Pavel Machek wrote:
> > > > > > > Still there on v4.9, 36 threads on nokia n900 cellphone.
> > > > > > >
> > > > > > > So.. what needs to be done there?
> > > > >
> > > > > > But, I just got an idea for how to handle this that might be halfway sane, maybe
> > > > > > I'll try and come up with a patch...
> > > > >
> > > > > Ok, here's such a patch, only lightly tested:
> > > >
> > > > I guess it would be nice for me to test it... but what it is against?
> > > > I tried after v4.10-rc5 and linux-next, but got rejects in both cases.
> > >
> > > Sorry, I forgot I had a few other patches in my branch that touch
> > > mempool/biosets code.
> > >
> > > Also, after thinking about it more and looking at the relevant code, I'm pretty
> > > sure we don't need rescuer threads for block devices that just split bios - i.e.
> > > most of them, so I changed my patch to do that.
> > >
> > > Tested it by ripping out the current->bio_list checks/workarounds from the
> > > bcache code, appears to work:
> >
> > Feedback on this patch below, but first:
> >
> > There are deeper issues with the current->bio_list and rescue workqueues
> > than thread counts.
> >
> > I cannot help but feel like you (and Jens) are repeatedly ignoring the
> > issue that has been raised numerous times, most recently:
> > https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html
> >
> > FYI, this test (albeit ugly) can be used to check if the dm-snapshot
> > deadlock is fixed:
> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
> >
> > This situation is the unfortunate pathological worst case for what
> > happens when changes are merged and nobody wants to own fixing the
> > unforseen implications/regressions. Like everyone else in a position
> > of Linux maintenance I've tried to stay away from owning the
> > responsibility of a fix -- it isn't working. Ok, I'll stop bitching
> > now.. I do bear responsibility for not digging in myself. We're all
> > busy and this issue is "hard".
>
> Mike, it's not my job to debug DM code for you or sift through your bug reports.
> I don't read dm-devel, and I don't know why you think I that's my job.
>
> If there's something you think the block layer should be doing differently, post
> patches - or at the very least, explain what you'd like to be done, with words.
> Don't get pissy because I'm not sifting through your bug reports.
>
> Hell, I'm not getting paid to work on kernel code at all right now, and you
> trying to rope me into fixing device mapper sure makes me want to work on the
> block layer more.
>
> DM developers have a long history of working siloed off from the rest of the
> block layer, building up their own crazy infrastructure (remember the old bio
> splitting code?) and going to extreme lengths to avoid having to work on or
> improve the core block layer infrastructure. It's ridiculous.
That is bullshit. I try to help block core where/when I can (did so
with discards and limits stacking, other fixes here and there).
Your take on what DM code is and how it evolved is way off base. But
that is to be expected from you. Not going to waste time laboring over
correcting you.
> You know what would be nice? What'd really make my day is if just once I got a
> thank you or a bit of appreciation from DM developers for the bvec iterators/bio
> splitting work I did that cleaned up a _lot_ of crazy hairy messes. Or getting
> rid of merge_bvec_fn, or trying to come up with a better solution for deadlocks
> due to running under generic_make_request() now.
I do appreciate the immutable biovec work you did. Helped speed up bio
cloning quite nicely.
But you've always been hostile to DM. You'll fabricate any excuse to
never touch or care about it. Repeatedly noted. But this is a block
core bug/regression. Not DM.
If you think I'm going to thank you for blindly breaking shit, and
refusing to care when you're made aware of it, then you're out of your
mind.
Save your predictably hostile response. It'll get marked as spam
anyway.
^ permalink raw reply
* [PATCH 4/6] scsi: simplify scsi_execute_req_flags
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
Add a sshdr argument to __scsi_execute so that we can decode the sense
data directly into the sense header instead of needing a copy of it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/scsi_lib.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 227a77869e13..35b43a8f1bfa 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -215,8 +215,9 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
- unsigned char *sense, int timeout, int retries, u64 flags,
- req_flags_t rq_flags, int *resid)
+ unsigned char *sense, struct scsi_sense_hdr *sshdr,
+ int timeout, int retries, u64 flags, req_flags_t rq_flags,
+ int *resid)
{
struct request *req;
struct scsi_request *rq;
@@ -259,6 +260,8 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
*resid = rq->resid_len;
if (sense && rq->sense_len)
memcpy(sense, rq->sense, SCSI_SENSE_BUFFERSIZE);
+ if (sshdr)
+ scsi_normalize_sense(rq->sense, rq->sense_len, sshdr);
ret = req->errors;
out:
blk_put_request(req);
@@ -288,7 +291,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int *resid)
{
return __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
- timeout, retries, flags, 0, resid);
+ NULL, timeout, retries, flags, 0, resid);
}
EXPORT_SYMBOL(scsi_execute);
@@ -297,21 +300,9 @@ int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd,
struct scsi_sense_hdr *sshdr, int timeout, int retries,
int *resid, u64 flags, req_flags_t rq_flags)
{
- char *sense = NULL;
- int result;
-
- if (sshdr) {
- sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
- if (!sense)
- return DRIVER_ERROR << 24;
- }
- result = __scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
- sense, timeout, retries, flags, rq_flags, resid);
- if (sshdr)
- scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, sshdr);
-
- kfree(sense);
- return result;
+ return __scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
+ NULL, sshdr, timeout, retries, flags, rq_flags,
+ resid);
}
EXPORT_SYMBOL(scsi_execute_req_flags);
--
2.11.0
^ permalink raw reply related
* sense handling improvements
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
Hi all,
this series is on top of the scsi_request changes in Jens' tree and
further improves the handling of the sense buffer.
The first patch prevents any possibily of reusing stale sense codes
in sense headers, and is a bug fix that we should probably get into
the block tree ASAP.
The rest cleans up handling of the parsed sense data and could go in
either through the block tree, or a SCSI branch on top of the block
tree.
^ permalink raw reply
* [PATCH 1/6] scsi: always zero sshdr in scsi_normalize_sense
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
This gives us a clear state even if a command didn't return sense data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/scsi_common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index b1383a71400e..a75673bb82b3 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -137,11 +137,11 @@ EXPORT_SYMBOL(int_to_scsilun);
bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
struct scsi_sense_hdr *sshdr)
{
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
if (!sense_buffer || !sb_len)
return false;
- memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
-
sshdr->response_code = (sense_buffer[0] & 0x7f);
if (!scsi_sense_valid(sshdr))
--
2.11.0
^ permalink raw reply related
* [PATCH 2/6] sd: improve TUR handling in sd_check_events
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
Remove bogus evaluations of retval and sshdr when the device is offline,
and fix a possible NULL pointer dereference by allocating the 8 byte
sized sense header on stack.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/sd.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 40b4038c019e..11e290d1dbff 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1425,7 +1425,6 @@ static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
{
struct scsi_disk *sdkp = scsi_disk_get(disk);
struct scsi_device *sdp;
- struct scsi_sense_hdr *sshdr = NULL;
int retval;
if (!sdkp)
@@ -1454,22 +1453,21 @@ static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
* by sd_spinup_disk() from sd_revalidate_disk(), which happens whenever
* sd_revalidate() is called.
*/
- retval = -ENODEV;
-
if (scsi_block_when_processing_errors(sdp)) {
- sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
+ struct scsi_sense_hdr sshdr = { 0, };
+
retval = scsi_test_unit_ready(sdp, SD_TIMEOUT, SD_MAX_RETRIES,
- sshdr);
- }
+ &sshdr);
- /* failed to execute TUR, assume media not present */
- if (host_byte(retval)) {
- set_media_not_present(sdkp);
- goto out;
- }
+ /* failed to execute TUR, assume media not present */
+ if (host_byte(retval)) {
+ set_media_not_present(sdkp);
+ goto out;
+ }
- if (media_not_present(sdkp, sshdr))
- goto out;
+ if (media_not_present(sdkp, &sshdr))
+ goto out;
+ }
/*
* For removable scsi disk we have to recognise the presence
@@ -1485,7 +1483,6 @@ static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
* Medium present state has changed in either direction.
* Device has indicated UNIT_ATTENTION.
*/
- kfree(sshdr);
retval = sdp->changed ? DISK_EVENT_MEDIA_CHANGE : 0;
sdp->changed = 0;
scsi_disk_put(sdkp);
--
2.11.0
^ permalink raw reply related
* [PATCH 3/6] scsi: make the sense header argument to scsi_test_unit_ready mandatory
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
It's a tiny structure that can be allocated on the stack, don't
complicate the code by making it optional.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/osd/osd_uld.c | 3 ++-
drivers/scsi/scsi_ioctl.c | 3 ++-
drivers/scsi/scsi_lib.c | 14 ++------------
3 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 243eab3d10d0..e0ce5d2fd14d 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -372,6 +372,7 @@ EXPORT_SYMBOL(osduld_device_same);
static int __detect_osd(struct osd_uld_device *oud)
{
struct scsi_device *scsi_device = oud->od.scsi_device;
+ struct scsi_sense_hdr sense_hdr;
char caps[OSD_CAP_LEN];
int error;
@@ -380,7 +381,7 @@ static int __detect_osd(struct osd_uld_device *oud)
*/
OSD_DEBUG("start scsi_test_unit_ready %p %p %p\n",
oud, scsi_device, scsi_device->request_queue);
- error = scsi_test_unit_ready(scsi_device, 10*HZ, 5, NULL);
+ error = scsi_test_unit_ready(scsi_device, 10*HZ, 5, &sense_hdr);
if (error)
OSD_ERR("warning: scsi_test_unit_ready failed\n");
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 8b8c814df5c7..b6bf3f29a12a 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -199,6 +199,7 @@ static int scsi_ioctl_get_pci(struct scsi_device *sdev, void __user *arg)
int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
{
char scsi_cmd[MAX_COMMAND_SIZE];
+ struct scsi_sense_hdr sense_hdr;
/* Check for deprecated ioctls ... all the ioctls which don't
* follow the new unique numbering scheme are deprecated */
@@ -243,7 +244,7 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
return scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
case SCSI_IOCTL_TEST_UNIT_READY:
return scsi_test_unit_ready(sdev, IOCTL_NORMAL_TIMEOUT,
- NORMAL_RETRIES, NULL);
+ NORMAL_RETRIES, &sense_hdr);
case SCSI_IOCTL_START_UNIT:
scsi_cmd[0] = START_STOP;
scsi_cmd[1] = 0;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 90f65c8f487a..227a77869e13 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2496,28 +2496,20 @@ EXPORT_SYMBOL(scsi_mode_sense);
* @sdev: scsi device to change the state of.
* @timeout: command timeout
* @retries: number of retries before failing
- * @sshdr_external: Optional pointer to struct scsi_sense_hdr for
- * returning sense. Make sure that this is cleared before passing
- * in.
+ * @sshdr: outpout pointer for decoded sense information.
*
* Returns zero if unsuccessful or an error if TUR failed. For
* removable media, UNIT_ATTENTION sets ->changed flag.
**/
int
scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
- struct scsi_sense_hdr *sshdr_external)
+ struct scsi_sense_hdr *sshdr)
{
char cmd[] = {
TEST_UNIT_READY, 0, 0, 0, 0, 0,
};
- struct scsi_sense_hdr *sshdr;
int result;
- if (!sshdr_external)
- sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
- else
- sshdr = sshdr_external;
-
/* try to eat the UNIT_ATTENTION if there are enough retries */
do {
result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, sshdr,
@@ -2528,8 +2520,6 @@ scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
} while (scsi_sense_valid(sshdr) &&
sshdr->sense_key == UNIT_ATTENTION && --retries);
- if (!sshdr_external)
- kfree(sshdr);
return result;
}
EXPORT_SYMBOL(scsi_test_unit_ready);
--
2.11.0
^ permalink raw reply related
* [PATCH 5/6] scsi: merge __scsi_execute into scsi_execute
From: Christoph Hellwig @ 2017-02-14 19:15 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
All but one caller want the decoded sense header, so offer the existing
__scsi_execute helper as the public scsi_execute API to simply the
callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 12 ++++------
drivers/scsi/cxlflash/superpipe.c | 8 +++----
drivers/scsi/cxlflash/vlun.c | 4 ++--
drivers/scsi/scsi_lib.c | 48 +++++++++++++++++----------------------
drivers/scsi/scsi_transport_spi.c | 24 ++++++++------------
drivers/scsi/sr_ioctl.c | 19 +++-------------
include/scsi/scsi_device.h | 5 ++--
7 files changed, 46 insertions(+), 74 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index c771d4c341ea..0b1e6762c6f9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -607,6 +607,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
u8 args[4], *argbuf = NULL, *sensebuf = NULL;
int argsize = 0;
enum dma_data_direction data_dir;
+ struct scsi_sense_hdr sshdr;
int cmd_result;
if (arg == NULL)
@@ -655,7 +656,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
/* Good values for timeout and retries? Values below
from scsi_ioctl_send_command() for default case... */
cmd_result = scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, argsize,
- sensebuf, (10*HZ), 5, 0, NULL);
+ sensebuf, &sshdr, (10*HZ), 5, 0, 0, NULL);
if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
u8 *desc = sensebuf + 8;
@@ -664,9 +665,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
/* If we set cc then ATA pass-through will cause a
* check condition even if no error. Filter that. */
if (cmd_result & SAM_STAT_CHECK_CONDITION) {
- struct scsi_sense_hdr sshdr;
- scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
- &sshdr);
if (sshdr.sense_key == RECOVERED_ERROR &&
sshdr.asc == 0 && sshdr.ascq == 0x1d)
cmd_result &= ~SAM_STAT_CHECK_CONDITION;
@@ -714,6 +712,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
int rc = 0;
u8 scsi_cmd[MAX_COMMAND_SIZE];
u8 args[7], *sensebuf = NULL;
+ struct scsi_sense_hdr sshdr;
int cmd_result;
if (arg == NULL)
@@ -741,7 +740,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
/* Good values for timeout and retries? Values below
from scsi_ioctl_send_command() for default case... */
cmd_result = scsi_execute(scsidev, scsi_cmd, DMA_NONE, NULL, 0,
- sensebuf, (10*HZ), 5, 0, NULL);
+ sensebuf, &sshdr, (10*HZ), 5, 0, 0, NULL);
if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
u8 *desc = sensebuf + 8;
@@ -750,9 +749,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
/* If we set cc then ATA pass-through will cause a
* check condition even if no error. Filter that. */
if (cmd_result & SAM_STAT_CHECK_CONDITION) {
- struct scsi_sense_hdr sshdr;
- scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
- &sshdr);
if (sshdr.sense_key == RECOVERED_ERROR &&
sshdr.asc == 0 && sshdr.ascq == 0x1d)
cmd_result &= ~SAM_STAT_CHECK_CONDITION;
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 9636970d9611..005a377a427f 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -305,6 +305,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
struct cxlflash_cfg *cfg = (struct cxlflash_cfg *)sdev->host->hostdata;
struct device *dev = &cfg->dev->dev;
struct glun_info *gli = lli->parent;
+ struct scsi_sense_hdr sshdr;
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
u8 *sense_buf = NULL;
@@ -332,7 +333,8 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_FROM_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, to, CMD_RETRIES, 0, NULL);
+ CMD_BUFSIZE, sense_buf, &sshdr, to, CMD_RETRIES,
+ 0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
if (rc) {
@@ -345,10 +347,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
if (driver_byte(result) == DRIVER_SENSE) {
result &= ~(0xFF<<24); /* DRIVER_SENSE is not an error */
if (result & SAM_STAT_CHECK_CONDITION) {
- struct scsi_sense_hdr sshdr;
-
- scsi_normalize_sense(sense_buf, SCSI_SENSE_BUFFERSIZE,
- &sshdr);
switch (sshdr.sense_key) {
case NO_SENSE:
case RECOVERED_ERROR:
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 90c5d7f5278e..6d191db1d874 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -454,8 +454,8 @@ static int write_same16(struct scsi_device *sdev,
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, to, CMD_RETRIES,
- 0, NULL);
+ CMD_BUFSIZE, sense_buf, NULL, to,
+ CMD_RETRIES, 0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
if (rc) {
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 35b43a8f1bfa..a15c188c2a7f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -213,7 +213,26 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
__scsi_queue_insert(cmd, reason, 1);
}
-static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+
+/**
+ * scsi_execute - insert request and wait for the result
+ * @sdev: scsi device
+ * @cmd: scsi command
+ * @data_direction: data direction
+ * @buffer: data buffer
+ * @bufflen: len of buffer
+ * @sense: optional sense buffer
+ * @sshdr: optional decoded sense header
+ * @timeout: request timeout in seconds
+ * @retries: number of times to retry request
+ * @flags: flags for ->cmd_flags
+ * @rq_flags: flags for ->rq_flags
+ * @resid: optional residual length
+ *
+ * returns the req->errors value which is the scsi_cmnd result
+ * field.
+ */
+int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -268,31 +287,6 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
return ret;
}
-
-/**
- * scsi_execute - insert request and wait for the result
- * @sdev: scsi device
- * @cmd: scsi command
- * @data_direction: data direction
- * @buffer: data buffer
- * @bufflen: len of buffer
- * @sense: optional sense buffer
- * @timeout: request timeout in seconds
- * @retries: number of times to retry request
- * @flags: or into request flags;
- * @resid: optional residual length
- *
- * returns the req->errors value which is the scsi_cmnd result
- * field.
- */
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
- int data_direction, void *buffer, unsigned bufflen,
- unsigned char *sense, int timeout, int retries, u64 flags,
- int *resid)
-{
- return __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
- NULL, timeout, retries, flags, 0, resid);
-}
EXPORT_SYMBOL(scsi_execute);
int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd,
@@ -300,7 +294,7 @@ int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd,
struct scsi_sense_hdr *sshdr, int timeout, int retries,
int *resid, u64 flags, req_flags_t rq_flags)
{
- return __scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
+ return scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
NULL, sshdr, timeout, retries, flags, rq_flags,
resid);
}
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index 319868f3f674..d0219e36080c 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -123,25 +123,21 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd,
{
int i, result;
unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+ struct scsi_sense_hdr sshdr_tmp;
+
+ if (!sshdr)
+ sshdr = &sshdr_tmp;
for(i = 0; i < DV_RETRIES; i++) {
- result = scsi_execute(sdev, cmd, dir, buffer, bufflen,
- sense, DV_TIMEOUT, /* retries */ 1,
+ result = scsi_execute(sdev, cmd, dir, buffer, bufflen, sense,
+ sshdr, DV_TIMEOUT, /* retries */ 1,
REQ_FAILFAST_DEV |
REQ_FAILFAST_TRANSPORT |
REQ_FAILFAST_DRIVER,
- NULL);
- if (driver_byte(result) & DRIVER_SENSE) {
- struct scsi_sense_hdr sshdr_tmp;
- if (!sshdr)
- sshdr = &sshdr_tmp;
-
- if (scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
- sshdr)
- && sshdr->sense_key == UNIT_ATTENTION)
- continue;
- }
- break;
+ 0, NULL);
+ if (!(driver_byte(result) & DRIVER_SENSE) ||
+ sshdr->sense_key != UNIT_ATTENTION)
+ break;
}
return result;
}
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index dfffdf63e44c..4610c8c5693f 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -187,30 +187,19 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
struct scsi_device *SDev;
struct scsi_sense_hdr sshdr;
int result, err = 0, retries = 0;
- struct request_sense *sense = cgc->sense;
SDev = cd->device;
- if (!sense) {
- sense = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
- if (!sense) {
- err = -ENOMEM;
- goto out;
- }
- }
-
retry:
if (!scsi_block_when_processing_errors(SDev)) {
err = -ENODEV;
goto out;
}
- memset(sense, 0, sizeof(*sense));
result = scsi_execute(SDev, cgc->cmd, cgc->data_direction,
- cgc->buffer, cgc->buflen, (char *)sense,
- cgc->timeout, IOCTL_RETRIES, 0, NULL);
-
- scsi_normalize_sense((char *)sense, sizeof(*sense), &sshdr);
+ cgc->buffer, cgc->buflen,
+ (unsigned char *)cgc->sense, &sshdr,
+ cgc->timeout, IOCTL_RETRIES, 0, 0, NULL);
/* Minimal error checking. Ignore cases we know about, and report the rest. */
if (driver_byte(result) != 0) {
@@ -261,8 +250,6 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
/* Wake up a process waiting for device */
out:
- if (!cgc->sense)
- kfree(sense);
cgc->stat = err;
return err;
}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e580b278..8c8d243db32b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -409,8 +409,9 @@ extern int scsi_is_target_device(const struct device *);
extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
- unsigned char *sense, int timeout, int retries,
- u64 flags, int *resid);
+ unsigned char *sense, struct scsi_sense_hdr *sshdr,
+ int timeout, int retries, u64 flags,
+ req_flags_t rq_flags, int *resid);
extern int scsi_execute_req_flags(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
--
2.11.0
^ permalink raw reply related
* [PATCH 6/6] scsi: remove scsi_execute_req_flags
From: Christoph Hellwig @ 2017-02-14 19:16 UTC (permalink / raw)
To: axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
And switch all callers to use scsi_execute instead.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/device_handler/scsi_dh_alua.c | 16 ++++++----------
drivers/scsi/device_handler/scsi_dh_emc.c | 7 +++----
drivers/scsi/device_handler/scsi_dh_hp_sw.c | 10 ++++------
drivers/scsi/device_handler/scsi_dh_rdac.c | 7 +++----
drivers/scsi/scsi_lib.c | 11 -----------
drivers/scsi/sd.c | 9 ++++-----
drivers/scsi/ufs/ufshcd.c | 10 +++++-----
include/scsi/scsi_device.h | 8 ++------
8 files changed, 27 insertions(+), 51 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index d704752b6332..48e200102221 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -151,11 +151,9 @@ static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
cdb[1] = MI_REPORT_TARGET_PGS;
put_unaligned_be32(bufflen, &cdb[6]);
- return scsi_execute_req_flags(sdev, cdb, DMA_FROM_DEVICE,
- buff, bufflen, sshdr,
- ALUA_FAILOVER_TIMEOUT * HZ,
- ALUA_FAILOVER_RETRIES, NULL,
- req_flags, 0);
+ return scsi_execute(sdev, cdb, DMA_FROM_DEVICE, buff, bufflen, NULL,
+ sshdr, ALUA_FAILOVER_TIMEOUT * HZ,
+ ALUA_FAILOVER_RETRIES, req_flags, 0, NULL);
}
/*
@@ -185,11 +183,9 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
cdb[1] = MO_SET_TARGET_PGS;
put_unaligned_be32(stpg_len, &cdb[6]);
- return scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
- stpg_data, stpg_len,
- sshdr, ALUA_FAILOVER_TIMEOUT * HZ,
- ALUA_FAILOVER_RETRIES, NULL,
- req_flags, 0);
+ return scsi_execute(sdev, cdb, DMA_TO_DEVICE, stpg_data, stpg_len, NULL,
+ sshdr, ALUA_FAILOVER_TIMEOUT * HZ,
+ ALUA_FAILOVER_RETRIES, req_flags, 0, NULL);
}
static struct alua_port_group *alua_find_get_pg(char *id_str, size_t id_size,
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 4a7679f6c73d..3997f21794df 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -276,10 +276,9 @@ static int send_trespass_cmd(struct scsi_device *sdev,
BUG_ON((len > CLARIION_BUFFER_SIZE));
memcpy(csdev->buffer, page22, len);
- err = scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
- csdev->buffer, len, &sshdr,
- CLARIION_TIMEOUT * HZ, CLARIION_RETRIES,
- NULL, req_flags, 0);
+ err = scsi_execute(sdev, cdb, DMA_TO_DEVICE, csdev->buffer, len, NULL,
+ &sshdr, CLARIION_TIMEOUT * HZ, CLARIION_RETRIES,
+ req_flags, 0, NULL);
if (err) {
if (scsi_sense_valid(&sshdr))
res = trespass_endio(sdev, &sshdr);
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index be43c940636d..62d314e07d11 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -100,9 +100,8 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
REQ_FAILFAST_DRIVER;
retry:
- res = scsi_execute_req_flags(sdev, cmd, DMA_NONE, NULL, 0, &sshdr,
- HP_SW_TIMEOUT, HP_SW_RETRIES,
- NULL, req_flags, 0);
+ res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+ HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
if (res) {
if (scsi_sense_valid(&sshdr))
ret = tur_done(sdev, h, &sshdr);
@@ -139,9 +138,8 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
REQ_FAILFAST_DRIVER;
retry:
- res = scsi_execute_req_flags(sdev, cmd, DMA_NONE, NULL, 0, &sshdr,
- HP_SW_TIMEOUT, HP_SW_RETRIES,
- NULL, req_flags, 0);
+ res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+ HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
if (res) {
if (!scsi_sense_valid(&sshdr)) {
sdev_printk(KERN_WARNING, sdev,
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index b64eaae8533d..3cbab8710e58 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -555,10 +555,9 @@ static void send_mode_select(struct work_struct *work)
(char *) h->ctlr->array_name, h->ctlr->index,
(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
- if (scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
- &h->ctlr->mode_select, data_size, &sshdr,
- RDAC_TIMEOUT * HZ,
- RDAC_RETRIES, NULL, req_flags, 0)) {
+ if (scsi_execute(sdev, cdb, DMA_TO_DEVICE, &h->ctlr->mode_select,
+ data_size, NULL, &sshdr, RDAC_TIMEOUT * HZ,
+ RDAC_RETRIES, req_flags, 0, NULL)) {
err = mode_select_handle_sense(sdev, &sshdr);
if (err == SCSI_DH_RETRY && retry_cnt--)
goto retry;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a15c188c2a7f..e689a358cd05 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -289,17 +289,6 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
}
EXPORT_SYMBOL(scsi_execute);
-int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd,
- int data_direction, void *buffer, unsigned bufflen,
- struct scsi_sense_hdr *sshdr, int timeout, int retries,
- int *resid, u64 flags, req_flags_t rq_flags)
-{
- return scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
- NULL, sshdr, timeout, retries, flags, rq_flags,
- resid);
-}
-EXPORT_SYMBOL(scsi_execute_req_flags);
-
/*
* Function: scsi_init_cmd_errh()
*
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 11e290d1dbff..817e0aa78f68 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1508,9 +1508,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
* Leave the rest of the command zero to indicate
* flush everything.
*/
- res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
- &sshdr, timeout, SD_MAX_RETRIES,
- NULL, 0, RQF_PM);
+ res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+ timeout, SD_MAX_RETRIES, 0, RQF_PM, NULL);
if (res == 0)
break;
}
@@ -3296,8 +3295,8 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
if (!scsi_device_online(sdp))
return -ENODEV;
- res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
- SD_TIMEOUT, SD_MAX_RETRIES, NULL, 0, RQF_PM);
+ res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+ SD_TIMEOUT, SD_MAX_RETRIES, 0, RQF_PM, NULL);
if (res) {
sd_print_result(sdkp, "Start/Stop Unit failed", res);
if (driver_byte(res) & DRIVER_SENSE)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 20e5e5fb048c..9b6956662980 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5859,9 +5859,9 @@ ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp)
goto out;
}
- ret = scsi_execute_req_flags(sdp, cmd, DMA_FROM_DEVICE, buffer,
- UFSHCD_REQ_SENSE_SIZE, NULL,
- msecs_to_jiffies(1000), 3, NULL, 0, RQF_PM);
+ ret = scsi_execute(sdp, cmd, DMA_FROM_DEVICE, buffer,
+ UFSHCD_REQ_SENSE_SIZE, NULL, NULL,
+ msecs_to_jiffies(1000), 3, 0, RQF_PM, NULL);
if (ret)
pr_err("%s: failed with err %d\n", __func__, ret);
@@ -5926,8 +5926,8 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
* callbacks hence set the RQF_PM flag so that it doesn't resume the
* already suspended childs.
*/
- ret = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
- START_STOP_TIMEOUT, 0, NULL, 0, RQF_PM);
+ ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+ START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
if (ret) {
sdev_printk(KERN_WARNING, sdp,
"START_STOP failed for power mode: %d, result %x\n",
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8c8d243db32b..c741a20f49e6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -412,17 +412,13 @@ extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
req_flags_t rq_flags, int *resid);
-extern int scsi_execute_req_flags(struct scsi_device *sdev,
- const unsigned char *cmd, int data_direction, void *buffer,
- unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
- int retries, int *resid, u64 flags, req_flags_t rq_flags);
static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
int retries, int *resid)
{
- return scsi_execute_req_flags(sdev, cmd, data_direction, buffer,
- bufflen, sshdr, timeout, retries, resid, 0, 0);
+ return scsi_execute(sdev, cmd, data_direction, buffer,
+ bufflen, NULL, sshdr, timeout, retries, 0, 0, resid);
}
extern void sdev_disable_disk_events(struct scsi_device *sdev);
extern void sdev_enable_disk_events(struct scsi_device *sdev);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v4] mmc: sdio: check the buffer address for sdio API
From: Russell King - ARM Linux @ 2017-02-14 19:34 UTC (permalink / raw)
To: Jens Axboe
Cc: Ulf Hansson, Shawn Lin, linux-mmc@vger.kernel.org,
Christoph Hellwig, linux-block
In-Reply-To: <3ab69e5c-e661-3011-cab4-3066024ecf36@kernel.dk>
On Tue, Feb 14, 2017 at 09:18:43AM -0700, Jens Axboe wrote:
> The current situation seems like a bit of a mess. Why don't you have two
> entry points, one for DMA and one for PIO. If the caller doesn't know if
> he can use DMA, he'd better call the PIO variant. Either that, or audit
> all callers and ensure they do the right thing wrt having a dma capable
> buffer.
It really shouldn't matter. MMC interfaces are just like USB - you
have a host controller, which interfaces what is a multi-lane serial
bus to the system. The SDIO card shouldn't care one bit whether
the host controller is using DMA or PIO.
However, I think that the DMA vs PIO thing is actually misleading here,
that's really not the issue at all.
Looking at the oops, I see it uses sdio_memcpy_toio(). Tracing that
code leads me to here:
for_each_sg(data.sg, sg_ptr, data.sg_len, i) {
sg_set_page(sg_ptr, virt_to_page(buf + (i * seg_size)),
min(seg_size, left_size),
offset_in_page(buf + (i * seg_size)));
so the buffer that is passed into sdio_memcpy_toio() gets passed into
virt_to_page().
Firstly, the fact that it's passed to virt_to_page() means that "buf"
must _only_ _ever_ be a lowmem address. It can't ever be a vmalloc
address (virt_to_page() is invalid on anything but lowmem.) Just like
certain kernel interfaces, passing pointers to memory of different types
from the one intended by the interface produces invalid results, and
that seems to be what's happening here.
Secondly, it's a scatterlist, and scatterlists can be passed to DMA
mapping operations, which also implies that _if_ a host driver decides
to use DMA on it, the buffer better be DMA-able.
Thirdly, while PIO may work (or even appear to work) because _maybe_
converting a vmalloc address to a ficticious struct page + offset, and
then converting that back again _might_ result in hitting the correct
memory, but it's not guaranteed to.
I suspect that virt_to_page() + kmap_atomic() is likely to try to
dereference a struct page pointer that does not point at a legal entry
in the memmap arrays, and result in scribbling over some random part
of kernel memory.
So every way I look at this, the binary driver that Shawn downloaded
is buggy, whether the host controller uses PIO or DMA.
I bet if Shawn tries running it against a modern kernel with
CONFIG_DEBUG_VIRTUAL enabled, Shawn will get complaints backing up
my claim.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH V6 1/3] uapi: sed-opal fix IOW for activate lsp to use correct struct
From: Scott Bauer @ 2017-02-15 0:29 UTC (permalink / raw)
To: linux-nvme
Cc: David.Laight, arnd, axboe, keith.busch, jonathan.derrick, hch,
linux-kernel, linux-block, Scott Bauer
The IOC_OPAL_ACTIVATE_LSP took the wrong strcure which would
give us the wrong size when using _IOC_SIZE, switch it to the
right structure.
Fixes: 058f8a2 ("Include: Uapi: Add user ABI for Sed/Opal")
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
include/uapi/linux/sed-opal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index fc06e3a..c72e073 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -106,7 +106,7 @@ struct opal_mbr_data {
#define IOC_OPAL_SAVE _IOW('p', 220, struct opal_lock_unlock)
#define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock)
#define IOC_OPAL_TAKE_OWNERSHIP _IOW('p', 222, struct opal_key)
-#define IOC_OPAL_ACTIVATE_LSP _IOW('p', 223, struct opal_key)
+#define IOC_OPAL_ACTIVATE_LSP _IOW('p', 223, struct opal_lr_act)
#define IOC_OPAL_SET_PW _IOW('p', 224, struct opal_new_pw)
#define IOC_OPAL_ACTIVATE_USR _IOW('p', 225, struct opal_session_info)
#define IOC_OPAL_REVERT_TPR _IOW('p', 226, struct opal_key)
--
2.7.4
^ permalink raw reply related
* [PATCH V6 2/3] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Scott Bauer @ 2017-02-15 0:29 UTC (permalink / raw)
To: linux-nvme
Cc: David.Laight, arnd, axboe, keith.busch, jonathan.derrick, hch,
linux-kernel, linux-block, Scott Bauer
In-Reply-To: <1487118577-1719-1-git-send-email-scott.bauer@intel.com>
When CONFIG_KASAN is enabled, compilation fails:
block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Moved all the ioctl structures off the stack and dynamically allocate
using _IOC_SIZE()
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 133 ++++++++++++++++-------------------------------
drivers/nvme/host/core.c | 3 +-
include/linux/sed-opal.h | 4 +-
3 files changed, 50 insertions(+), 90 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..e95b8a5 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2344,9 +2344,10 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
}
EXPORT_SYMBOL(opal_unlock_from_suspend);
-int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
+int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
{
- void __user *arg = (void __user *)ptr;
+ void *p;
+ int ret = -ENOTTY;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -2355,94 +2356,52 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
return -ENOTSUPP;
}
- switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_save(dev, &lk_unlk);
- }
- case IOC_OPAL_LOCK_UNLOCK: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_lock_unlock(dev, &lk_unlk);
- }
- case IOC_OPAL_TAKE_OWNERSHIP: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_take_ownership(dev, &opal_key);
- }
- case IOC_OPAL_ACTIVATE_LSP: {
- struct opal_lr_act opal_lr_act;
-
- if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
- return -EFAULT;
- return opal_activate_lsp(dev, &opal_lr_act);
- }
- case IOC_OPAL_SET_PW: {
- struct opal_new_pw opal_pw;
-
- if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
- return -EFAULT;
- return opal_set_new_pw(dev, &opal_pw);
- }
- case IOC_OPAL_ACTIVATE_USR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_activate_user(dev, &session);
- }
- case IOC_OPAL_REVERT_TPR: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_reverttper(dev, &opal_key);
- }
- case IOC_OPAL_LR_SETUP: {
- struct opal_user_lr_setup lrs;
+ p = memdup_user(arg, _IOC_SIZE(cmd));
+ if (IS_ERR(p))
+ return PTR_ERR(p);
- if (copy_from_user(&lrs, arg, sizeof(lrs)))
- return -EFAULT;
- return opal_setup_locking_range(dev, &lrs);
- }
- case IOC_OPAL_ADD_USR_TO_LR: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_add_user_to_lr(dev, &lk_unlk);
- }
- case IOC_OPAL_ENABLE_DISABLE_MBR: {
- struct opal_mbr_data mbr;
-
- if (copy_from_user(&mbr, arg, sizeof(mbr)))
- return -EFAULT;
- return opal_enable_disable_shadow_mbr(dev, &mbr);
- }
- case IOC_OPAL_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_erase_locking_range(dev, &session);
- }
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
+ switch (cmd) {
+ case IOC_OPAL_SAVE:
+ ret = opal_save(dev, p);
+ break;
+ case IOC_OPAL_LOCK_UNLOCK:
+ ret = opal_lock_unlock(dev, p);
+ break;
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ ret = opal_take_ownership(dev, p);
+ break;
+ case IOC_OPAL_ACTIVATE_LSP:
+ ret = opal_activate_lsp(dev, p);
+ break;
+ case IOC_OPAL_SET_PW:
+ ret = opal_set_new_pw(dev, p);
+ break;
+ case IOC_OPAL_ACTIVATE_USR:
+ ret = opal_activate_user(dev, p);
+ break;
+ case IOC_OPAL_REVERT_TPR:
+ ret = opal_reverttper(dev, p);
+ break;
+ case IOC_OPAL_LR_SETUP:
+ ret = opal_setup_locking_range(dev, p);
+ break;
+ case IOC_OPAL_ADD_USR_TO_LR:
+ ret = opal_add_user_to_lr(dev, p);
+ break;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ ret = opal_enable_disable_shadow_mbr(dev, p);
+ break;
+ case IOC_OPAL_ERASE_LR:
+ ret = opal_erase_locking_range(dev, p);
+ break;
+ case IOC_OPAL_SECURE_ERASE_LR:
+ ret = opal_secure_erase_locking_range(dev, p);
+ break;
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
- return -ENOTTY;
+
+ kfree(p);
+ return ret;
}
EXPORT_SYMBOL_GPL(sed_ioctl);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d9f4903..04c48e7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd, arg);
+ return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ (void __user *) arg);
return -ENOTTY;
}
}
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index af1a85e..205d520 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -132,7 +132,7 @@ struct opal_dev {
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
-int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr);
+int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);
static inline bool is_sed_ioctl(unsigned int cmd)
{
@@ -160,7 +160,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
}
static inline int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
- unsigned long ptr)
+ void __user *ioctl_ptr)
{
return 0;
}
--
2.7.4
^ permalink raw reply related
* [PATCH V6 3/3] Maintainers: Modify SED list from nvme to block
From: Scott Bauer @ 2017-02-15 0:29 UTC (permalink / raw)
To: linux-nvme
Cc: David.Laight, arnd, axboe, keith.busch, jonathan.derrick, hch,
linux-kernel, linux-block, Scott Bauer
In-Reply-To: <1487118577-1719-1-git-send-email-scott.bauer@intel.com>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index e325373..b983b25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11094,7 +11094,7 @@ SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER
M: Scott Bauer <scott.bauer@intel.com>
M: Jonathan Derrick <jonathan.derrick@intel.com>
M: Rafael Antognolli <rafael.antognolli@intel.com>
-L: linux-nvme@lists.infradead.org
+L: linux-block@vger.kernel.org
S: Supported
F: block/sed*
F: block/opal_proto.h
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v1 1/5] block: introduce bio_clone_bioset_partial()
From: Ming Lei @ 2017-02-15 2:26 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
NeilBrown
In-Reply-To: <20170214160151.GB32705@infradead.org>
On Wed, Feb 15, 2017 at 12:01 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Feb 14, 2017 at 09:04:26AM +0800, Ming Lei wrote:
>> On Mon, Feb 13, 2017 at 9:46 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Fri, Feb 10, 2017 at 06:56:13PM +0800, Ming Lei wrote:
>> >> md still need bio clone(not the fast version) for behind write,
>> >> and it is more efficient to use bio_clone_bioset_partial().
>> >>
>> >> The idea is simple and just copy the bvecs range specified from
>> >> parameters.
>> >
>> > Given how few users bio_clone_bioset has I wonder if we shouldn't
>> > simply add the two new arguments to it instead of adding another
>> > indirection.
>>
>> For md write-behind, looks we have to provide the two arguments,
>> could you explain a bit how we can do that by adding another indirection?
>
> I meant to just pass the additional arguments that
> bio_clone_bioset_partial has to bio_clone_bioset.
That may cause more changes(fs, ...) into this patchset, so I suggest to
do that in another patchset, especially after we confirmed current
users of bio_clone is absolutely necessary, and I will check if other bio_clone
can be converted to fast clone.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH V6 1/3] uapi: sed-opal fix IOW for activate lsp to use correct struct
From: Jens Axboe @ 2017-02-15 2:47 UTC (permalink / raw)
To: Scott Bauer, linux-nvme
Cc: David.Laight, arnd, keith.busch, jonathan.derrick, hch,
linux-kernel, linux-block
In-Reply-To: <1487118577-1719-1-git-send-email-scott.bauer@intel.com>
On 02/14/2017 05:29 PM, Scott Bauer wrote:
> The IOC_OPAL_ACTIVATE_LSP took the wrong strcure which would
> give us the wrong size when using _IOC_SIZE, switch it to the
> right structure.
Applied 1-3, thanks.
--
Jens Axboe
^ permalink raw reply
* [PATCH] block: do not allow updates through sysfs until registration completes
From: Tahsin Erdogan @ 2017-02-15 3:27 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: linux-kernel, Tahsin Erdogan
When a new disk shows up, sysfs queue directory is created before elevator
is registered. This allows a user to attempt a scheduler switch even though
the initial registration hasn't completed yet.
In one scenario, blk_register_queue() calls elv_register_queue() and
right before cfq_registered_queue() is called, another process executes
elevator_switch() and replaces q->elevator with deadline scheduler. When
cfq_registered_queue() executes it interprets e->elevator_data as struct
cfq_data even though it is actually struct deadline_data.
Grab q->sysfs_lock in blk_register_queue() to synchronize with sysfs
callers.
Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
block/blk-sysfs.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 070d81bae1d5..002af836aa87 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -902,17 +902,20 @@ int blk_register_queue(struct gendisk *disk)
if (ret)
return ret;
+ if (q->mq_ops)
+ blk_mq_register_dev(dev, q);
+
+ /* Prevent changes through sysfs until registration is completed. */
+ mutex_lock(&q->sysfs_lock);
+
ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
if (ret < 0) {
blk_trace_remove_sysfs(dev);
- return ret;
+ goto unlock;
}
kobject_uevent(&q->kobj, KOBJ_ADD);
- if (q->mq_ops)
- blk_mq_register_dev(dev, q);
-
blk_wb_init(q);
if (q->request_fn || (q->mq_ops && q->elevator)) {
@@ -922,11 +925,13 @@ int blk_register_queue(struct gendisk *disk)
kobject_del(&q->kobj);
blk_trace_remove_sysfs(dev);
kobject_put(&dev->kobj);
- return ret;
+ goto unlock;
}
}
-
- return 0;
+ ret = 0;
+unlock:
+ mutex_unlock(&q->sysfs_lock);
+ return ret;
}
void blk_unregister_queue(struct gendisk *disk)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [PATCH v4] mmc: sdio: check the buffer address for sdio API
From: Shawn Lin @ 2017-02-15 4:12 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jens Axboe, Ulf Hansson, linux-mmc@vger.kernel.org,
Christoph Hellwig, linux-block
In-Reply-To: <20170214193457.GZ27312@n2100.armlinux.org.uk>
Hi Russell,
On 2017/2/15 3:34, Russell King - ARM Linux wrote:
> On Tue, Feb 14, 2017 at 09:18:43AM -0700, Jens Axboe wrote:
>> The current situation seems like a bit of a mess. Why don't you have two
>> entry points, one for DMA and one for PIO. If the caller doesn't know if
>> he can use DMA, he'd better call the PIO variant. Either that, or audit
>> all callers and ensure they do the right thing wrt having a dma capable
>> buffer.
>
> It really shouldn't matter. MMC interfaces are just like USB - you
> have a host controller, which interfaces what is a multi-lane serial
> bus to the system. The SDIO card shouldn't care one bit whether
> the host controller is using DMA or PIO.
>
> However, I think that the DMA vs PIO thing is actually misleading here,
> that's really not the issue at all.
>
> Looking at the oops, I see it uses sdio_memcpy_toio(). Tracing that
> code leads me to here:
>
> for_each_sg(data.sg, sg_ptr, data.sg_len, i) {
> sg_set_page(sg_ptr, virt_to_page(buf + (i * seg_size)),
> min(seg_size, left_size),
> offset_in_page(buf + (i * seg_size)));
>
> so the buffer that is passed into sdio_memcpy_toio() gets passed into
> virt_to_page().
>
> Firstly, the fact that it's passed to virt_to_page() means that "buf"
> must _only_ _ever_ be a lowmem address. It can't ever be a vmalloc
> address (virt_to_page() is invalid on anything but lowmem.) Just like
> certain kernel interfaces, passing pointers to memory of different types
> from the one intended by the interface produces invalid results, and
> that seems to be what's happening here.
>
> Secondly, it's a scatterlist, and scatterlists can be passed to DMA
> mapping operations, which also implies that _if_ a host driver decides
> to use DMA on it, the buffer better be DMA-able.
>
> Thirdly, while PIO may work (or even appear to work) because _maybe_
> converting a vmalloc address to a ficticious struct page + offset, and
> then converting that back again _might_ result in hitting the correct
> memory, but it's not guaranteed to.
>
[1]:
If no DMA involved, the host drivers usually use memcpy or readl/writel
to transfer the data between MMIO address and buffer coming from the
caller. So, is it also not guaranteed when using memcpy or readl to
transfer data between MMIO address and vmalloc/heap buffer?
> I suspect that virt_to_page() + kmap_atomic() is likely to try to
> dereference a struct page pointer that does not point at a legal entry
> in the memmap arrays, and result in scribbling over some random part
> of kernel memory.
If that is the fact, so what I am concerned mostly is that by
seraching the APIs, sdio_writeb and sdio_readb, under the drivers/net
/wireless/, I could see almost all sdio based WLAN drivers passed in
a vmalloc area(actually when built as moudle, it should be located in
MODULE range which also be included as vmalloc area, no?) or heap
buffer.
I assume my question[1] above is fine, then thanks to none of the mmc
host drivers use DMA for sdio_writeb and sdio_readb since it only
request one byte which didn't be fetched from host FIFO and the host
controller HW didn't support this kind of request to use DMA(but may be
not in the future). Otherwise, it may result in scribbling over some
random part of kernel memory.
Actually we didn't see that issues before, so I think the actual
question should be if the buffer from heap or vmalloc will be used with
DMA involved?
>
> So every way I look at this, the binary driver that Shawn downloaded
> is buggy, whether the host controller uses PIO or DMA.
So it's really more dangerous that we were/are taking risking of
scribbling over some memory belonging to other code even if using
PIO if it also not guaranteed to use heap/vmalloc buffer..
>
> I bet if Shawn tries running it against a modern kernel with
> CONFIG_DEBUG_VIRTUAL enabled, Shawn will get complaints backing up
> my claim.
>
^ permalink raw reply
* Re: [PATCH 1/6] scsi: always zero sshdr in scsi_normalize_sense
From: Hannes Reinecke @ 2017-02-15 7:59 UTC (permalink / raw)
To: Christoph Hellwig, axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-2-hch@lst.de>
On 02/14/2017 08:15 PM, Christoph Hellwig wrote:
> This gives us a clear state even if a command didn't return sense data.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/scsi/scsi_common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
> index b1383a71400e..a75673bb82b3 100644
> --- a/drivers/scsi/scsi_common.c
> +++ b/drivers/scsi/scsi_common.c
> @@ -137,11 +137,11 @@ EXPORT_SYMBOL(int_to_scsilun);
> bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> struct scsi_sense_hdr *sshdr)
> {
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
> if (!sense_buffer || !sb_len)
> return false;
>
> - memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> -
> sshdr->response_code = (sense_buffer[0] & 0x7f);
>
> if (!scsi_sense_valid(sshdr))
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* Re: [PATCH 2/6] sd: improve TUR handling in sd_check_events
From: Hannes Reinecke @ 2017-02-15 8:00 UTC (permalink / raw)
To: Christoph Hellwig, axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-3-hch@lst.de>
On 02/14/2017 08:15 PM, Christoph Hellwig wrote:
> Remove bogus evaluations of retval and sshdr when the device is offline,
> and fix a possible NULL pointer dereference by allocating the 8 byte
> sized sense header on stack.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/scsi/sd.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* Re: sense handling improvements
From: Hannes Reinecke @ 2017-02-15 8:19 UTC (permalink / raw)
To: Christoph Hellwig, axboe; +Cc: linux-block, linux-scsi
In-Reply-To: <20170214191600.17480-1-hch@lst.de>
On 02/14/2017 08:15 PM, Christoph Hellwig wrote:
> Hi all,
>
> this series is on top of the scsi_request changes in Jens' tree and
> further improves the handling of the sense buffer.
>
Sorry, but I'm feeling really daft: which scsi_request changes?
To be found in which tree?
I dimly remember seeing them, but have been unable to find them again.
> The first patch prevents any possibily of reusing stale sense codes
> in sense headers, and is a bug fix that we should probably get into
> the block tree ASAP.
>
> The rest cleans up handling of the parsed sense data and could go in
> either through the block tree, or a SCSI branch on top of the block
> tree.
>
Have we audited all drivers to _not_ do DMA into the sense buffer?
By first glance some still do, so they'll break horribly when moving the
sense buffer onto the stack ...
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* [PATCH] mmc: core: make block layer non-optional
From: Linus Walleij @ 2017-02-15 9:53 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente
Cc: linux-block, Jens Axboe, Christoph Hellwig, Arnd Bergmann,
Linus Walleij
I do not know at what point people have wanted to have a system
with MMC/SD support without the block layer. We are anyway now
so tightly integrated with the block layer that this is onlt
teoretical and it makes no sense to have the block layer interface
as optional.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/core/Kconfig | 12 ------------
drivers/mmc/core/Makefile | 5 ++---
2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index cdfa8520a4b1..2920bc4351ed 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -23,19 +23,8 @@ config PWRSEQ_SIMPLE
This driver can also be built as a module. If so, the module
will be called pwrseq_simple.
-config MMC_BLOCK
- tristate "MMC block device driver"
- depends on BLOCK
- default y
- help
- Say Y here to enable the MMC block device driver support.
- This provides a block device driver, which you can use to
- mount the filesystem. Almost everyone wishing MMC support
- should say Y or M here.
-
config MMC_BLOCK_MINORS
int "Number of minors per block device"
- depends on MMC_BLOCK
range 4 256
default 8
help
@@ -53,7 +42,6 @@ config MMC_BLOCK_MINORS
config MMC_BLOCK_BOUNCE
bool "Use bounce buffer for simple hosts"
- depends on MMC_BLOCK
default y
help
SD/MMC is a high latency protocol where it is crucial to
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index b2a257dc644f..42f0d781d911 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -7,12 +7,11 @@ mmc_core-y := core.o bus.o host.o \
mmc.o mmc_ops.o sd.o sd_ops.o \
sdio.o sdio_ops.o sdio_bus.o \
sdio_cis.o sdio_io.o sdio_irq.o \
- quirks.o slot-gpio.o
+ quirks.o slot-gpio.o \
+ block.o queue.o
mmc_core-$(CONFIG_OF) += pwrseq.o
obj-$(CONFIG_PWRSEQ_SIMPLE) += pwrseq_simple.o
obj-$(CONFIG_PWRSEQ_EMMC) += pwrseq_emmc.o
mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
-obj-$(CONFIG_MMC_BLOCK) += mmc_block.o
-mmc_block-objs := block.o queue.o
obj-$(CONFIG_MMC_TEST) += mmc_test.o
obj-$(CONFIG_SDIO_UART) += sdio_uart.o
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v4] mmc: sdio: check the buffer address for sdio API
From: Russell King - ARM Linux @ 2017-02-15 9:55 UTC (permalink / raw)
To: Shawn Lin
Cc: Jens Axboe, Ulf Hansson, linux-mmc@vger.kernel.org,
Christoph Hellwig, linux-block
In-Reply-To: <6e4cd1d2-9c35-fbce-e398-e7498514b930@rock-chips.com>
On Wed, Feb 15, 2017 at 12:12:47PM +0800, Shawn Lin wrote:
> Hi Russell,
>
> On 2017/2/15 3:34, Russell King - ARM Linux wrote:
> >On Tue, Feb 14, 2017 at 09:18:43AM -0700, Jens Axboe wrote:
> >>The current situation seems like a bit of a mess. Why don't you have two
> >>entry points, one for DMA and one for PIO. If the caller doesn't know if
> >>he can use DMA, he'd better call the PIO variant. Either that, or audit
> >>all callers and ensure they do the right thing wrt having a dma capable
> >>buffer.
> >
> >It really shouldn't matter. MMC interfaces are just like USB - you
> >have a host controller, which interfaces what is a multi-lane serial
> >bus to the system. The SDIO card shouldn't care one bit whether
> >the host controller is using DMA or PIO.
> >
> >However, I think that the DMA vs PIO thing is actually misleading here,
> >that's really not the issue at all.
> >
> >Looking at the oops, I see it uses sdio_memcpy_toio(). Tracing that
> >code leads me to here:
> >
> > for_each_sg(data.sg, sg_ptr, data.sg_len, i) {
> > sg_set_page(sg_ptr, virt_to_page(buf + (i * seg_size)),
> > min(seg_size, left_size),
> > offset_in_page(buf + (i * seg_size)));
> >
> >so the buffer that is passed into sdio_memcpy_toio() gets passed into
> >virt_to_page().
> >
> >Firstly, the fact that it's passed to virt_to_page() means that "buf"
> >must _only_ _ever_ be a lowmem address. It can't ever be a vmalloc
> >address (virt_to_page() is invalid on anything but lowmem.) Just like
> >certain kernel interfaces, passing pointers to memory of different types
> >from the one intended by the interface produces invalid results, and
> >that seems to be what's happening here.
> >
> >Secondly, it's a scatterlist, and scatterlists can be passed to DMA
> >mapping operations, which also implies that _if_ a host driver decides
> >to use DMA on it, the buffer better be DMA-able.
> >
> >Thirdly, while PIO may work (or even appear to work) because _maybe_
> >converting a vmalloc address to a ficticious struct page + offset, and
> >then converting that back again _might_ result in hitting the correct
> >memory, but it's not guaranteed to.
> >
>
> [1]:
> If no DMA involved, the host drivers usually use memcpy or readl/writel
> to transfer the data between MMIO address and buffer coming from the
> caller. So, is it also not guaranteed when using memcpy or readl to
> transfer data between MMIO address and vmalloc/heap buffer?
The point here is, if buf is a vmalloc address:
v = kmap_atomic(virt_to_page(buf))
v may be _anything_ at all. The kmap_atomic() will be done by the MMC
host driver, the virt_to_page() is done by the code I quoted above.
v may be a lowmem page address. v may be somewhere in userspace. v may
be some device mapping. v may be (if you're lucky) the same address as
"buf". There's no guarantees what it will be.
Note that the scatterlist above does _not_ store the virtual address
that was passed into it, but only the struct page, offset and length.
So, drivers can not know what the original virtual address was.
> >I suspect that virt_to_page() + kmap_atomic() is likely to try to
> >dereference a struct page pointer that does not point at a legal entry
> >in the memmap arrays, and result in scribbling over some random part
> >of kernel memory.
>
> If that is the fact, so what I am concerned mostly is that by
> seraching the APIs, sdio_writeb and sdio_readb, under the drivers/net
> /wireless/, I could see almost all sdio based WLAN drivers passed in
> a vmalloc area(actually when built as moudle, it should be located in
> MODULE range which also be included as vmalloc area, no?) or heap
> buffer.
sdio_readb() and sdio_writeb() convey the data in the command stream,
not the data stream. Firstly, sdio_writeb() takes the actual value to
be written, so the memory storage is irrelevant (on many platforms, it
will be passed as a value in CPU registers.) Eventually, it's written
into the MMC command structure as the command argument, which typically
ends up being written to a host controller register.
In the case of sdio_readb(), the returned value comes from the command
status, which is typically read from registers in the host controller.
mmc_io_rw_direct_host() writes the value to the passed address. Hence,
the host controller, again, never sees the address.
> I assume my question[1] above is fine, then thanks to none of the mmc
> host drivers use DMA for sdio_writeb and sdio_readb since it only
> request one byte which didn't be fetched from host FIFO and the host
> controller HW didn't support this kind of request to use DMA(but may be
> not in the future). Otherwise, it may result in scribbling over some
> random part of kernel memory.
See above, your understanding of how sdio_readb() and sdio_writeb() is
not correct. These are single value transfer functions, using
mmc_io_rw_direct() as the underlying method of access, and do not
transfer anything over the data lines.
These functions are quite different from sdio_memcpy_toio(),
sdio_memcpy_fromio(), sdio_readsb() and sdio_writesb(), which are
multiple value transfer functions, and these perform the transfer using
the data lines. These use sdio_io_rw_ext_helper(), which then uses
mmc_io_rw_extended() to perform the transfer. The code I quoted in my
original email is from mmc_io_rw_extended().
The command path is entirely separate from the data path in most MMC
host controllers. The command path is commonly PIO, the data path is
commonly DMA, but host controllers _may_ choose PIO for small transfers
or when they have no DMA support.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH] mmc: core: make block layer non-optional
From: Johannes Thumshirn @ 2017-02-15 10:00 UTC (permalink / raw)
To: Linus Walleij, linux-mmc, Ulf Hansson, Adrian Hunter,
Paolo Valente
Cc: linux-block, Jens Axboe, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170215095353.23888-1-linus.walleij@linaro.org>
Hi Linus,
some nits below:
On 02/15/2017 10:53 AM, Linus Walleij wrote:
> I do not know at what point people have wanted to have a system
> with MMC/SD support without the block layer. We are anyway now
> so tightly integrated with the block layer that this is onlt
only ^
> teoretical and it makes no sense to have the block layer interface
^ theoretical
> as optional.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Byte,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox