Re: [PATCH 0/2] scsi: core: regression fixes for request batching
On 07/08/19 16:49, Steffen Maier wrote: > Hi James, Martin, Paolo, Ming, > > multipathing with linux-next is broken since 20190723 in our CI. > The patches fix a memleak and a severe dh/multipath functional regression. > It would be nice if we could get them to 5.4/scsi-queue and also next. > > I would have preferred if such a new feature had used its own > new copy scsi_mq_ops_batching instead of changing the use case and > semantics of the existing scsi_mq_ops, because this would likely > cause less regressions for all the other users not using the new feature. > > Steffen Maier (2): > scsi: core: fix missing .cleanup_rq for SCSI hosts without request > batching > scsi: core: fix dh and multipathing for SCSI hosts without request > batching > > drivers/scsi/scsi_lib.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Reviewed-by: Paolo Bonzini
Re: SCSI batching since next-20190723 seems to fail multipath table creation?
On 25/07/19 19:02, Steffen Maier wrote:
>
> 9e5470fe2d61 ("scsi: virtio_scsi: implement request batching")
> 8930a6c20791 ("scsi: core: add support for request batching")
>
> REBOOT into kernel with above commits reverted and now multipath
> assembly works again:
Can you try applying only this from these two commits:
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 601b9f1de267..eb4e67d02bfe 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1673,10 +1673,11 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx
*hctx,
blk_mq_start_request(req);
}
+ cmd->flags &= SCMD_PRESERVED_FLAGS;
if (sdev->simple_tags)
cmd->flags |= SCMD_TAGGED;
- else
- cmd->flags &= ~SCMD_TAGGED;
+ if (bd->last)
+ cmd->flags |= SCMD_LAST;
scsi_init_cmd_errh(cmd);
cmd->scsi_done = scsi_mq_done;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..91bd749a02f7 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -57,6 +57,7 @@ struct scsi_pointer {
#define SCMD_TAGGED(1 << 0)
#define SCMD_UNCHECKED_ISA_DMA (1 << 1)
#define SCMD_INITIALIZED (1 << 2)
+#define SCMD_LAST (1 << 3)
/* flags preserved across unprep / reprep */
#define SCMD_PRESERVED_FLAGS (SCMD_UNCHECKED_ISA_DMA | SCMD_INITIALIZED)
? These should be the only changes visible to the zfcp driver.
Thanks,
Paolo
Re: [PATCH] scsi: virtio_scsi: don't send sc payload with tmfs
On 27/02/19 17:10, Felipe Franciosi wrote:
> The virtio scsi spec defines struct virtio_scsi_ctrl_tmf as a set of
> device-readable records and a single device-writable response entry:
>
> struct virtio_scsi_ctrl_tmf
> {
> // Device-readable part
> le32 type;
> le32 subtype;
> u8 lun[8];
> le64 id;
> // Device-writable part
> u8 response;
> }
>
> The above should be organised as two descriptor entries (or potentially
> more if using VIRTIO_F_ANY_LAYOUT), but without any extra data after
> "le64 id" or after "u8 response".
>
> The Linux driver doesn't respect that, with virtscsi_abort() and
> virtscsi_device_reset() setting cmd->sc before calling virtscsi_tmf().
> It results in the original scsi command payload (or writable buffers)
> added to the tmf.
>
> This fixes the problem by leaving cmd->sc zeroed out, which makes
> virtscsi_kick_cmd() add the tmf to the control vq without any payload.
Indeed, all that the completion routine needs is cmd->comp, which is set
directly in virtscsi_tmf.
Reviewed-by: Paolo Bonzini
Cc: sta...@vger.kernel.org
>
> Signed-off-by: Felipe Franciosi
> ---
> drivers/scsi/virtio_scsi.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 772b976..464cba5 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -594,7 +594,6 @@ static int virtscsi_device_reset(struct scsi_cmnd *sc)
> return FAILED;
>
> memset(cmd, 0, sizeof(*cmd));
> - cmd->sc = sc;
> cmd->req.tmf = (struct virtio_scsi_ctrl_tmf_req){
> .type = VIRTIO_SCSI_T_TMF,
> .subtype = cpu_to_virtio32(vscsi->vdev,
> @@ -653,7 +652,6 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
> return FAILED;
>
> memset(cmd, 0, sizeof(*cmd));
> - cmd->sc = sc;
> cmd->req.tmf = (struct virtio_scsi_ctrl_tmf_req){
> .type = VIRTIO_SCSI_T_TMF,
> .subtype = VIRTIO_SCSI_T_TMF_ABORT_TASK,
>
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 16/11/18 19:17, Bart Van Assche wrote: > On Fri, 2018-11-16 at 12:43 -0500, Theodore Y. Ts'o wrote: >> I'd argue that a purpose-built eBPF access control facility is >> superior to the security_file_ioctl() LSM hook because it can make >> available to the authorization function access to the cached results >> of the SCSI INQUIRY command, and it avoids needing to duplicate >> knowledge of how to parse the parameters of the SG_IO ioctl in the LSM >> module as well as in the SCSI stack. > > If an eBPF program would decide which SG_IO commands will be executed > and which ones not, does that mean that a SCSI parser would have to be > implemented in eBPF? If so, does that mean that both the eBPF and the > LSM approach share the disadvantage of requiring to do SCSI CDB parsing > outside the SCSI core? The LSM approach cannot do SCSI CDB parsing, unless you add a special SCSI-specific hook called after parsing the SG_IO argument, due to race conditions. I'd rather not do that, however it would have that disadvantage indeed. The eBPF approach pushes the policy and the parsing entirely to userspace, so I'm not sure you can say it's a disadvantage. It's just a different design. If you use SG_IO you're already in for writing userspace code that handles CDBs, sense data etc. Paolo
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 16/11/18 10:32, Christoph Hellwig wrote:
> On Mon, Nov 12, 2018 at 11:17:29AM +0100, Paolo Bonzini wrote:
>>> Well, that's what we have the security_file_ioctl() LSM hook for so that
>>> your security model can arbitrate access to ioctls.
>>
>> Doesn't that have TOC-TOU races by design?
>
> If you want to look at the command - yes. If you just want to filter
> read vs write vs ioctl, no.
Yeah, but looking at the command is what Ted wants. The thing that we
did in RHEL was a single sysfs bool that allows unfiltered access,
because it was sort of enough and made the delta very small. But for
upstream I want to do it right, even if that means learning all that
new-fangled BPF stuff. :)
>> Also, what about SG_IO giving write access to files that are only opened
>> read-only (and only have read permissions)?
>
> Allowing SG_IO on read-only permissions sounds like a reall bad idea,
> filtering or not.
I would even agree, however it's allowed right now and I would be
surprised if no one was relying on it in good faith ("I'm just doing an
INQUIRY, why do I need to open O_RDWR"). And indeed:
$ sudo chmod a+r /dev/sda
$ strace -e openat sg_inq /dev/sda
openat(AT_FDCWD, "/dev/sda", O_RDONLY|O_NONBLOCK) = 3
So it would be a regression.
Paolo
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 16/11/18 01:37, Bart Van Assche wrote: > All user space interfaces in the Linux kernel for storage that I'm familiar > with not only allow configuration of parameters but also make it easy to > query which parameters have been configured. The existing sysfs and configfs > interfaces demonstrate this. Using BPF to configure SG/IO access has a > significant disadvantage, namely that it is very hard to figure out what has > been configured. Figuring out what has been configured namely requires > disassembling BPF. I'm not sure anyone will be enthusiast about this. Well, that's a problem with BPF in general. With great power comes great obscurability. Paolo
Re: [PATCH 3/3] block: add back command filter modification via sysfs
On 16/11/18 06:46, Bart Van Assche wrote: > I do not know any application for which it would be useful to allow some but > not all of these commands. With the proposed interface however users will > have to examine all SCSI opcodes and for each opcode they will have to decide > whether or not it should be allowed. Additionally, for opcodes like 7fh that > represent multiple commands, users will have to decide whether they want to > allow all these commands or none. That's why I think that filtering SCSI > commands based on their CDB is an unfortunate choice. Would it be sufficient > for the use cases you are looking at to group SCSI commands as follows and to > enable/disable these commands per group: > * SCSI command that read information from the medium (e.g. READ) or from the > controller (e.g. READ CAPACITY). > * SCSI commands that modify information on the medium (e.g. WRITE). > * SCSI commands that modify controller settings (e.g. MODE SELECT or SET > TARGET PORT GROUPS). And also: * all SCSI commands (e.g. write microcode, vendor specific commands). It would. However, it would be impossible to do this without making the filter depend on the SCSI device type. This has been rejected in 2012. Paolo
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 11/11/2018 15:14, Theodore Y. Ts'o wrote: > On Sun, Nov 11, 2018 at 02:26:45PM +0100, Paolo Bonzini wrote: >> >> I'm not very eBPF savvy, the question I have is: what kind of >> information about the running process is available in an eBPF program? >> For example, even considering only the examples you make, would it be >> able to access the CDB, the capabilities and uid/gid of the task, the >> SCSI device type, the WWN? Of course you also need the mode of the file >> descriptor in order to allow SANITIZE ERASE if the disk is opened for write. > > The basic uid/gid of the task is certainly available. For storage > stack specific things, it's a matter of what we make available to the > eBPF function. For example, there is an experimental netfilter > replacement which uses eBPF; obviously that requires making the packet > which is being inspecting so it can be given a thumbs up or thumbs > down result. That's going to require letting the eBPF function to > have access to the network header, access to the connection tracking > tables, etc. Yeah, and there are even already helpers such as bpf_get_current_uid_gid. So that part can be done in a sort-of generic way. I can try and do the work, but I'd like some agreement on the design first... For example a more important question is how would the BPF filter be attached? Two possibilities that come to mind are: - add it to the /dev/sg* or /dev/sd* struct file(*) via a ioctl, and use pass the file descriptor to the unprivileged QEMU after setting up the BPF filter, via either fork() or SCM_RIGHTS. This would be a very nice model for privilege separation, but I'm afraid it would not work for your use case - add BPF programs to cgroups, in the form of a new BPF_PROG_TYPE_CGROUP_CDB_FILTER or something like that. That would also work for my usecase, and it seems to be in line with how the network guys are doing things. So it would seem like the way to go. Some other details... Registering the first cgroup-based filter would disable the default filter; if multiple filters are attached, the outcomes of all filters would be AND-ed, also similarly to how socket and sockaddr cgroup BPF works. Finally, filters would be applied also to processes with CAP_SYS_RAWIO, unlike the current filter. Needless to say, this would not add special case code, but it would still add a substantial amount of code, probably comparable to this series. Christoph? Paolo (*) that's not immediate for /dev/sd*, because it would require using the block device file's private_data; that's not possible yet via struct block_device_operations, but as far as I can tell block devices themselves do not need the private_data, so it is at least doable.
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 12/11/2018 09:20, Christoph Hellwig wrote: > On Sun, Nov 11, 2018 at 08:42:42AM -0500, Theodore Y. Ts'o wrote: >> It really depends on the security model being used on a particular >> system. I can easily imagine scenarios where userspace is allowed >> full access to the device with respect to read/write/open, but the >> security model doesn't want to allow access to various SCSI commands >> such as firmware upload commands, TCG commads, the >> soon-to-be-standardized Zone Activation Commands (which allow dynamic >> conversion of HDD recording modes between CMR and SMR), etc. > > Well, that's what we have the security_file_ioctl() LSM hook for so that > your security model can arbitrate access to ioctls. Doesn't that have TOC-TOU races by design? Also, what about SG_IO giving write access to files that are only opened read-only (and only have read permissions)? Paolo
Re: [PATCH 0/3] SG_IO command filtering via sysfs
On 10/11/2018 20:05, Theodore Y. Ts'o wrote: > I wonder if a better way of adding SG_IO command filtering is via > eBPF? We are currently carrying a inside Google a patch which allows > a specific of SCSI commands to non-root processes --- if the process > belonged to a particular Unix group id. > > It's pretty specific to our use case, in terms of the specific SCSI > commands we want to allow through. I can imagine people wanting > different filters based on the type of the SCSI device, or a HDD's > WWID, not just a group id. For example, this might be useful for > people wanting to do crazy things with containers --- maybe you'd > want to allow container root to send a SANITIZE ERASE command to one > of its exclusively assigned disks, but not to other HDD's. > > So having something that's more general than a flat file in sysfs > might be preferable to resurrecting an interface which we would then > after to support forever, even if we come up with a more general > interface. Heh, this was exactly the answer I dreaded, because I can't deny it makes sense. :) My main argument against it is that while superseding an interface and still having to support it forever sucks, having a super-complex interface is also bad (back in 2012 I wrote https://lwn.net/Articles/501742/ which I'm not particularly enthusiastic about). In many cases a combination of MAC policies, ACLs, etc. can be just as effective. I'm not very eBPF savvy, the question I have is: what kind of information about the running process is available in an eBPF program? For example, even considering only the examples you make, would it be able to access the CDB, the capabilities and uid/gid of the task, the SCSI device type, the WWN? Of course you also need the mode of the file descriptor in order to allow SANITIZE ERASE if the disk is opened for write. Paolo
[PATCH 1/3] block: add back queue-private command filter
The command filter used to be mutable via sysfs, but this was broken
and backed out. Let's add it back. This patch adds the infrastructure
for filtering, but unlike the old code this one just adds a pointer to
request_queue, so as to make it cheaper in the majority of cases where
no special filtering is desired.
This is a revert of commit 018e044 ("block: get rid of queue-private
command filter", 2009-06-26), though with many changes.
Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke
Cc: Martin K. Petersen
Cc: James Bottomley
Signed-off-by: Paolo Bonzini
---
block/blk-sysfs.c | 2 ++
block/bsg-lib.c| 4 ++--
block/bsg.c| 8
block/scsi_ioctl.c | 19 +--
drivers/scsi/sg.c | 5 +++--
include/linux/blkdev.h | 9 -
include/linux/bsg.h| 4 ++--
7 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 3772671cf2bc..d1ec150a3478 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -838,6 +838,8 @@ static void __blk_release_queue(struct work_struct *work)
blk_exit_rl(q, &q->root_rl);
+ kfree(q->cmd_filter);
+
if (q->queue_tags)
__blk_queue_free_tags(q);
diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index f3501cdaf1a6..23200d8b035d 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -41,8 +41,8 @@ static int bsg_transport_check_proto(struct sg_io_v4 *hdr)
return 0;
}
-static int bsg_transport_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
- fmode_t mode)
+static int bsg_transport_fill_hdr(struct request_queue *q, struct request *rq,
+ struct sg_io_v4 *hdr, fmode_t mode)
{
struct bsg_job *job = blk_mq_rq_to_pdu(rq);
diff --git a/block/bsg.c b/block/bsg.c
index 9a442c23a715..ec8cbf3bf734 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -69,8 +69,8 @@ static int bsg_scsi_check_proto(struct sg_io_v4 *hdr)
return 0;
}
-static int bsg_scsi_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
- fmode_t mode)
+static int bsg_scsi_fill_hdr(struct request_queue *q, struct request *rq,
+struct sg_io_v4 *hdr, fmode_t mode)
{
struct scsi_request *sreq = scsi_req(rq);
@@ -83,7 +83,7 @@ static int bsg_scsi_fill_hdr(struct request *rq, struct
sg_io_v4 *hdr,
if (copy_from_user(sreq->cmd, uptr64(hdr->request), sreq->cmd_len))
return -EFAULT;
- if (blk_verify_command(sreq->cmd, mode))
+ if (blk_verify_command(q, sreq->cmd, mode))
return -EPERM;
return 0;
}
@@ -159,7 +159,7 @@ static void bsg_scsi_free_rq(struct request *rq)
if (IS_ERR(rq))
return rq;
- ret = q->bsg_dev.ops->fill_hdr(rq, hdr, mode);
+ ret = q->bsg_dev.ops->fill_hdr(q, rq, hdr, mode);
if (ret)
goto out;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 533f4aee8567..5d577c89f9e6 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -34,11 +34,6 @@
#include
#include
-struct blk_cmd_filter {
- unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
- unsigned long write_ok[BLK_SCSI_CMD_PER_LONG];
-};
-
static struct blk_cmd_filter blk_default_cmd_filter;
/* Command group 3 is reserved and should never be used. */
@@ -207,14 +202,18 @@ static void blk_set_cmd_filter_defaults(struct
blk_cmd_filter *filter)
__set_bit(GPCMD_SET_READ_AHEAD, filter->write_ok);
}
-int blk_verify_command(unsigned char *cmd, fmode_t mode)
+int blk_verify_command(struct request_queue *q, unsigned char *cmd,
+ fmode_t mode)
{
- struct blk_cmd_filter *filter = &blk_default_cmd_filter;
-
+ struct blk_cmd_filter *filter = q->cmd_filter;
+
/* root can do any command. */
if (capable(CAP_SYS_RAWIO))
return 0;
+ if (!filter)
+ filter = &blk_default_cmd_filter;
+
/* Anybody who can open the device can do a read-safe command */
if (test_bit(cmd[0], filter->read_ok))
return 0;
@@ -234,7 +233,7 @@ static int blk_fill_sghdr_rq(struct request_queue *q,
struct request *rq,
if (copy_from_user(req->cmd, hdr->cmdp, hdr->cmd_len))
return -EFAULT;
- if (blk_verify_command(req->cmd, mode))
+ if (blk_verify_command(q, req->cmd, mode))
return -EPERM;
/*
@@ -468,7 +467,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk
*disk, fmode_t mode,
if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))
goto error;
- err = blk_verify_command(req->cmd, mode);
+ err = blk_verify_command(q, req->cmd, mode);
if (err)
goto error;
diff --git a/drivers/scsi/sg.c b/drivers/scsi
[PATCH 0/3] SG_IO command filtering via sysfs
Currently, SG_IO ioctls are implemented so that non-CAP_SYS_RAWIO users
can send commands from a predetermined whitelist. The whitelist is very
simple-minded though, and basically corresponds to MMC commands---the idea
being that it would be nice for local users to read/copy/burn CDs.
This was probably sensible when the whitelist was first added (in the pre-git
era), but quite a few things have changed since then:
- there is a lot more focus on not running things as root unnecessarily;
it is generally much more common to have non-root processes accessing disks
and we would like that to happen more, not less.
- there is also a lot more focus on not giving capabilities unnecessarily.
Using CAP_SYS_RAWIO, which gives full access to all commands, allows
you to send a WRITE SCSI command to a file opened for reading, which is
a nice recipe for data corruption. A more fine-grained whitelist allows
you to give the desired access to the application.
- we've discovered that some commands conflict between the various
SCSI standards. UNMAP (a write command) in SBC has the same number as
the obscure MMC command READ SUBCHANNEL. As such it's allowed if a
block device is opened for reading!
This series, which was last sent in 2012 before I lost interest in the
endless discussions that followed, adds the possibility to make the filter
mutable via sysfs, so that it can be set up per device. This of course can
go both ways; interested applications can set a wider filter, but one can
also imagine setting much more restrictive filters by default (possibly
allowing little more than INQUIRY, TEST UNIT READY, READ CAPACITY and the
like).
Back then there was opposition to giving unfettered access to "dangerous"
or "too easily destructive" commands such as WRITE SAME or PERSISTENT
RESERVE OUT to unprivileged users. Even then, I think this objection
is now moot thanks to the following things that have happened in 2012:
- WRITE SAME commands, which were considered too destructive, have
been added to the filter since commit 25cdb6451064 ("block: allow
WRITE_SAME commands with the SG_IO ioctl", 2016-12-15, Linux 4.10).
They are basically the only non-MMC commands included in the filter,
by the way.
- persistent reservations are also allowed now via PR ioctls (commit
924d55b06347, "sd: implement the Persistent Reservation API", 2015-10-21,
Linux 4.4). These require CAP_SYS_ADMIN, which is the same capability
that is needed to *grant* access to PR commands via the SG_IO filter.
So, here is the 2018 version of these patches. Please review! :)
Paolo
Paolo Bonzini (3):
block: add back queue-private command filter
scsi: create an all-one filter for scanners
block: add back command filter modification via sysfs
Documentation/block/queue-sysfs.txt | 19 +
block/Kconfig | 10 +++
block/blk-sysfs.c | 43
block/bsg-lib.c | 4 +-
block/bsg.c | 8 +--
block/scsi_ioctl.c | 136 +---
drivers/scsi/scsi_scan.c| 13
drivers/scsi/sg.c | 6 +-
include/linux/blkdev.h | 18 -
include/linux/bsg.h | 4 +-
10 files changed, 238 insertions(+), 23 deletions(-)
--
1.8.3.1
[PATCH 3/3] block: add back command filter modification via sysfs
This adds two new sysfs attributes to the queue kobject. The attributes
allow reading and writing the whitelist of unprivileged commands.
This is again a bit different from what was removed in commit 018e044
(block: get rid of queue-private command filter, 2009-06-26), but the idea
is the same. One difference is that it does not use a separate kobject.
Also, the supported sysfs syntax is a bit more expressive: it includes
ranges, the ability to replace all of the filter with a single command,
and does not force usage of hexadecimal.
Since the names are different, and the old ones were anyway never really
enabled, the different userspace API is not a problem.
Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke
Cc: Martin K. Petersen
Cc: James Bottomley
Suggested-by: James Bottomley
Signed-off-by: Paolo Bonzini
---
Documentation/block/queue-sysfs.txt | 19 ++
block/Kconfig | 10 +++
block/blk-sysfs.c | 41 +
block/scsi_ioctl.c | 117
include/linux/blkdev.h | 9 +++
5 files changed, 196 insertions(+)
diff --git a/Documentation/block/queue-sysfs.txt
b/Documentation/block/queue-sysfs.txt
index 2c1e67058fd3..1fe5fe5fd80a 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -162,6 +162,25 @@ control of this block device to that new IO scheduler.
Note that writing
an IO scheduler name to this file will attempt to load that IO scheduler
module, if it isn't already present in the system.
+sgio_read_filter (RW)
+-
+When read, this file will display a list of SCSI commands (i.e. values of
+the first byte of a CDB) that are always available for unprivileged users
+via /dev/bsg, /dev/sgNN, or ioctls such as SG_IO and CDROM_SEND_PACKET.
+When written, the list of commands will be modified. The default filter
+can be restored by writing "default" to the file; otherwise the input should
+be a list of byte values or ranges such as "0x00-0xff". In the latter case,
+instead of replacing the filter completely you can add to the commands,
+by writing a string that begins with '+', or remove them by writing a
+string that begins with '-'.
+
+sgio_write_filter (RW)
+--
+When read, this file will display a list of SCSI commands (i.e. values of
+the first byte of a CDB) that are available for unprivileged users
+when the block device is open for writing. Writing to this file behaves
+as for sgio_read_filter.
+
write_cache (RW)
When read, this file will display whether the device has write back
diff --git a/block/Kconfig b/block/Kconfig
index 1f2469a0123c..a5bc37d50e07 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -71,6 +71,16 @@ config BLK_DEV_BSG
access device serial numbers, etc.
If unsure, say Y.
+
+config BLK_DEV_SG_FILTER_SYSFS
+ bool "Customizable SG_IO filters in sysfs"
+ default y
+ help
+ Saying Y here will let you use sysfs to customize the list
+ of SCSI commands that are available (via /dev/sg, /dev/bsg or
+ ioctls such as SG_IO) to unprivileged users.
+
+ If unsure, say Y.
config BLK_DEV_BSGLIB
bool "Block layer SG support v4 helper lib"
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index d1ec150a3478..ea2a47272bcf 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -703,6 +703,43 @@ static ssize_t queue_dax_show(struct request_queue *q,
char *page)
};
#endif
+#ifdef CONFIG_BLK_DEV_SG_FILTER_SYSFS
+static ssize_t queue_sgio_filter_read_show(struct request_queue *q, char *page)
+{
+ return blk_filter_show(q, page, READ);
+}
+
+static ssize_t queue_sgio_filter_write_show(struct request_queue *q,
+ char *page)
+{
+ return blk_filter_show(q, page, WRITE);
+}
+
+static ssize_t queue_sgio_filter_read_store(struct request_queue *q,
+ const char *page, size_t count)
+{
+ return blk_filter_store(q, page, count, READ);
+}
+
+static ssize_t queue_sgio_filter_write_store(struct request_queue *q,
+const char *page, size_t count)
+{
+ return blk_filter_store(q, page, count, WRITE);
+}
+
+static struct queue_sysfs_entry queue_sgio_filter_read_entry = {
+ .attr = { .name = "sgio_filter_read", .mode = S_IRUGO | S_IWUSR },
+ .show = queue_sgio_filter_read_show,
+ .store = queue_sgio_filter_read_store,
+};
+
+static struct queue_sysfs_entry queue_sgio_filter_write_entry = {
+ .attr = {.name = "sgio_filter_write", .mode = S_IRUGO | S_IWUSR },
+ .show = queue_sgio_filter_write_show,
+ .store = queue_sgio_filter_write_store,
+};
+#endif
+
static struct attribute *default_attrs[] = {
&queue_requests_entry.attr
[PATCH 2/3] scsi: create an all-one filter for scanners
Any command is allowed for scanners when /dev/sg is used.
Reimplement this using customizable command filters, so that the
sysfs knobs will work in this case, too.
Cc: linux-scsi@vger.kernel.org
Cc: Hannes Reinecke
Cc: Martin K. Petersen
Cc: James Bottomley
Signed-off-by: Paolo Bonzini
---
drivers/scsi/scsi_scan.c | 13 +
drivers/scsi/sg.c| 3 ---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 78ca63dfba4a..ceb7f5535f44 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -844,6 +844,19 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned
char *inq_result,
*bflags |= BLIST_NOREPORTLUN;
}
+ if (sdev->type == TYPE_SCANNER) {
+ sdev->request_queue->cmd_filter =
+ kzalloc(sizeof(struct blk_cmd_filter), GFP_KERNEL);
+
+ if (!sdev->request_queue->cmd_filter)
+ return SCSI_SCAN_NO_RESPONSE;
+
+ memset(sdev->request_queue->cmd_filter->read_ok, 0xFF,
+ sizeof(sdev->request_queue->cmd_filter->read_ok));
+ memset(sdev->request_queue->cmd_filter->write_ok, 0xFF,
+ sizeof(sdev->request_queue->cmd_filter->write_ok));
+ }
+
/*
* For a peripheral qualifier (PQ) value of 1 (001b), the SCSI
* spec says: The device server is capable of supporting the
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 1b04016d3bb8..e04acf41f283 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -242,9 +242,6 @@ static int sg_allow_access(struct file *filp, unsigned char
*cmd)
struct sg_fd *sfp = filp->private_data;
struct scsi_device *device = sfp->parentdp->device;
- if (device->type == TYPE_SCANNER)
- return 0;
-
return blk_verify_command(device->request_queue, cmd, filp->f_mode);
}
--
1.8.3.1
Re: [PATCH V2 6/8] scsi: virtio_scsi: fix IO hang by irq vector automatic affinity
On 05/02/2018 16:20, Ming Lei wrote:
> Now 84676c1f21e8ff5(genirq/affinity: assign vectors to all possible CPUs)
> has been merged to V4.16-rc, and it is easy to allocate all offline CPUs
> for some irq vectors, this can't be avoided even though the allocation
> is improved.
>
> For example, on a 8cores VM, 4~7 are not-present/offline, 4 queues of
> virtio-scsi, the irq affinity assigned can become the following shape:
>
> irq 36, cpu list 0-7
> irq 37, cpu list 0-7
> irq 38, cpu list 0-7
> irq 39, cpu list 0-1
> irq 40, cpu list 4,6
> irq 41, cpu list 2-3
> irq 42, cpu list 5,7
>
> Then IO hang is triggered in case of non-SCSI_MQ.
>
> Given storage IO is always C/S model, there isn't such issue with
> SCSI_MQ(blk-mq),
> because no IO can be submitted to one hw queue if the hw queue hasn't online
> CPUs.
>
> Fix this issue by forcing to use blk_mq.
>
> BTW, I have been used virtio-scsi(scsi_mq) for several years, and it has
> been quite stable, so it shouldn't cause extra risk.
I think that's ok now that we have I/O schedulers for blk-mq.
Acked-by: Paolo Bonzini
Paolo
> Cc: Arun Easi
> Cc: Omar Sandoval ,
> Cc: "Martin K. Petersen" ,
> Cc: James Bottomley ,
> Cc: Christoph Hellwig ,
> Cc: Don Brace
> Cc: Kashyap Desai
> Cc: Peter Rivera
> Cc: Paolo Bonzini
> Cc: Mike Snitzer
> Reviewed-by: Hannes Reinecke
> Tested-by: Laurence Oberman
> Signed-off-by: Ming Lei
> ---
> drivers/scsi/virtio_scsi.c | 59
> +++---
> 1 file changed, 3 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 7c28e8d4955a..54e3a0f6844c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -91,9 +91,6 @@ struct virtio_scsi_vq {
> struct virtio_scsi_target_state {
> seqcount_t tgt_seq;
>
> - /* Count of outstanding requests. */
> - atomic_t reqs;
> -
> /* Currently active virtqueue for requests sent to this target. */
> struct virtio_scsi_vq *req_vq;
> };
> @@ -152,8 +149,6 @@ static void virtscsi_complete_cmd(struct virtio_scsi
> *vscsi, void *buf)
> struct virtio_scsi_cmd *cmd = buf;
> struct scsi_cmnd *sc = cmd->sc;
> struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
> - struct virtio_scsi_target_state *tgt =
> - scsi_target(sc->device)->hostdata;
>
> dev_dbg(&sc->device->sdev_gendev,
> "cmd %p response %u status %#02x sense_len %u\n",
> @@ -210,8 +205,6 @@ static void virtscsi_complete_cmd(struct virtio_scsi
> *vscsi, void *buf)
> }
>
> sc->scsi_done(sc);
> -
> - atomic_dec(&tgt->reqs);
> }
>
> static void virtscsi_vq_done(struct virtio_scsi *vscsi,
> @@ -580,10 +573,7 @@ static int virtscsi_queuecommand_single(struct Scsi_Host
> *sh,
> struct scsi_cmnd *sc)
> {
> struct virtio_scsi *vscsi = shost_priv(sh);
> - struct virtio_scsi_target_state *tgt =
> - scsi_target(sc->device)->hostdata;
>
> - atomic_inc(&tgt->reqs);
> return virtscsi_queuecommand(vscsi, &vscsi->req_vqs[0], sc);
> }
>
> @@ -596,55 +586,11 @@ static struct virtio_scsi_vq
> *virtscsi_pick_vq_mq(struct virtio_scsi *vscsi,
> return &vscsi->req_vqs[hwq];
> }
>
> -static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
> -struct virtio_scsi_target_state
> *tgt)
> -{
> - struct virtio_scsi_vq *vq;
> - unsigned long flags;
> - u32 queue_num;
> -
> - local_irq_save(flags);
> - if (atomic_inc_return(&tgt->reqs) > 1) {
> - unsigned long seq;
> -
> - do {
> - seq = read_seqcount_begin(&tgt->tgt_seq);
> - vq = tgt->req_vq;
> - } while (read_seqcount_retry(&tgt->tgt_seq, seq));
> - } else {
> - /* no writes can be concurrent because of atomic_t */
> - write_seqcount_begin(&tgt->tgt_seq);
> -
> - /* keep previous req_vq if a reader just arrived */
> - if (unlikely(atomic_read(&tgt->reqs) > 1)) {
> - vq = tgt->req_vq;
> - goto unlock;
> - }
> -
> - queue_num = smp_processor_id();
> - while (unlikely(queue_num >= vscsi->num_queues))
> -
Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
On 11/08/2017 19:23, Michael S. Tsirkin wrote: > On Fri, Aug 11, 2017 at 04:09:26PM +0200, Paolo Bonzini wrote: >> On 10/08/2017 23:41, Michael S. Tsirkin wrote: >>>>> Then we probably should fail probe if vq size is too small. >>>> What does this mean? >>> >>> We must prevent driver from submitting s/g lists > vq size to device. >> >> What is the rationale for the limit? > > So the host knows what it needs to support. > >> both virtio-blk and virtio-scsi transmit their own value for the >> maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk). > > No other device has it, and it seemed like a good idea to > limit it generally at the time. > > we can fix the spec to relax the requirement for blk and scsi - > want to submit a proposal? Alternatively, add a generic field > for that. Yes, I can submit a proposal. blk and scsi are the ones that are most likely to have very long sg lists. When I was designing scsi I just copied that field from blk. :) Paolo
Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
On 10/08/2017 23:41, Michael S. Tsirkin wrote: >>> Then we probably should fail probe if vq size is too small. >> What does this mean? > > We must prevent driver from submitting s/g lists > vq size to device. What is the rationale for the limit? It makes no sense if indirect descriptors are available, especially because... > Either tell linux to avoid s/g lists that are too long, or > simply fail request if this happens, or refuse to attach driver to device. > > Later option would look something like this within probe: > > for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++) > if (vqs[i]->num < MAX_SG_USED_BY_LINUX) > goto err; > > > I don't know what's MAX_SG_USED_BY_LINUX though. > ... both virtio-blk and virtio-scsi transmit their own value for the maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk). Paolo
Re: [PATCH v2 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.
On 10/08/2017 18:56, Richard W.M. Jones wrote:
> Since switching to blk-mq as the default in commit 5c279bd9e406
> ("scsi: default to scsi-mq"), virtio-scsi LUNs consume about 10x as
> much kernel memory.
>
> qemu currently allocates a fixed 128 entry virtqueue. can_queue
> currently is set to 1024. But with indirect descriptors, each command
> in the queue takes 1 virtqueue entry, so the number of commands which
> can be queued is equal to the length of the virtqueue.
>
> Note I intend to send a patch to qemu to allow the virtqueue size to
> be configured from the qemu command line.
>
> Thanks Paolo Bonzini, Christoph Hellwig.
>
> Signed-off-by: Richard W.M. Jones
> ---
> drivers/scsi/virtio_scsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 9be211d68b15..7c28e8d4955a 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -818,7 +818,6 @@ static struct scsi_host_template
> virtscsi_host_template_single = {
> .eh_timed_out = virtscsi_eh_timed_out,
> .slave_alloc = virtscsi_device_alloc,
>
> - .can_queue = 1024,
> .dma_boundary = UINT_MAX,
> .use_clustering = ENABLE_CLUSTERING,
> .target_alloc = virtscsi_target_alloc,
> @@ -839,7 +838,6 @@ static struct scsi_host_template
> virtscsi_host_template_multi = {
> .eh_timed_out = virtscsi_eh_timed_out,
> .slave_alloc = virtscsi_device_alloc,
>
> - .can_queue = 1024,
> .dma_boundary = UINT_MAX,
> .use_clustering = ENABLE_CLUSTERING,
> .target_alloc = virtscsi_target_alloc,
> @@ -972,6 +970,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
> if (err)
> goto virtscsi_init_failed;
>
> + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
> +
> cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
> shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0x;
>
Acked-by: Paolo Bonzini
Re: [PATCH 2/2] virtio: virtio_scsi: Set can_queue to the length of the virtqueue.
On 10/08/2017 18:40, Richard W.M. Jones wrote:
> Since switching to blk-mq as the default in commit 5c279bd9e406
> ("scsi: default to scsi-mq"), virtio-scsi LUNs consume about 10x as
> much kernel memory.
>
> qemu currently allocates a fixed 128 entry virtqueue. can_queue
> currently is set to 1024. But with indirect descriptors, each command
> in the queue takes 1 virtqueue entry, so the number of commands which
> can be queued is equal to the length of the virtqueue.
>
> Note I intend to send a patch to qemu to allow the virtqueue size to
> be configured from the qemu command line.
You can clear .can_queue from the templates. Otherwise looks good.
Paolo
> Thanks Paolo Bonzini, Christoph Hellwig.
>
> Signed-off-by: Richard W.M. Jones
> ---
> drivers/scsi/virtio_scsi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 9be211d68b15..d6b4ff634c0d 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
> if (err)
> goto virtscsi_init_failed;
>
> + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
> +
> cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
> shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0x;
>
Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.
On 10/08/2017 18:40, Richard W.M. Jones wrote:
> If using indirect descriptors, you can make the total_sg as large as
> you want. If not, BUG is too serious because the function later
> returns -ENOSPC.
>
> Thanks Paolo Bonzini, Christoph Hellwig.
>
> Signed-off-by: Richard W.M. Jones
> ---
> drivers/virtio/virtio_ring.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 5e1b548828e6..27cbc1eab868 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> }
> #endif
>
> - BUG_ON(total_sg > vq->vring.num);
> BUG_ON(total_sg == 0);
>
> head = vq->free_head;
> @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>* buffers, then go indirect. FIXME: tune this threshold */
> if (vq->indirect && total_sg > 1 && vq->vq.num_free)
> desc = alloc_indirect(_vq, total_sg, gfp);
> - else
> + else {
> desc = NULL;
> + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
So we get here only if vq->vq.num_free is zero. In that case,
vq->indirect makes no difference for the appropriateness of the warning
(that is, it should never be issued for indirect descriptors).
> + }
>
> if (desc) {
> /* Use a single buffer which doesn't continue */
>
Reviewed-by: Paolo Bonzini
Paolo
Re: Increased memory usage with scsi-mq
On 10/08/2017 17:40, Richard W.M. Jones wrote:
> OK this is looking a bit better now.
>
> With scsi-mq enabled: 175 disks
> virtqueue_size=64: 318 disks *
> virtqueue_size=16: 775 disks *
> With scsi-mq disabled: 1755 disks
> * = new results
>
> I also ran the whole libguestfs test suite with virtqueue_size=16
> (with no failures shown). As this tests many different disk I/O
> operations, it gives me some confidence that things generally work.
Yes, it looks good. I'll grab you on IRC to discuss the commit message.
Paolo
> Do you have any other comments about the patches? I'm not sure I know
> enough to write an intelligent commit message for the kernel patch.
>
> Rich.
>
> --- kernel patch ---
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 9be211d68b15..d6b4ff634c0d 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
> if (err)
> goto virtscsi_init_failed;
>
> + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
> +
> cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
> shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0x;
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 5e1b548828e6..2d7509da9f39 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> }
> #endif
>
> - BUG_ON(total_sg > vq->vring.num);
> BUG_ON(total_sg == 0);
>
> head = vq->free_head;
> @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>* buffers, then go indirect. FIXME: tune this threshold */
> if (vq->indirect && total_sg > 1 && vq->vq.num_free)
> desc = alloc_indirect(_vq, total_sg, gfp);
> - else
> + else {
> desc = NULL;
> + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
> +}
>
> if (desc) {
> /* Use a single buffer which doesn't continue */
>
> --- qemu patch ---
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index eb639442d1..aadd99aad1 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -867,10 +867,10 @@ void virtio_scsi_common_realize(DeviceState *dev,
> s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
>
> -s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
> -s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
> +s->ctrl_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, ctrl);
> +s->event_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, evt);
> for (i = 0; i < s->conf.num_queues; i++) {
> -s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
> +s->cmd_vqs[i] = virtio_add_queue(vdev, s->conf.virtqueue_size, cmd);
> }
> }
>
> @@ -917,6 +917,7 @@ static void virtio_scsi_device_unrealize(DeviceState
> *dev, Error **errp)
>
> static Property virtio_scsi_properties[] = {
> DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues,
> 1),
> +DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
> parent_obj.conf.virtqueue_size, 128),
> DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI,
> parent_obj.conf.max_sectors,
>0x),
> DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI,
> parent_obj.conf.cmd_per_lun,
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index de6ae5a9f6..e30a92d3e7 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
>
> struct VirtIOSCSIConf {
> uint32_t num_queues;
> +uint32_t virtqueue_size;
> uint32_t max_sectors;
> uint32_t cmd_per_lun;
> #ifdef CONFIG_VHOST_SCSI
>
>
>
Re: Increased memory usage with scsi-mq
On 10/08/2017 16:16, Richard W.M. Jones wrote:
> On Thu, Aug 10, 2017 at 02:53:58PM +0200, Paolo Bonzini wrote:
>> can_queue and cmd_per_lun are different. can_queue should be set to the
>> value of vq->vring.num where vq is the command virtqueue (the first one
>> is okay if there's >1).
>>
>> If you want to change it, you'll have to do so in QEMU.
>
> Here are a couple more patches I came up with, the first to Linux,
> the second to qemu.
>
> There are a few problems ...
>
> (1) Setting virtqueue_size to < 128 causes a BUG_ON failure in
> virtio_ring.c:virtqueue_add at:
>
> BUG_ON(total_sg > vq->vring.num);
That bug is bogus. You can change it to
WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
and move it in the "else" here:
if (vq->indirect && total_sg > 1 && vq->vq.num_free)
desc = alloc_indirect(_vq, total_sg, gfp);
else
desc = NULL;
You just get a -ENOSPC after the WARN, so no need to crash the kernel!
> (2) Can/should the ctrl, event and cmd queue sizes be set to the same
> values? Or should ctrl & event be left at 128?
It's okay if they're all the same.
> (3) It seems like it might be a problem that virtqueue_size is not
> passed through the virtio_scsi_conf struct (which is ABI between the
> kernel and qemu AFAICT and so not easily modifiable). However I think
> this is not a problem because virtqueue size is stored in the
> Virtqueue Descriptor table, which is how the kernel gets the value.
> Is that right?
Yes.
Paolo
> Rich.
>
>
> --- kernel patch ---
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 9be211d68b15..d6b4ff634c0d 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
> if (err)
> goto virtscsi_init_failed;
>
> + shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
> +
> cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
> shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0x;
>
>
> --- qemu patch ---
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index eb639442d1..aadd99aad1 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -867,10 +867,10 @@ void virtio_scsi_common_realize(DeviceState *dev,
> s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
>
> -s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
> -s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
> +s->ctrl_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, ctrl);
> +s->event_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, evt);
> for (i = 0; i < s->conf.num_queues; i++) {
> -s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
> +s->cmd_vqs[i] = virtio_add_queue(vdev, s->conf.virtqueue_size, cmd);
> }
> }
>
> @@ -917,6 +917,7 @@ static void virtio_scsi_device_unrealize(DeviceState
> *dev, Error **errp)
>
> static Property virtio_scsi_properties[] = {
> DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues,
> 1),
> +DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
> parent_obj.conf.virtqueue_size, 128),
> DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI,
> parent_obj.conf.max_sectors,
>0x),
> DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI,
> parent_obj.conf.cmd_per_lun,
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index de6ae5a9f6..e30a92d3e7 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
>
> struct VirtIOSCSIConf {
> uint32_t num_queues;
> +uint32_t virtqueue_size;
> uint32_t max_sectors;
> uint32_t cmd_per_lun;
> #ifdef CONFIG_VHOST_SCSI
>
>
Re: Increased memory usage with scsi-mq
On 10/08/2017 14:22, Richard W.M. Jones wrote: > On Wed, Aug 09, 2017 at 06:50:10PM +0200, Paolo Bonzini wrote: >> On 09/08/2017 18:01, Christoph Hellwig wrote: >>> On Mon, Aug 07, 2017 at 03:07:48PM +0200, Paolo Bonzini wrote: >>>> can_queue should depend on the virtqueue size, which unfortunately can >>>> vary for each virtio-scsi device in theory. The virtqueue size is >>>> retrieved by drivers/virtio prior to calling vring_create_virtqueue, and >>>> in QEMU it is the second argument to virtio_add_queue. >>> >>> Why is that unfortunate? We don't even have to set can_queue in >>> the host template, we can dynamically set it on per-host basis. >> >> Ah, cool, I thought allocations based on can_queue happened already in >> scsi_host_alloc, but they happen at scsi_add_host time. > > I think I've decoded all that information into the patch below. > > I tested it, and it appears to work: when I set cmd_per_lun on the > qemu command line, I see that the guest can add more disks: > > With scsi-mq enabled: 175 disks > cmd_per_lun not set:177 disks * > cmd_per_lun=16: 776 disks * > cmd_per_lun=4: 1160 disks * > With scsi-mq disabled: 1755 disks > * = new result > > From my point of view, this is a good result, but you should be warned > that I don't fully understand what's going on here and I may have made > obvious or not-so-obvious mistakes. can_queue and cmd_per_lun are different. can_queue should be set to the value of vq->vring.num where vq is the command virtqueue (the first one is okay if there's >1). If you want to change it, you'll have to do so in QEMU. Paolo > I tested the performance impact and it's not noticable in the > libguestfs case even with very small cmd_per_lun settings, but > libguestfs is largely serial and so this result won't be applicable to > guests in general. > > Also, should the guest kernel validate cmd_per_lun to make sure it's > not too small or large? And if so, what would the limits be? > > Rich. > > From e923e49846189b2f55f3f02b70a290d4be237ed5 Mon Sep 17 00:00:00 2001 > From: "Richard W.M. Jones" > Date: Thu, 10 Aug 2017 12:21:47 +0100 > Subject: [PATCH] scsi: virtio_scsi: Set can_queue based on cmd_per_lun passed > by hypervisor. > > Signed-off-by: Richard W.M. Jones > --- > drivers/scsi/virtio_scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 9be211d68b15..b22591e9b16b 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -973,7 +973,7 @@ static int virtscsi_probe(struct virtio_device *vdev) > goto virtscsi_init_failed; > > cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1; > - shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue); > + shost->cmd_per_lun = shost->can_queue = cmd_per_lun; > shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0x; > > /* LUNs > 256 are reported with format 1, so they go in the range >
Re: Increased memory usage with scsi-mq
On 09/08/2017 18:01, Christoph Hellwig wrote: > On Mon, Aug 07, 2017 at 03:07:48PM +0200, Paolo Bonzini wrote: >> can_queue should depend on the virtqueue size, which unfortunately can >> vary for each virtio-scsi device in theory. The virtqueue size is >> retrieved by drivers/virtio prior to calling vring_create_virtqueue, and >> in QEMU it is the second argument to virtio_add_queue. > > Why is that unfortunate? We don't even have to set can_queue in > the host template, we can dynamically set it on per-host basis. Ah, cool, I thought allocations based on can_queue happened already in scsi_host_alloc, but they happen at scsi_add_host time. Paolo
Re: Increased memory usage with scsi-mq
On 07/08/2017 14:27, Richard W.M. Jones wrote: > Also I noticed this code in virtio_scsi.c: > > cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1; > shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue); > > but setting cmd_per_lun (as a qemu virtio-scsi-pci parameter) didn't > seem to make any difference to memory usage. That's because memory depends on shost->can_queue, not shost->cmd_per_lun (see scsi_mq_setup_tags). can_queue should depend on the virtqueue size, which unfortunately can vary for each virtio-scsi device in theory. The virtqueue size is retrieved by drivers/virtio prior to calling vring_create_virtqueue, and in QEMU it is the second argument to virtio_add_queue. For virtio-scsi this is VIRTIO_SCSI_VQ_SIZE, which is 128, so changing can_queue to 128 should be a no brainer. Thanks, Paolo
Re: Increased memory usage with scsi-mq
On 05/08/2017 17:51, Richard W.M. Jones wrote:
> On Sat, Aug 05, 2017 at 03:39:54PM +0200, Christoph Hellwig wrote:
>> For now can you apply this testing patch to the guest kernel?
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index 9be211d68b15..0cbe2c882e1c 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
>> @@ -818,7 +818,7 @@ static struct scsi_host_template
>> virtscsi_host_template_single = {
>> .eh_timed_out = virtscsi_eh_timed_out,
>> .slave_alloc = virtscsi_device_alloc,
>>
>> -.can_queue = 1024,
>> +.can_queue = 64,
>> .dma_boundary = UINT_MAX,
>> .use_clustering = ENABLE_CLUSTERING,
>> .target_alloc = virtscsi_target_alloc,
>> @@ -839,7 +839,7 @@ static struct scsi_host_template
>> virtscsi_host_template_multi = {
>> .eh_timed_out = virtscsi_eh_timed_out,
>> .slave_alloc = virtscsi_device_alloc,
>>
>> -.can_queue = 1024,
>> +.can_queue = 64,
>> .dma_boundary = UINT_MAX,
>> .use_clustering = ENABLE_CLUSTERING,
>> .target_alloc = virtscsi_target_alloc,
>> @@ -983,7 +983,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
>> shost->max_id = num_targets;
>> shost->max_channel = 0;
>> shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
>> -shost->nr_hw_queues = num_queues;
>> +shost->nr_hw_queues = 1;
>>
>> #ifdef CONFIG_BLK_DEV_INTEGRITY
>> if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
>
> Yes, that's an improvement, although it's still a little way off the
> density possible the old way:
>
> With scsi-mq enabled: 175 disks
> * With this patch:319 disks *
> With scsi-mq disabled: 1755 disks
>
> Also only the first two hunks are necessary. The kernel behaves
> exactly the same way with or without the third hunk (ie. num_queues
> must already be 1).
>
> Can I infer from this that qemu needs a way to specify the can_queue
> setting to the virtio-scsi driver in the guest kernel?
You could also add a module parameter to the driver, and set it to 64 on
the kernel command line (there is an example in
drivers/scsi/vmw_pvscsi.c of how to do it).
Paolo
[PATCH] virtio_scsi: always read VPD pages for multiqueue too
Multi-queue virtio-scsi uses a different scsi_host_template struct. Add the .device_alloc field there, too. Fixes: 25d1d50e23275e141e3a3fe06c25a99f4c4bf4e0 Cc: sta...@vger.kernel.org Cc: David Gibson Signed-off-by: Paolo Bonzini --- drivers/scsi/virtio_scsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index f8dbfeee6c63..ad1e7f1aba4c 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -826,6 +826,7 @@ static int virtscsi_map_queues(struct Scsi_Host *shost) .change_queue_depth = virtscsi_change_queue_depth, .eh_abort_handler = virtscsi_abort, .eh_device_reset_handler = virtscsi_device_reset, + .slave_alloc = virtscsi_device_alloc, .can_queue = 1024, .dma_boundary = UINT_MAX, -- 1.8.3.1
[PATCH] virtio_scsi: let host do exception handling
virtio_scsi tries to do exception handling after the default
30 seconds timeout expires. However, it's better to let the host
control the timeout, otherwise with a heavy I/O load it is
likely that an abort will also timeout. This leads to fatal
errors like filesystems going offline.
Disable the 'sd' timeout and allow the host to do exception
handling, following the precedent of the storvsc driver.
Hannes has a proposal to introduce timeouts in virtio, but
this provides an immediate solution for stable kernels too.
Reported-by: Douglas Miller
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: Hannes Reinecke
Cc: linux-scsi@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Paolo Bonzini
---
drivers/scsi/virtio_scsi.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index f8dbfeee6c63..55d344ea6869 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -796,6 +796,16 @@ static int virtscsi_map_queues(struct Scsi_Host *shost)
return blk_mq_virtio_map_queues(&shost->tag_set, vscsi->vdev, 2);
}
+/*
+ * The host guarantees to respond to each command, although I/O latencies might
+ * be higher than on bare meta. Reset the timer unconditionally to give the
+ * host a chance to perform EH.
+ */
+static enum blk_eh_timer_return virtscsi_eh_timed_out(struct scsi_cmnd *scmnd)
+{
+ return BLK_EH_RESET_TIMER;
+}
+
static struct scsi_host_template virtscsi_host_template_single = {
.module = THIS_MODULE,
.name = "Virtio SCSI HBA",
@@ -806,6 +816,7 @@ static struct scsi_host_template
virtscsi_host_template_single = {
.change_queue_depth = virtscsi_change_queue_depth,
.eh_abort_handler = virtscsi_abort,
.eh_device_reset_handler = virtscsi_device_reset,
+ .eh_timed_out = virtscsi_eh_timed_out,
.slave_alloc = virtscsi_device_alloc,
.can_queue = 1024,
@@ -826,6 +837,7 @@ static struct scsi_host_template
virtscsi_host_template_multi = {
.change_queue_depth = virtscsi_change_queue_depth,
.eh_abort_handler = virtscsi_abort,
.eh_device_reset_handler = virtscsi_device_reset,
+ .eh_timed_out = virtscsi_eh_timed_out,
.can_queue = 1024,
.dma_boundary = UINT_MAX,
--
2.13.0
Re: [PATCH] virtio_scsi: Always try to read VPD pages
On 13/04/2017 15:39, Stefan Hajnoczi wrote:
> On Thu, Apr 13, 2017 at 12:13:00PM +1000, David Gibson wrote:
>> @@ -705,6 +706,28 @@ static int virtscsi_device_reset(struct scsi_cmnd *sc)
>> return virtscsi_tmf(vscsi, cmd);
>> }
>>
>> +static int virtscsi_device_alloc(struct scsi_device *sdevice)
>> +{
>> +/*
>> + * Passed through SCSI targets (e.g. with qemu's 'scsi-block')
>> + * may have transfer limits which come from the host SCSI
>> + * controller something on the host side other than the target
>
> s/controller something/controller or something/ ?
Acked-by: Paolo Bonzini
with this change.
Paolo
Re: [PATCH 26/27] scsi: sd: Separate zeroout and discard command choices
On 05/04/2017 19:21, Christoph Hellwig wrote:
> +static ssize_t
> +zeroing_mode_store(struct device *dev, struct device_attribute *attr,
> +const char *buf, size_t count)
> +{
> + struct scsi_disk *sdkp = to_scsi_disk(dev);
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> +
> + if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE], 20))
> + sdkp->zeroing_mode = SD_ZERO_WRITE;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS16_UNMAP;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS10_UNMAP;
Should this be conditional on lbprz, lbpws, lbpws10 and max_ws_blocks?
Thanks,
Paolo
Re: [PATCH 03/23] sd: implement REQ_OP_WRITE_ZEROES
On 29/03/2017 18:28, Bart Van Assche wrote: > On Wed, 2017-03-29 at 16:51 +0200, Paolo Bonzini wrote: >> On 28/03/2017 20:50, Bart Van Assche wrote: >>> This means that just like the start and end of a discard must be aligned on >>> a >>> discard_granularity boundary, WRITE SAME commands with the UNMAP bit set >>> must >>> also respect that granularity. I think this means that either >>> __blkdev_issue_zeroout() has to be modified such that it rejects unaligned >>> REQ_OP_WRITE_ZEROES operations or that blk_bio_write_same_split() has to be >>> modified such that it generates REQ_OP_WRITEs for the unaligned start and >>> tail. >> >> I don't think this is the case. > > Hello Paolo, > > Can you cite the section(s) from the SCSI specs that support your view? I > reread the "5.49 WRITE SAME (10) command" and "4.7.3.4.4 WRITE SAME command > and unmap operations" sections but I have not found any explicit statement > that specifies the behavior for unaligned WRITE SAME commands with the UNMAP > bit set. It seems to me like the OPTIMAL UNMAP GRANULARITY parameter was > overlooked when both sections were written. Should we ask the T10 committee > for a clarification? >From 4.7.3.4.4: -- If unmap operations are requested in a WRITE SAME command, then for each specified LBA: if the Data-Out Buffer of the WRITE SAME command is the same as the logical block data returned by a read operation from that LBA while in the unmapped state (see 4.7.4.5), then: 1) the device server performs the actions described in table 6; and 2) if an unmap operation is not performed in step 1), then the device server shall perform the specified write operation to that LBA; -- and from the description of WRITE SAME (10): "subsequent read operations behave as if the device server wrote the single block of user data received from the Data-Out Buffer to each logical block without modification" (I have a slightly older copy though, it's 5.45 here). It's pretty unambiguous that if the device cannot unmap (including the case where the request is misaligned with respect to the granularity) it does a write. > Another question is, if the specification of WRITE SAME + UNMAP would be > made unambiguous in the SBC document, whether or not we should take the risk > to trigger behavior that is not what we expect by sending unaligned WRITE > SAME + UNMAP commands to SCSI devices? Yes, I think we should. Paolo
Re: RFC: always use REQ_OP_WRITE_ZEROES for zeroing offload
On 23/03/2017 23:53, Lars Ellenberg wrote: > Thin does not claim to zero data on discard. which is ok, and correct, > because it only punches holes on full chunks (or whatever you call > them), and leaves the rest in the mapping tree as is. > > And that behaviour would prevent DRBD from exposing discards if > configured on top of thin. (see above) > > But thin *could* easily guarantee zeroing, by simply punching holes > where it can, and zeroing out the not fully-aligned partial start and > end of the range. That's the difference between REQ_OP_DISCARD (only punches holes on full chunks) and REQ_OP_WRITE_ZEROES with the REQ_UNMAP flag (punches holes + zeroes incomplete chunks). dm-thinp's REQ_OP_DISCARD should not do anything for unaligned parts. Instead, layers above should use REQ_OP_WRITE_ZEROES (with or without REQ_UNMAP, as required) if they need zeroes. dm-thinp would have to split off the partial chunks, and zero them in the lower-level device with REQ_OP_WRITE_ZEROES. Paolo
Re: [PATCH 12/23] sd: handle REQ_UNMAP
On 28/03/2017 18:48, Bart Van Assche wrote:
>> +if (rq->cmd_flags & REQ_UNMAP) {
>> +switch (sdkp->provisioning_mode) {
>> +case SD_LBP_WS16:
>> +return sd_setup_write_same16_cmnd(cmd, true);
>> +case SD_LBP_WS10:
>> +return sd_setup_write_same10_cmnd(cmd, true);
>> +}
>> +}
>> +
>> if (sdp->no_write_same)
>> return BLKPREP_INVALID;
>> if (sdkp->ws16 || sector > 0x || nr_sectors > 0x)
> Users can change the provisioning mode from user space from SD_LBP_WS16 into
> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
> 0x || nr_sectors > 0x) check if REQ_UNMAP is set.
Yeah, if REQ_UNMAP is set you should probably check sdkp->provisioning_mode
instead of sdkp->ws16, but apart from this it should still go through the
checks below.
Plus, if the provisioning mode is not ws10 or ws16, should
sd_setup_write_zeroes_cmnd:
1) do a WRITE SAME without UNMAP (what Christoph's code does)
2) return BLKPREP_INVALID
3) ignore provisioning mode and do a WRITE SAME with UNMAP
4) do a WRITE SAME without UNMAP for SD_LBP_{ZERO,FULL,DISABLE},
do a WRITE SAME with UNMAP for SD_LBP_{WS10,WS16,UNMAP}.
I'm in favor of (4). The distinction between SD_LBP_UNMAP, SD_LBP_WS10
and SD_LBP_WS16 is as problematic as discard_zeroes_data in my opinion.
Thanks,
Paolo
Re: [PATCH 23/23] block: remove the discard_zeroes_data flag
On 28/03/2017 19:00, Bart Van Assche wrote:
> On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
>> Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
>> kill this hack.
>>
>> [ ... ]
>>
>> diff --git a/Documentation/ABI/testing/sysfs-block
>> b/Documentation/ABI/testing/sysfs-block
>> index 2da04ce6aeef..dea212db9df3 100644
>> --- a/Documentation/ABI/testing/sysfs-block
>> +++ b/Documentation/ABI/testing/sysfs-block
>> @@ -213,14 +213,8 @@ What:
>> /sys/block//queue/discard_zeroes_data
>> Date: May 2011
>> Contact:Martin K. Petersen
>> Description:
>> -Devices that support discard functionality may return
>> -stale or random data when a previously discarded block
>> -is read back. This can cause problems if the filesystem
>> -expects discarded blocks to be explicitly cleared. If a
>> -device reports that it deterministically returns zeroes
>> -when a discarded area is read the discard_zeroes_data
>> -parameter will be set to one. Otherwise it will be 0 and
>> -the result of reading a discarded area is undefined.
>> +Will always return 0. Don't rely on any specific behavior
>> +for discards, and don't read this file.
>>
>> What: /sys/block//queue/write_same_max_bytes
>> Date: January 2012
>>
>> [ ... ]
>>
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -208,7 +208,7 @@ static ssize_t queue_discard_max_store(struct
>> request_queue *q,
>>
>> static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char
>> *page)
>> {
>> -return queue_var_show(queue_discard_zeroes_data(q), page);
>> +return 0;
>> }
>
> Hello Christoph,
>
> It seems to me like the documentation in Documentation/ABI/testing/sysfs-block
> and the above code are not in sync. I think the above code will cause reading
> from the discard_zeroes_data attribute to return an empty string ("") instead
> of "0\n".
>
> BTW, my personal preference is to remove this attribute entirely because
> keeping
> it will cause confusion, no matter how well we document the behavior of this
> attribute.
If you remove it, you should probably remove the BLKDISCARDZEROES ioctl too.
That said, the issue with discard_zeroes_data is that it is badly
defined; it was defined as "if I unmap X, will it read as zeroes?" but
this is not how the SCSI standard defines e.g. the UNMAP command with
LBPRZ=1. But knowing something like LBPRZ ("if something is unmapped,
will it read as zeroes?") _would_ actually be useful for userspace.
This will be especially true once sd maps lseek(SEEK_HOLE/SEEK_DATA) to
the SCSI GET LBA STATUS command, or once dm-thin supports them.
Secondarily, if the former returns 1, userspace is also interested in
knowing "can REQ_OP_WRITE_ZEROES+REQ_UNMAP ever unmap anything?", i.e.
whether BLKDEV_ZERO_NOFALLBACK will ever return anything but
-EOPNOTSUPP. For SCSI, this should intuitively mean whether LBPWS or
LBPWS10 are set, but the details depend on how the sd driver implements
REQ_OP_WRITE_ZEROES with REQ_UNMAP.
Paolo
Re: [PATCH 03/23] sd: implement REQ_OP_WRITE_ZEROES
On 28/03/2017 20:50, Bart Van Assche wrote: > > This means that just like the start and end of a discard must be aligned on a > discard_granularity boundary, WRITE SAME commands with the UNMAP bit set must > also respect that granularity. I think this means that either > __blkdev_issue_zeroout() has to be modified such that it rejects unaligned > REQ_OP_WRITE_ZEROES operations or that blk_bio_write_same_split() has to be > modified such that it generates REQ_OP_WRITEs for the unaligned start and > tail. I don't think this is the case. Rather, Linux should try to align the WRITE SAME commands to the optimal unmap granularity if the zeroed area requires performing more than one WRITE SAME command (i.e. > maximum write same length or too large to fit in the CDB). However, even in that case it can use WRITE SAME with UNMAP for the unaligned start and tail. Unlike the UNMAP command, the SCSI standard does guarantee that zeroes are written in the unaligned parts. Paolo
Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host
- Original Message -
> From: "Michael S. Tsirkin"
> To: "Fam Zheng"
> Cc: linux-ker...@vger.kernel.org, "Paolo Bonzini" ,
> linux-scsi@vger.kernel.org, "James E.J.
> Bottomley" , "Jason Wang" ,
> "Martin K. Petersen"
> , stefa...@redhat.com,
> virtualizat...@lists.linux-foundation.org
> Sent: Thursday, January 26, 2017 11:06:27 PM
> Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host
>
> On Thu, Jan 26, 2017 at 11:41:09AM +0800, Fam Zheng wrote:
> > This implements the VIRTIO_SCSI_F_FC_HOST feature by reading the config
> > fields and presenting them as sysfs fc_host attributes. The config
> > change handler is added here because primary_active will toggle during
> > migration.
>
> Looks like there's active discussion on virtio tc mailing list.
> It's ok to post patches meanwhile but best as RFC,
> and repost after controversy is resolved.
Discussion on the TC mailing list was not about the merit of the feature,
only about the timing of the vote.
Paolo
>
> >
> > Signed-off-by: Fam Zheng
> > ---
> > drivers/scsi/virtio_scsi.c | 60
> > +-
> > 1 file changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> > index ec91bd0..1bb330c 100644
> > --- a/drivers/scsi/virtio_scsi.c
> > +++ b/drivers/scsi/virtio_scsi.c
> > @@ -28,6 +28,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #define VIRTIO_SCSI_MEMPOOL_SZ 64
> > @@ -795,6 +796,15 @@ static struct scsi_host_template
> > virtscsi_host_template_multi = {
> > .track_queue_depth = 1,
> > };
> >
> > +static struct fc_function_template virtscsi_fc_template = {
> > + .show_host_node_name = 1,
> > + .show_host_port_name = 1,
> > + .show_host_port_type = 1,
> > + .show_host_port_state = 1,
> > +};
> > +
> > +static struct scsi_transport_template *virtscsi_fc_transport_template;
> > +
> > #define virtscsi_config_get(vdev, fld) \
> > ({ \
> > typeof(((struct virtio_scsi_config *)0)->fld) __val; \
> > @@ -956,15 +966,42 @@ static int virtscsi_init(struct virtio_device *vdev,
> > return err;
> > }
> >
> > +static void virtscsi_update_fc_host_attrs(struct virtio_device *vdev)
> > +{
> > + struct Scsi_Host *shost = vdev->priv;
> > + u8 node_name[8], port_name[8];
> > +
> > + if (virtscsi_config_get(vdev, primary_active)) {
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, primary_wwnn),
> > + &node_name, 8);
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, primary_wwpn),
> > + &port_name, 8);
> > + } else {
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, secondary_wwnn),
> > + &node_name, 8);
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, secondary_wwpn),
> > + &port_name, 8);
> > + }
>
> This is racy, isn't it? You need to wrap this in a generation check
> otherwise read can race with primary_active changing.
> And you might want a wrapper to virtio_cread_bytes that does not
> include generation check.
>
> > + fc_host_node_name(shost) = wwn_to_u64(node_name);
> > + fc_host_port_name(shost) = wwn_to_u64(port_name);
> > + fc_host_port_type(shost) = FC_PORTTYPE_NPORT;
> > + fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
> > +}
> > +
> > static int virtscsi_probe(struct virtio_device *vdev)
> > {
> > - struct Scsi_Host *shost;
> > + struct Scsi_Host *shost = NULL;
> > struct virtio_scsi *vscsi;
> > int err;
> > u32 sg_elems, num_targets;
> > u32 cmd_per_lun;
> > u32 num_queues;
> > struct scsi_host_template *hostt;
> > + bool fc_host_enabled;
> >
> > if (!vdev->config->get) {
> > dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > @@ -987,6 +1024,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
> > if (!shost)
> > return -ENOMEM;
> >
> > + fc_host_enabled = virtio_has_feature(vdev, VIRTIO_SCSI_F_FC_HOST);
> > + if (fc_host_enabled)
> > +
Re: [PATCH 2/2] virtio_scsi: Implement fc_host
On 16/01/2017 18:26, Fam Zheng wrote: >> Is the endianness correct for big-endian host here? > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name, > this patch does the same, so using virtio_* helpers for these fields should > handle the endianness correctly. I was suspicious about it because they are defined as "u8 x[8]" in the virtio_scsi_config struct. So you would need to read with virtio_cread_bytes and pass the result to wwn_to_u64. For example, if you have 0x500123456789abcd this would be 0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd in virtio_scsi_config, and then virtio_cread64 would read it as a little-endian u64, 0xcdab896745230150. Maybe your QEMU patch is also writing things as little-endian 64-bit integers, rather than 8-element arrays of bytes? Paolo > Maybe we should use u64 in struct virtio_scsi_config as well? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature
> How it this supposed to work? > You do export the WWPN/WWNN of the associated host to the guest (nb: > will get interesting for non NPIV setups ...), but virtio scsi will > still do a LUN remapping. > IE the LUNs you see on the host will be different from the LUNs > presented to the guest. This is taken care of in the host by presenting to the host all LUNs from a host's NPIV vHBA. (Libvirt probably would be the one taking care of this, because QEMU may not have enough permissions). > Plus you don't _actually_ expose the FC host, but rather the WWPN of the > host presenting the LUN. > So how do you handle LUNs from different FC hosts on the guest? I'm not sure I understand. Neither I nor Fam know this stuff very well, but we are trying to do the same as Hyper-V (and other proprietary hypervisors too). > Overall, I'm not overly happy with this approach. > You already added WWPN ids to the virtio transport, so why didn't you > update the LUN field, too, to avoid this ominous LUN remapping? Is this your old idea of adding a separate target field to commands, in order to support 64-bit LUNs? That is separate, and most FC drivers only default to 16-bit LUNs anyway. > And we really should make sure to have a single FC host in the guest > presenting all LUNs. Yes, of course. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] virtio_scsi: Implement fc_host
On 16/01/2017 17:04, Fam Zheng wrote:
> + node_name = virtio_cread64(vdev,
> + offsetof(struct virtio_scsi_config, primary_wwnn));
> + port_name = virtio_cread64(vdev,
> + offsetof(struct virtio_scsi_config, primary_wwpn));
> + } else {
> + node_name = virtio_cread64(vdev,
> + offsetof(struct virtio_scsi_config, secondary_wwnn));
> + port_name = virtio_cread64(vdev,
> + offsetof(struct virtio_scsi_config, secondary_wwpn));
Is the endianness correct for big-endian host here?
Thanks,
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ibmvscsi: add write memory barrier to CRQ processing
On 08/12/2016 00:31, Tyrel Datwyler wrote:
> The first byte of each CRQ entry is used to indicate whether an entry is
> a valid response or free for the VIOS to use. After processing a
> response the driver sets the valid byte to zero to indicate the entry is
> now free to be reused. Add a memory barrier after this write to ensure
> no other stores are reordered when updating the valid byte.
>
> Signed-off-by: Tyrel Datwyler
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
> b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index d9534ee..2f5b07e 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data)
> while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) {
> ibmvscsi_handle_crq(crq, hostdata);
> crq->valid = VIOSRP_CRQ_FREE;
> + wmb();
> }
>
> vio_enable_interrupts(vdev);
> @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data)
> vio_disable_interrupts(vdev);
> ibmvscsi_handle_crq(crq, hostdata);
> crq->valid = VIOSRP_CRQ_FREE;
> + wmb();
Should this driver use virt_wmb instead?
Paolo
> } else {
> done = 1;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()
_t rc = 0;
> unsigned long max_pages_per_loop = PVM_MAX_KMALLOC_PAGES
> / sizeof(struct pages *);
> + unsigned int flags = FOLL_REMOTE;
>
> /* Work out address and page range required */
> if (len == 0)
> return 0;
> nr_pages = (addr + len - 1) / PAGE_SIZE - addr / PAGE_SIZE + 1;
>
> + if (vm_write)
> + flags |= FOLL_WRITE;
> +
> while (!rc && nr_pages && iov_iter_count(iter)) {
> int pages = min(nr_pages, max_pages_per_loop);
> size_t bytes;
> @@ -104,8 +108,7 @@ static int process_vm_rw_single_vec(unsigned long addr,
>* current/current->mm
>*/
> pages = __get_user_pages_unlocked(task, mm, pa, pages,
> - vm_write, 0, process_pages,
> - FOLL_REMOTE);
> + process_pages, flags);
> if (pages <= 0)
> return -EFAULT;
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index db96688..8035cc1 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -84,7 +84,8 @@ static void async_pf_execute(struct work_struct *work)
>* mm and might be done in another context, so we must
>* use FOLL_REMOTE.
>*/
> - __get_user_pages_unlocked(NULL, mm, addr, 1, 1, 0, NULL, FOLL_REMOTE);
> + __get_user_pages_unlocked(NULL, mm, addr, 1, NULL,
> + FOLL_WRITE | FOLL_REMOTE);
>
> kvm_async_page_present_sync(vcpu, apf);
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 81dfc73..28510e7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1416,10 +1416,15 @@ static int hva_to_pfn_slow(unsigned long addr, bool
> *async, bool write_fault,
> down_read(¤t->mm->mmap_sem);
> npages = get_user_page_nowait(addr, write_fault, page);
> up_read(¤t->mm->mmap_sem);
> - } else
> + } else {
> + unsigned int flags = FOLL_TOUCH | FOLL_HWPOISON;
> +
> + if (write_fault)
> + flags |= FOLL_WRITE;
> +
> npages = __get_user_pages_unlocked(current, current->mm, addr,
> 1,
> -write_fault, 0, page,
> -FOLL_TOUCH|FOLL_HWPOISON);
> +page, flags);
> + }
> if (npages != 1)
> return npages;
>
>
Acked-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove the unnecessary semicolons
On 19/08/2016 04:03, Zhou Jie wrote:
> At the end of funcions, semicolons are unnecessary. So drop them.
>
> Signed-off-by: Chao Fan
> ---
> drivers/scsi/virtio_scsi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 7dbbb29..9632a0c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -241,7 +241,7 @@ static void virtscsi_req_done(struct virtqueue *vq)
> struct virtio_scsi_vq *req_vq = &vscsi->req_vqs[index];
>
> virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
> -};
> +}
>
> static void virtscsi_poll_requests(struct virtio_scsi *vscsi)
> {
> @@ -267,7 +267,7 @@ static void virtscsi_ctrl_done(struct virtqueue *vq)
> struct virtio_scsi *vscsi = shost_priv(sh);
>
> virtscsi_vq_done(vscsi, &vscsi->ctrl_vq, virtscsi_complete_free);
> -};
> +}
>
> static void virtscsi_handle_event(struct work_struct *work);
>
> @@ -413,7 +413,7 @@ static void virtscsi_event_done(struct virtqueue *vq)
> struct virtio_scsi *vscsi = shost_priv(sh);
>
> virtscsi_vq_done(vscsi, &vscsi->event_vq, virtscsi_complete_event);
> -};
> +}
>
> /**
> * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
>
Acked-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: Add QEMU CD-ROM to VPD Inquiry Blacklist
On 06/06/2016 17:47, John Snow wrote: > > > Various downstreams may have backported the VPD fix to older versions, > > > we need to be careful not to block those, too ... so targeting the core > > > behavior seems like the more strictly correct, easily maintainable > > > solution. > > > > I think this is not practical. I'm okay with the big hammer if an > > algorithmic fix is not feasible; but otherwise it does seem a better > > idea than blacklisting based on inquiry data... > > You think the more practical solution is a SCSI driver that can hang > because of an incorrect/missing response and to maintain a carefully > curated blacklist to work around this behavior? The best solution would be an algorithmic fix, perhaps predicated by some kind of quirk bit. A carefully curated blacklist is impossible because you cannot account for a zillion downstreams, most of which probably don't change the inquire vendor/product data; version numbers are awful. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: Add QEMU CD-ROM to VPD Inquiry Blacklist
On 06/06/2016 17:41, John Snow wrote: > On 06/06/2016 11:05 AM, Paolo Bonzini wrote: >> For ATAPI, you have to blacklist all versions up to 2.2 inclusive. >> >> This gives: >> >> - QEMU / QEMU CD-ROM / 0.8.(this is IDE and SCSI) >> - QEMU / QEMU CD-ROM / 0.9.(this is IDE and SCSI) >> - QEMU / QEMU CD-ROM / 0.10(this is SCSI only) >> - QEMU / QEMU CD-ROM / 0.11(this is SCSI only) >> - QEMU / QEMU DVD-ROM / 0.8. (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.9. (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.10 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.11 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.12 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.13 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.14 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 0.15 (this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.0(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.1(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.2(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.3(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.4(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.5(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.6(this is IDE only) >> - QEMU / QEMU DVD-ROM / 1.7(this is IDE only) >> - QEMU / QEMU DVD-ROM / 2.0(this is IDE only) >> - QEMU / QEMU DVD-ROM / 2.1(this is IDE only) >> - QEMU / QEMU DVD-ROM / 2.2(this is IDE only) >> > > If this bug is caused by a missing VPD response, Paolo's version history > here is correct for upstream versions. > > Various downstreams may have backported the VPD fix to older versions, > we need to be careful not to block those, too ... so targeting the core > behavior seems like the more strictly correct, easily maintainable solution. I think this is not practical. I'm okay with the big hammer if an algorithmic fix is not feasible; but otherwise it does seem a better idea than blacklisting based on inquiry data... Thanks, Paolo > Why not just dynamically blacklist devices that fail to respond to VPD > inquiries? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: Add QEMU CD-ROM to VPD Inquiry Blacklist
On 06/06/2016 16:22, Hannes Reinecke wrote:
> So either we dig into what went wrong with qemu 0.8, or we figure out
> from which qemu version things start to behave nicely, and blacklist
> earlier versions.
>
> > > Either way, this patch is wrong.
> >
> > If we can identify which versions work, we can update it. Otherwise
> > I think we have to be conservative.
>
> So far we just had this single report where the upstream kernel didn't
> work correctly with a (really old) version of qemu.
> Hardly justifying blacklisting qemu CD-ROM in general.
To further complicate the matter there are two QEMU MMC devices:
1) ATAPI
- vendor "QEMU" / product name "QEMU CD-ROM" before QEMU 0.10.0
- vendor "QEMU" / product name "QEMU DVD-ROM" since QEMU 0.10.0
2) native SCSI
- vendor "QEMU" / product name "QEMU CD-ROM"
VPD in the SCSI CD-ROM probably has always worked, but I would blacklist
up to 0.11 inclusive just to be safe. Those versions are dead anyway.
VPD in the ATAPI CD-ROM is newer, and that's where the bug was reported on:
> [4.439488] ata2.00: ATAPI: QEMU CD-ROM, 0.8.2, max UDMA/100
> [4.443649] ata2.00: configured for MWDMA2
> [4.450267] scsi 1:0:0:0: CD-ROMQEMU QEMU CD-ROM 0.8.
> PQ: 0 ANSI: 5
> [4.464317] ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
> frozen
> [4.464319] ata2.00: BMDMA stat 0x5
> [4.464339] ata2.00: cmd a0/01:00:00:00:01/00:00:00:00:00/a0 tag 0 dma
> 16640 in
> [4.464339] Inquiry 12 01 00 00 ff 00res
> 48/20:02:00:24:00/00:00:00:00:00/a0 Emask 0x2 (HSM violation)
> [4.464341] ata2.00: status: { DRDY DRQ }
For ATAPI, you have to blacklist all versions up to 2.2 inclusive.
This gives:
- QEMU / QEMU CD-ROM / 0.8.(this is IDE and SCSI)
- QEMU / QEMU CD-ROM / 0.9.(this is IDE and SCSI)
- QEMU / QEMU CD-ROM / 0.10(this is SCSI only)
- QEMU / QEMU CD-ROM / 0.11(this is SCSI only)
- QEMU / QEMU DVD-ROM / 0.8. (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.9. (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.10 (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.11 (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.12 (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.13 (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.14 (this is IDE only)
- QEMU / QEMU DVD-ROM / 0.15 (this is IDE only)
- QEMU / QEMU DVD-ROM / 1.0(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.1(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.2(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.3(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.4(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.5(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.6(this is IDE only)
- QEMU / QEMU DVD-ROM / 1.7(this is IDE only)
- QEMU / QEMU DVD-ROM / 2.0(this is IDE only)
- QEMU / QEMU DVD-ROM / 2.1(this is IDE only)
- QEMU / QEMU DVD-ROM / 2.2(this is IDE only)
Thanks,
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Application error handling with write-back caching
On 10/05/2016 19:31, James Bottomley wrote: > > What about a SPACE ALLOCATION FAILED error or a similar error that > > can be fixed by administrator actions (or just by a concurrent > > process doing an UNMAP)? Would a subsequent cache flush cause data > > loss? > > You're now asking about how these are handled? It's not a SCSI > problem. I believe if you look at the various layers, RAID would still > treat it as fatal and fail the drive and so would most filesystems. > The AEN warnings for TP are reported, but the admin has to sort it out > before they become a fatal error. Thanks, fatal errors are fine I guess. We were worried that the next SYNCHRONIZE CACHE would succeed and throw away the writes because it has already "performed a write medium operation". POSIX fsync is pretty underspecified in this respect too; gluster has been throwing away those writes for a long time! It stopped now because we explained the issue to them, but it's pointless if the next layer below does the same---hence Stefan's question. (In our case the next layer is not the page cache, because we generally use O_DIRECT. Evicting dirty pages from the page cache would be okay if the process(es) that wrote them are SIGKILLed, but in general it would be a problem too). Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Application error handling with write-back caching
On 10/05/2016 16:16, James Bottomley wrote: > > If "is performed" just means "completes", maybe with an error, the > > application would have to resubmit write requests and then try to > > flush the write cache again. > > > > I'm not aware of applications that keep acknowledged write data > > around until the cache flush completion in order to retry writes. > > I think you may be misunderstanding the nature of the returned error. > It will be permanent and fatal and usually signal that the device has > a failed sector that can't be remapped and so the device itself has for > most purposes failed. The only recovery is if you happen to have RAID, > in which case the RAID layer will mostly take care of it. What about a SPACE ALLOCATION FAILED error or a similar error that can be fixed by administrator actions (or just by a concurrent process doing an UNMAP)? Would a subsequent cache flush cause data loss? Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] scsi: cleanup ioctl headers and provide UAPI versions
On 25/09/2015 11:27, Paolo Bonzini wrote: > This is v3 of the series to provide an "official" sg.h header (and > scsi_ioctl.h too, though it's basically obsolete) together with the other > userspace API definitions. The change from v2 to v3 is that defaults > for sg.c are not exported in include/uapi/linux/sg.c. > > Paolo > > 2.5.0 > What happened to these patches?... Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 02/11/2015 16:05, Mike Snitzer wrote: > > In any case, if we don't start path activation we should return > > ENOTCONN, not ENOTTY. > > Currently, if we don't start path activation we're returning EIO. > ENOTCONN is used for when we do start path activation (and ENOTCONN is > the means for DM core to retry) > > We _could_ change the ENOTCONN to be EAGAIN and EIO to ENOTCONN... This makes sense... though of course testing the impact of this on userspace is going to be hard. :( Chances are that userspace is not expecting EAGAIN either. Even if they did, how would someone know that they can now retry the ioctl after getting EAGAIN? Should they just do it in a loop? Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 02/11/2015 14:56, Hannes Reinecke wrote: > But then the real question remains: > > What is the 'correct' behaviour for ioctls when no path retry > is active (or when no paths are present)? > > Should we start path activation? > If so, should we wait for path activation to finish, risking udev > killing the worker for that event (udev has a built-in timeout of > 120 seconds, which we might easily exceed when we need to activate > paths for large installations or slow path activation ... just > thinking of NetApp takeover/giveback cycle)? > If we're not waiting for path activation, where would be the point > in starting it, seeing that we're not actually interested in the result? > And if we shouldn't start a path activation, what is the point of > having code for it in the first place? That's a fair question, and it depends on what said udev worker actually does. In any case, if we don't start path activation we should return ENOTCONN, not ENOTTY. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 02/11/2015 08:28, Hannes Reinecke wrote: > > With the original code we would need to wait for path activation > before we would be able to figure out if the ioctl is valid. > However, the callback to verify the ioctl is sd_ioctl(), which as > a first step calls scsi_verify_ioctl(). > So my reasoning was that we can as well call scsi_verify_ioctl() > early, and allow it to filter out known invalid ioctls. > Then we wouldn't need to wait for path activation and return > immediately. That in principle makes sense. Unfortunately, before path activation you can only find out if a ioctl is valid. You cannot find out which ioctls are _in_valid, because path activation sets the bdev and that might make all ioctls valid. > Incidentally, in the 'r == -ENOTCONN' case, we're waiting > for path activation, but then just bail out with -ENOTCONN. > As we're not resetting -ENOTCONN, where's the point in activate the > paths here? > Shouldn't we retry to figure out if -ENOTCONN is still true? That makes sense too. You've done the work, might as well reap the benefits... Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 31/10/2015 23:47, Mike Snitzer wrote:
> Yes, with your commit ec8013be ("dm: do not forward ioctls from logical
> volumes to the underlying device") you added protections to disallow
> issuing ioctls to a partition that could impact the rest of the device.
>
> Given that I can see why you're seizing on the "ti->len !=
> i_size_read(bdev->bd_inode) >> SECTOR_SHIFT" negative checks that gate
> the call to scsi_verify_blk_ioctl().
Right.
> For Hannes, and in my head, it didn't matter if a future bdev satisfies
> the length condition.
I agree actually. The only problem is that the returned errno value is
ENOTTY, and to userspace that "sounds like" a future bdev will not make
the ioctl valid.
> I could've sworn that unprivledged users (without CAP_SYS_RAWIO)
> wouldn't be allowed to issue ioctls. Am I completely mistaken?
They are allowed to issue ioctls.
CAP_SYS_RAWIO changes that to also allow issuing of ioctls to
partitions. That was required by Linus for backwards compatibility.
> Or is
> it still contentious and DM-mpath removing the ability to allow these
> unprivledged ioctls (as a side-effect of Hannes' commit ec8013be) makes
> your life, and other virt users' lives, harder?
Yes, it would. virt runs as an unprivileged user (so does CD burning,
which was the original reason to let SG_IO run by unprivileged users;
there are probably other use cases).
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 31/10/2015 19:13, Mike Snitzer wrote: > > But that's wrong, I think. It's a false positive in > > scsi_verify_blk_ioctl(). > > > > If the ioctl is valid when bdev becomes non-NULL (and it will be if > > ti->len becomes equal to i_size_read(bdev->bd_inode) >> SECTOR_SHIFT), > > you should not return -ENOIOCTLCMD aka ENOTTY, because userspace doesn't > > think the ioctls can go away and come back. So Hannes's patch broke the > > userspace ABI. :( > > Huh? All that Hannes' patch did was add early verification of the ioctl > if there are no paths, since: there is no point queueing an ioctl that > is invalid. > > [snip discussion of Christoph's patches] > > The point is scsi_verify_blk_ioctl() is saying the ioctl isn't valid. > It has nothing to do with the existance of a bdev or not; but everything > to do with the unprivledged user's request to issue an ioctl. ... but the call is skipped (and all ioctls are valid) if ti->len == i_size_read(bdev->bd_inode) >> SECTOR_SHIFT. Therefore, until you have the bdev you don't know which ioctls are valid, and you must assume all of them are. You can't do anything unsafe anyway until you have the bdev. This is the reasoning prior to Hannes's change. Afterwards, you end up calling scsi_verify_blk_ioctl() when bdev == NULL. If the future bdev satisfies the above condition on ti->len, this means that ioctl(SG_IO) switches from ENOTTY to available. Userspace is clearly not expecting that. > Paolo, AFAIK unprivledged ioctls is one of your pet-projects so your > insight on what, if anything, needs changing to support them is the > insight I think we need. I hope the above provides some extra information. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]
On 29/10/2015 14:18, Mike Snitzer wrote: > > 4) dmesg shows that scsi_verify_blk_ioctl() failed for SG_IO (0x2285); > >it returns -ENOIOCTLCMD, later replaced with -ENOTTY in vfs_ioctl(). > > > > $ dmesg > > <...> > > [] device-mapper: multipath: Failing path 65:144. > > [] device-mapper: multipath: Failing path 67:144. > > [] device-mapper: multipath: Failing path 65:224. > > [] device-mapper: multipath: Failing path 68:32. > > [] sgio_inquiry: sending ioctl 2285 to a partition! > > So scsi_verify_blk_ioctl() considers the ioctl invalid. But that's wrong, I think. It's a false positive in scsi_verify_blk_ioctl(). If the ioctl is valid when bdev becomes non-NULL (and it will be if ti->len becomes equal to i_size_read(bdev->bd_inode) >> SECTOR_SHIFT), you should not return -ENOIOCTLCMD aka ENOTTY, because userspace doesn't think the ioctls can go away and come back. So Hannes's patch broke the userspace ABI. :( Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/4] scsi: cleanup scsi/scsi_ioctl.h
SCSI_REMOVAL_* goes together with other SCSI command constants in
include/scsi/scsi.h. It is also used outside the implementation
of the ioctls (and it is not part of the user API).
scsi_fctargaddress/Scsi_FCTargAddress has had no in-tree use since
commit ca61f10ab2b8 ("[SCSI] remove broken driver cpqfc", 2005-10-29).
Remove it, just in time for the the tenth anniversary of its demise.
Cc: James Bottomley
Cc: Christoph Hellwig
Cc: linux-scsi@vger.kernel.org
Reviewed-by: Bart Van Assche
Signed-off-by: Paolo Bonzini
---
include/scsi/scsi.h | 6 ++
include/scsi/scsi_ioctl.h | 8
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index e0a3398b1547..5e2bafdbd96f 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -279,6 +279,12 @@ static inline int scsi_is_wlun(u64 lun)
#define SCSI_INQ_PQ_NOT_CON 0x01
#define SCSI_INQ_PQ_NOT_CAP 0x03
+/*
+ * PREVENT/ALLOW MEDIUM REMOVAL
+ */
+#defineSCSI_REMOVAL_PREVENT1
+#defineSCSI_REMOVAL_ALLOW 0
+
/*
* Here are some scsi specific ioctl commands which are sometimes useful.
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index 8d19d1d233c3..c81962bef7a0 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -12,9 +12,6 @@
#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
-#defineSCSI_REMOVAL_PREVENT1
-#defineSCSI_REMOVAL_ALLOW 0
-
#ifdef __KERNEL__
struct scsi_device;
@@ -34,11 +31,6 @@ typedef struct scsi_idlun {
__u32 host_unique_id;
} Scsi_Idlun;
-/* Fibre Channel WWN, port_id struct */
-typedef struct scsi_fctargaddress {
- __u32 host_port_id;
- unsigned char host_wwn[8]; // include NULL term.
-} Scsi_FCTargAddress;
int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev,
int cmd, bool ndelay);
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
Provide a UAPI version of the header in the kernel, making it easier
for interested projects to use an up-to-date version of the header.
The new headers are placed under uapi/linux/ so as not to conflict
with the glibc-provided headers in /usr/include/scsi.
/dev/sgN default values are implementation aspects, and are moved to
drivers/scsi/sg.c instead (together with e.g. SG_ALLOW_DIO_DEF).
However, SG_SCATTER_SZ is used by Wine so it is kept in linux/sg.h
SG_MAX_QUEUE could also be useful.
struct scsi_ioctl_command and struct scsi_idlun used to be under
"#ifdef __KERNEL__", but they are actually useful for userspace as
well. Add them to the new header.
Cc: James Bottomley
Cc: Christoph Hellwig
Cc: linux-scsi@vger.kernel.org
Cc: Bart Van Assche
Signed-off-by: Paolo Bonzini
---
drivers/scsi/sg.c | 8 +-
include/scsi/scsi_ioctl.h | 50 +-
include/scsi/sg.h | 261 +-
include/uapi/linux/Kbuild | 2 +
include/{scsi => uapi/linux}/scsi_ioctl.h | 23 +--
include/{scsi => uapi/linux}/sg.h | 40 +
6 files changed, 25 insertions(+), 359 deletions(-)
copy include/{scsi => uapi/linux}/scsi_ioctl.h (77%)
copy include/{scsi => uapi/linux}/sg.h (91%)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 9d7b7db75e4b..3410e50a37d0 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -90,7 +90,14 @@ static void sg_proc_cleanup(void);
*/
#define MULDIV(X,MUL,DIV) X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
+#define SG_DEFAULT_TIMEOUT_USER(60*USER_HZ) /* HZ == 'jiffies in 1
second' */
#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+#define SG_DEFAULT_RETRIES 0
+#define SG_DEF_FORCE_LOW_DMA 0
+#define SG_DEF_FORCE_PACK_ID 0
+#define SG_DEF_KEEP_ORPHAN 0
+#define SG_DEF_COMMAND_Q 0
+#define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ
int sg_big_buff = SG_DEF_RESERVED_SIZE;
/* N.B. This variable is readable and writeable via
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index 2bd9d67c201a..d54f9db2e079 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -1,60 +1,12 @@
#ifndef _SCSI_IOCTL_H
#define _SCSI_IOCTL_H
-#define SCSI_IOCTL_SEND_COMMAND 1
-#define SCSI_IOCTL_TEST_UNIT_READY 2
-#define SCSI_IOCTL_BENCHMARK_COMMAND 3
-#define SCSI_IOCTL_SYNC 4 /* Request synchronous
parameters */
-#define SCSI_IOCTL_START_UNIT 5
-#define SCSI_IOCTL_STOP_UNIT 6
-/* The door lock/unlock constants are compatible with Sun constants for
- the cdrom */
-#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
-#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
-
-/*
- * Here are some obsolete SCSI-specific ioctl commands.
- *
- * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
- */
-
-/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
-#define SCSI_IOCTL_GET_IDLUN 0x5382
-
-/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
-
-/* Used to obtain the host number of a device. */
-#define SCSI_IOCTL_PROBE_HOST 0x5385
-
-/* Used to obtain the bus number for a device */
-#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
-
-/* Used to obtain the PCI location of a device */
-#define SCSI_IOCTL_GET_PCI 0x5387
-
-#ifdef __KERNEL__
+#include
struct scsi_device;
-/*
- * Structures used for scsi_ioctl et al.
- */
-
-typedef struct scsi_ioctl_command {
- unsigned int inlen;
- unsigned int outlen;
- unsigned char data[0];
-} Scsi_Ioctl_Command;
-
-typedef struct scsi_idlun {
- __u32 dev_id;
- __u32 host_unique_id;
-} Scsi_Idlun;
-
-
int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev,
int cmd, bool ndelay);
extern int scsi_ioctl(struct scsi_device *, int, void __user *);
-#endif /* __KERNEL__ */
#endif /* _SCSI_IOCTL_H */
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 370c78c37926..f9d3d1dace41 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -2,267 +2,8 @@
#define _SCSI_GENERIC_H
#include
+#include
-/*
- * History:
- * Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
- * process control of SCSI devices.
- * Development Sponsored by Killy Corp. NY NY
- *
- * Original driver (sg.h):
- * Copyright (C) 1992 Lawrence Foard
- * Version 2 and 3 extensions to driver:
- * Copyright (C) 1998 - 2014 Douglas Gilbert
- *
- * Version: 3.5.36 (20140603)
- * This version is for 2.6 and 3 series kernels.
- *
- * Documentation
- * =
- * A web site for the SG device driver can be found at:
- * http://sg.danny.cz/sg [alternatively check the MAINTAINERS file]
- * The documentation for the sg version 3 driver can be found at:
- * http://sg.danny.cz/sg/p/sg_v3_ho.html
- * Also see: /Documentation/s
[PATCH v3 3/4] scsi: move all obsolete ioctls to scsi_ioctl.h
Some are in scsi.h. Keep them together in preparation for exposing them
in UAPI headers.
Cc: James Bottomley
Cc: Christoph Hellwig
Cc: linux-scsi@vger.kernel.org
Reviewed-by: Bart Van Assche
Signed-off-by: Paolo Bonzini
---
include/scsi/scsi.h | 20
include/scsi/scsi_ioctl.h | 20
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 5e2bafdbd96f..a96df31af89e 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -286,26 +286,6 @@ static inline int scsi_is_wlun(u64 lun)
#defineSCSI_REMOVAL_ALLOW 0
-/*
- * Here are some scsi specific ioctl commands which are sometimes useful.
- *
- * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
- */
-
-/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
-#define SCSI_IOCTL_GET_IDLUN 0x5382
-
-/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
-
-/* Used to obtain the host number of a device. */
-#define SCSI_IOCTL_PROBE_HOST 0x5385
-
-/* Used to obtain the bus number for a device */
-#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
-
-/* Used to obtain the PCI location of a device */
-#define SCSI_IOCTL_GET_PCI 0x5387
-
/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
static inline __u32 scsi_to_u32(__u8 *ptr)
{
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index c81962bef7a0..2bd9d67c201a 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -12,6 +12,26 @@
#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
+/*
+ * Here are some obsolete SCSI-specific ioctl commands.
+ *
+ * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
+ */
+
+/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
+#define SCSI_IOCTL_GET_IDLUN 0x5382
+
+/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
+
+/* Used to obtain the host number of a device. */
+#define SCSI_IOCTL_PROBE_HOST 0x5385
+
+/* Used to obtain the bus number for a device */
+#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
+
+/* Used to obtain the PCI location of a device */
+#define SCSI_IOCTL_GET_PCI 0x5387
+
#ifdef __KERNEL__
struct scsi_device;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] scsi: cleanup ioctl headers and provide UAPI versions
This is v3 of the series to provide an "official" sg.h header (and scsi_ioctl.h too, though it's basically obsolete) together with the other userspace API definitions. The change from v2 to v3 is that defaults for sg.c are not exported in include/uapi/linux/sg.c. Paolo 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] scsi: remove old-style type names from sg.h
These will not be exported by the new linux/sg.h header, and scsi/sg.h will
not have any user API after linux/sg.h is created. Since they have no
user in the kernel, they can be zapped.
Cc: James Bottomley
Cc: Christoph Hellwig
Cc: linux-scsi@vger.kernel.org
Reviewed-by: Bart Van Assche
Signed-off-by: Paolo Bonzini
---
include/scsi/sg.h | 6 --
1 file changed, 6 deletions(-)
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 3afec7032448..370c78c37926 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -207,12 +207,6 @@ typedef struct sg_req_info { /* used by
SG_GET_REQUEST_TABLE ioctl() */
#define SG_BIG_BUFF SG_DEF_RESERVED_SIZE/* for backward compatibility */
-/* Alternate style type names, "..._t" variants preferred */
-typedef struct sg_io_hdr Sg_io_hdr;
-typedef struct sg_io_vec Sg_io_vec;
-typedef struct sg_scsi_id Sg_scsi_id;
-typedef struct sg_req_info Sg_req_info;
-
/* vv */
/* The older SG interface based on the 'sg_header' structure follows. */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
On 17/09/2015 17:32, Bart Van Assche wrote: > >> + >> +#ifndef __KERNEL__ >> +/* Keep in sync with SG_DEFAULT_TIMEOUT of scsi/sg.h */ >> #define SG_DEFAULT_TIMEOUT(60*HZ) /* HZ == 'jiffies in 1 >> second' */ >> #endif > > Is it useful and/or necessary to export this constant ? To me this looks > like an implementation aspect rather than an aspect of the scsi-sg API. That's fine, I can remove it. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
Provide a UAPI version of the header in the kernel, making it easier
for interested projects to use an up-to-date version of the header.
The new headers are placed under uapi/linux/ so as not to conflict
with the glibc-provided headers in /usr/include/scsi.
struct scsi_ioctl_command and struct scsi_idlun used to be under
"#ifdef __KERNEL__", but they are actually useful for userspace as
well. Add them to the new header.
Signed-off-by: Paolo Bonzini
---
include/scsi/scsi_ioctl.h | 50 +-
include/scsi/sg.h | 260 +-
include/uapi/linux/Kbuild | 2 +
include/{scsi => uapi/linux}/scsi_ioctl.h | 23 +--
include/{scsi => uapi/linux}/sg.h | 20 +--
5 files changed, 20 insertions(+), 335 deletions(-)
copy include/{scsi => uapi/linux}/scsi_ioctl.h (77%)
copy include/{scsi => uapi/linux}/sg.h (97%)
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index 2bd9d67c201a..d54f9db2e079 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -1,60 +1,12 @@
#ifndef _SCSI_IOCTL_H
#define _SCSI_IOCTL_H
-#define SCSI_IOCTL_SEND_COMMAND 1
-#define SCSI_IOCTL_TEST_UNIT_READY 2
-#define SCSI_IOCTL_BENCHMARK_COMMAND 3
-#define SCSI_IOCTL_SYNC 4 /* Request synchronous
parameters */
-#define SCSI_IOCTL_START_UNIT 5
-#define SCSI_IOCTL_STOP_UNIT 6
-/* The door lock/unlock constants are compatible with Sun constants for
- the cdrom */
-#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
-#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
-
-/*
- * Here are some obsolete SCSI-specific ioctl commands.
- *
- * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
- */
-
-/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
-#define SCSI_IOCTL_GET_IDLUN 0x5382
-
-/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
-
-/* Used to obtain the host number of a device. */
-#define SCSI_IOCTL_PROBE_HOST 0x5385
-
-/* Used to obtain the bus number for a device */
-#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
-
-/* Used to obtain the PCI location of a device */
-#define SCSI_IOCTL_GET_PCI 0x5387
-
-#ifdef __KERNEL__
+#include
struct scsi_device;
-/*
- * Structures used for scsi_ioctl et al.
- */
-
-typedef struct scsi_ioctl_command {
- unsigned int inlen;
- unsigned int outlen;
- unsigned char data[0];
-} Scsi_Ioctl_Command;
-
-typedef struct scsi_idlun {
- __u32 dev_id;
- __u32 host_unique_id;
-} Scsi_Idlun;
-
-
int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev,
int cmd, bool ndelay);
extern int scsi_ioctl(struct scsi_device *, int, void __user *);
-#endif /* __KERNEL__ */
#endif /* _SCSI_IOCTL_H */
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 370c78c37926..1c80177b9793 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -2,267 +2,11 @@
#define _SCSI_GENERIC_H
#include
+#include
-/*
- * History:
- * Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
- * process control of SCSI devices.
- * Development Sponsored by Killy Corp. NY NY
- *
- * Original driver (sg.h):
- * Copyright (C) 1992 Lawrence Foard
- * Version 2 and 3 extensions to driver:
- * Copyright (C) 1998 - 2014 Douglas Gilbert
- *
- * Version: 3.5.36 (20140603)
- * This version is for 2.6 and 3 series kernels.
- *
- * Documentation
- * =
- * A web site for the SG device driver can be found at:
- * http://sg.danny.cz/sg [alternatively check the MAINTAINERS file]
- * The documentation for the sg version 3 driver can be found at:
- * http://sg.danny.cz/sg/p/sg_v3_ho.html
- * Also see: /Documentation/scsi/scsi-generic.txt
- *
- * For utility and test programs see: http://sg.danny.cz/sg/sg3_utils.html
- */
-
-#ifdef __KERNEL__
extern int sg_big_buff; /* for sysctl */
-#endif
-
-
-typedef struct sg_iovec /* same structure as used by readv() Linux system */
-{ /* call. It defines one scatter-gather element. */
-void __user *iov_base; /* Starting address */
-size_t iov_len; /* Length in bytes */
-} sg_iovec_t;
-
-
-typedef struct sg_io_hdr
-{
-int interface_id; /* [i] 'S' for SCSI generic (required) */
-int dxfer_direction;/* [i] data transfer direction */
-unsigned char cmd_len; /* [i] SCSI command length */
-unsigned char mx_sb_len;/* [i] max length to write to sbp */
-unsigned short iovec_count; /* [i] 0 implies no scatter gather */
-unsigned int dxfer_len; /* [i] byte count of data transfer */
-void __user *dxferp; /* [i], [*io] points to data transfer memory
- or scatter gather list */
-unsigned char __user *cmdp; /* [i], [*i] points to
[PATCH v2 2/4] scsi: cleanup scsi/scsi_ioctl.h
SCSI_REMOVAL_* goes together with other SCSI command constants in
include/scsi/scsi.h. It is also used outside the implementation
of the ioctls (and it is not part of the user API).
scsi_fctargaddress/Scsi_FCTargAddress has had no in-tree use since
commit ca61f10ab2b8 ("[SCSI] remove broken driver cpqfc", 2005-10-29).
Remove it, just in time for the the tenth anniversary of its demise.
Signed-off-by: Paolo Bonzini
---
include/scsi/scsi.h | 6 ++
include/scsi/scsi_ioctl.h | 8
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index e0a3398b1547..5e2bafdbd96f 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -279,6 +279,12 @@ static inline int scsi_is_wlun(u64 lun)
#define SCSI_INQ_PQ_NOT_CON 0x01
#define SCSI_INQ_PQ_NOT_CAP 0x03
+/*
+ * PREVENT/ALLOW MEDIUM REMOVAL
+ */
+#defineSCSI_REMOVAL_PREVENT1
+#defineSCSI_REMOVAL_ALLOW 0
+
/*
* Here are some scsi specific ioctl commands which are sometimes useful.
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index 8d19d1d233c3..c81962bef7a0 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -12,9 +12,6 @@
#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
-#defineSCSI_REMOVAL_PREVENT1
-#defineSCSI_REMOVAL_ALLOW 0
-
#ifdef __KERNEL__
struct scsi_device;
@@ -34,11 +31,6 @@ typedef struct scsi_idlun {
__u32 host_unique_id;
} Scsi_Idlun;
-/* Fibre Channel WWN, port_id struct */
-typedef struct scsi_fctargaddress {
- __u32 host_port_id;
- unsigned char host_wwn[8]; // include NULL term.
-} Scsi_FCTargAddress;
int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev,
int cmd, bool ndelay);
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] scsi: remove old-style type names from sg.h
These will not be exported by the new linux/sg.h header, and scsi/sg.h will
not have any user API after linux/sg.h is created. Since they have no
user in the kernel, they can be zapped.
Signed-off-by: Paolo Bonzini
---
include/scsi/sg.h | 6 --
1 file changed, 6 deletions(-)
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 3afec7032448..370c78c37926 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -207,12 +207,6 @@ typedef struct sg_req_info { /* used by
SG_GET_REQUEST_TABLE ioctl() */
#define SG_BIG_BUFF SG_DEF_RESERVED_SIZE/* for backward compatibility */
-/* Alternate style type names, "..._t" variants preferred */
-typedef struct sg_io_hdr Sg_io_hdr;
-typedef struct sg_io_vec Sg_io_vec;
-typedef struct sg_scsi_id Sg_scsi_id;
-typedef struct sg_req_info Sg_req_info;
-
/* vv */
/* The older SG interface based on the 'sg_header' structure follows. */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] scsi: cleanup ioctl headers and provide UAPI versions
This is v2 of the series I sent a few days ago. A bit heavier on
the cleanups:
- export the structs for obsolete SCSI_IOCTL_SEND_COMMAND in
uapi/linux/scsi_ioctl.h
- reduce the kernel's scsi/scsi_ioctl.h even more
- remove obsolete types from uapi/linux/sg.h
- place all obsolete ioctls in uapi/linux/scsi_ioctl.h, even those
formerly in scsi/scsi.h [requested by hch]
... and reverting to the flat uapi/linux/ structure instead of
creating uapi/linux/scsi/.
Paolo
Paolo Bonzini (4):
scsi: remove old-style type names from sg.h
scsi: cleanup scsi/scsi_ioctl.h
scsi: move all obsolete ioctls to scsi_ioctl.h
scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
include/scsi/scsi.h | 20 +--
include/scsi/scsi_ioctl.h | 38 +
include/scsi/sg.h | 266 +-
include/uapi/linux/Kbuild | 2 +
include/{scsi => uapi/linux}/scsi_ioctl.h | 41 +++--
include/{scsi => uapi/linux}/sg.h | 26 +--
6 files changed, 38 insertions(+), 355 deletions(-)
copy include/{scsi => uapi/linux}/scsi_ioctl.h (51%)
copy include/{scsi => uapi/linux}/sg.h (95%)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] scsi: move all obsolete ioctls to scsi/scsi_ioctl.h
Some are in scsi/scsi.h. Keep them together in preparation for exposing
them in UAPI headers.
Suggested-by: Christoph Hellwig
Signed-off-by: Paolo Bonzini
---
include/scsi/scsi.h | 20
include/scsi/scsi_ioctl.h | 20
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 5e2bafdbd96f..a96df31af89e 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -286,26 +286,6 @@ static inline int scsi_is_wlun(u64 lun)
#defineSCSI_REMOVAL_ALLOW 0
-/*
- * Here are some scsi specific ioctl commands which are sometimes useful.
- *
- * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
- */
-
-/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
-#define SCSI_IOCTL_GET_IDLUN 0x5382
-
-/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
-
-/* Used to obtain the host number of a device. */
-#define SCSI_IOCTL_PROBE_HOST 0x5385
-
-/* Used to obtain the bus number for a device */
-#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
-
-/* Used to obtain the PCI location of a device */
-#define SCSI_IOCTL_GET_PCI 0x5387
-
/* Pull a u32 out of a SCSI message (using BE SCSI conventions) */
static inline __u32 scsi_to_u32(__u8 *ptr)
{
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index c81962bef7a0..2bd9d67c201a 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -12,6 +12,26 @@
#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
+/*
+ * Here are some obsolete SCSI-specific ioctl commands.
+ *
+ * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
+ */
+
+/* Used to obtain PUN and LUN info. Conflicts with CDROMAUDIOBUFSIZ */
+#define SCSI_IOCTL_GET_IDLUN 0x5382
+
+/* 0x5383 and 0x5384 were used for SCSI_IOCTL_TAGGED_{ENABLE,DISABLE} */
+
+/* Used to obtain the host number of a device. */
+#define SCSI_IOCTL_PROBE_HOST 0x5385
+
+/* Used to obtain the bus number for a device */
+#define SCSI_IOCTL_GET_BUS_NUMBER 0x5386
+
+/* Used to obtain the PCI location of a device */
+#define SCSI_IOCTL_GET_PCI 0x5387
+
#ifdef __KERNEL__
struct scsi_device;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
On 16/09/2015 16:23, Christoph Hellwig wrote: > > I'm trying to cater to the objections that were made to Andy's patch. > > If you mean the non-UAPI headers, James wanted them to stay in scsi/; if > > you mean the UAPI headers, Douglas complained about the flat structure > > of include/linux/. > > Yes, keep the non-UAPI ones as is and move the UAPI ones to linux/ - the flat > structure is a feature, not a bug. > > > > Also scsi/scsi.h has some additional ioctl defintions that should be > > > added to the UAPI scsi_ioctl.h. Otherwise this looks ok to me. > > > > I can do this, but I think they are obsoleted by SG_GET_SCSI_ID, except > > for SCSI_IOCTL_GET_PCI which is horrible anyway. :) The comment about > > conflicts with CDROM ioctls is also interesting. I can see why one > > would want to keep them out of the new canonical place for SCSI ioctls. > > No point in hiding them. We can still officially deprecate them, but I'd > much rather have them in a single place than hidden away somewhere. Ok, will do both. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
On 16/09/2015 15:39, Christoph Hellwig wrote: > Hi Paolo, > > can you just move them to include/linux/ directly? I'm trying to cater to the objections that were made to Andy's patch. If you mean the non-UAPI headers, James wanted them to stay in scsi/; if you mean the UAPI headers, Douglas complained about the flat structure of include/linux/. > Also scsi/scsi.h has some additional ioctl defintions that should be > added to the UAPI scsi_ioctl.h. Otherwise this looks ok to me. I can do this, but I think they are obsoleted by SG_GET_SCSI_ID, except for SCSI_IOCTL_GET_PCI which is horrible anyway. :) The comment about conflicts with CDROM ioctls is also interesting. I can see why one would want to keep them out of the new canonical place for SCSI ioctls. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi: provide UAPI version of scsi/sg.h and scsi/scsi_ioctl.h
Provide a UAPI version of the header in the kernel, making it easier
for interested projects to use an up-to-date version of the header.
The new headers are placed under uapi/linux/scsi/ so as not to conflict
with the glibc-provided headers in /usr/include/scsi.
Cc: Andy Grover
Cc: James Bottomley
Cc: Christoph Hellwig
Cc: Douglas Gilbert
Cc: Linux SCSI List
Signed-off-by: Paolo Bonzini
---
Similar to Andy Grover's patch from last January, though developed
independently. Based on the remarks to Andy's patch, I'm leaving
scsi.h aside (Christoph), leaving the internal header in scsi/sg.h
(James) and moving the headers to include/linux/scsi/ (Douglas).
MAINTAINERS| 1 +
include/scsi/scsi_ioctl.h | 17 +-
include/scsi/sg.h | 267 +
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/scsi/Kbuild | 2 +
include/{ => uapi/linux}/scsi/scsi_ioctl.h | 36 +---
include/{ => uapi/linux}/scsi/sg.h | 18 +-
7 files changed, 17 insertions(+), 325 deletions(-)
create mode 100644 include/uapi/linux/scsi/Kbuild
copy include/{ => uapi/linux}/scsi/scsi_ioctl.h (41%)
copy include/{ => uapi/linux}/scsi/sg.h (97%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 67a4443daed9..b40764d43dde 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9173,6 +9173,7 @@ T:git
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
S: Maintained
F: drivers/scsi/
F: include/scsi/
+F: include/uapi/linux/scsi/
SCSI TAPE DRIVER
M: Kai Mäkisara
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index 8d19d1d233c3..5b07a0a91675 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -1,21 +1,7 @@
#ifndef _SCSI_IOCTL_H
#define _SCSI_IOCTL_H
-#define SCSI_IOCTL_SEND_COMMAND 1
-#define SCSI_IOCTL_TEST_UNIT_READY 2
-#define SCSI_IOCTL_BENCHMARK_COMMAND 3
-#define SCSI_IOCTL_SYNC 4 /* Request synchronous
parameters */
-#define SCSI_IOCTL_START_UNIT 5
-#define SCSI_IOCTL_STOP_UNIT 6
-/* The door lock/unlock constants are compatible with Sun constants for
- the cdrom */
-#define SCSI_IOCTL_DOORLOCK 0x5380 /* lock the eject mechanism */
-#define SCSI_IOCTL_DOORUNLOCK 0x5381 /* unlock the mechanism */
-
-#defineSCSI_REMOVAL_PREVENT1
-#defineSCSI_REMOVAL_ALLOW 0
-
-#ifdef __KERNEL__
+#include
struct scsi_device;
@@ -44,5 +30,4 @@ int scsi_ioctl_block_when_processing_errors(struct
scsi_device *sdev,
int cmd, bool ndelay);
extern int scsi_ioctl(struct scsi_device *, int, void __user *);
-#endif /* __KERNEL__ */
#endif /* _SCSI_IOCTL_H */
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 3afec7032448..87b35e7c4c31 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -1,274 +1,11 @@
#ifndef _SCSI_GENERIC_H
#define _SCSI_GENERIC_H
-#include
+#include
-/*
- * History:
- * Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
- * process control of SCSI devices.
- * Development Sponsored by Killy Corp. NY NY
- *
- * Original driver (sg.h):
- * Copyright (C) 1992 Lawrence Foard
- * Version 2 and 3 extensions to driver:
- * Copyright (C) 1998 - 2014 Douglas Gilbert
- *
- * Version: 3.5.36 (20140603)
- * This version is for 2.6 and 3 series kernels.
- *
- * Documentation
- * =
- * A web site for the SG device driver can be found at:
- * http://sg.danny.cz/sg [alternatively check the MAINTAINERS file]
- * The documentation for the sg version 3 driver can be found at:
- * http://sg.danny.cz/sg/p/sg_v3_ho.html
- * Also see: /Documentation/scsi/scsi-generic.txt
- *
- * For utility and test programs see: http://sg.danny.cz/sg/sg3_utils.html
- */
-
-#ifdef __KERNEL__
extern int sg_big_buff; /* for sysctl */
-#endif
-
-
-typedef struct sg_iovec /* same structure as used by readv() Linux system */
-{ /* call. It defines one scatter-gather element. */
-void __user *iov_base; /* Starting address */
-size_t iov_len; /* Length in bytes */
-} sg_iovec_t;
-
-
-typedef struct sg_io_hdr
-{
-int interface_id; /* [i] 'S' for SCSI generic (required) */
-int dxfer_direction;/* [i] data transfer direction */
-unsigned char cmd_len; /* [i] SCSI command length */
-unsigned char mx_sb_len;/* [i] max length to write to sbp */
-unsigned short iovec_count; /* [i] 0 implies no scatter gather */
-unsigned int dxfer_len; /* [i] byte count of data transfer */
-void __user *dxferp; /* [i], [*io] points to data transfer memory
- or scatter gather list */
-unsigned char __user *cmdp; /* [i], [*i] points to command to perform */
-void __user *sbp; /* [i
Re: [PATCH v2] virtio_scsi: don't select CONFIG_BLK_DEV_INTEGRITY
On 27/04/2015 14:56, Christoph Hellwig wrote:
> T10 PI is just another optional feature, LLDDs should work without
> the infrastructure.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/Kconfig | 1 -
> drivers/scsi/virtio_scsi.c | 11 ++-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index b021bcb..896bea6 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1743,7 +1743,6 @@ config SCSI_BFA_FC
> config SCSI_VIRTIO
> tristate "virtio-scsi support"
> depends on VIRTIO
> - select BLK_DEV_INTEGRITY
> help
>This is the virtual HBA driver for virtio. If the kernel will
>be used in a virtual machine, say Y or M.
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index f164f24..285f775 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -501,6 +501,7 @@ static void virtio_scsi_init_hdr(struct virtio_device
> *vdev,
> cmd->crn = 0;
> }
>
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
> struct virtio_scsi_cmd_req_pi *cmd_pi,
> struct scsi_cmnd *sc)
> @@ -524,6 +525,7 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device
> *vdev,
> blk_rq_sectors(rq) *
> bi->tuple_size);
> }
> +#endif
>
> static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>struct virtio_scsi_vq *req_vq,
> @@ -546,11 +548,14 @@ static int virtscsi_queuecommand(struct virtio_scsi
> *vscsi,
>
> BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
> virtio_scsi_init_hdr_pi(vscsi->vdev, &cmd->req.cmd_pi, sc);
> memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
> req_size = sizeof(cmd->req.cmd_pi);
> - } else {
> + } else
> +#endif
> + {
> virtio_scsi_init_hdr(vscsi->vdev, &cmd->req.cmd, sc);
> memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
> req_size = sizeof(cmd->req.cmd);
> @@ -1002,6 +1007,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
> shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> shost->nr_hw_queues = num_queues;
>
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
> host_prot = SHOST_DIF_TYPE1_PROTECTION |
> SHOST_DIF_TYPE2_PROTECTION |
> SHOST_DIF_TYPE3_PROTECTION |
> SHOST_DIX_TYPE1_PROTECTION |
> @@ -1010,6 +1016,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
> scsi_host_set_prot(shost, host_prot);
> scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> }
> +#endif
>
> err = scsi_add_host(shost, &vdev->dev);
> if (err)
> @@ -1090,7 +1097,9 @@ static struct virtio_device_id id_table[] = {
> static unsigned int features[] = {
> VIRTIO_SCSI_F_HOTPLUG,
> VIRTIO_SCSI_F_CHANGE,
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> VIRTIO_SCSI_F_T10_PI,
> +#endif
> };
>
> static struct virtio_driver virtio_scsi_driver = {
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] virtio_scsi: don't select CONFIG_BLK_DEV_INTEGRITY
On 23/04/2015 20:10, Christoph Hellwig wrote:
> T10 PI is just another optional feature, LLDDs should work without
> the infrastructure.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/Kconfig | 1 -
> drivers/scsi/virtio_scsi.c | 11 ++-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index b021bcb..896bea6 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1743,7 +1743,6 @@ config SCSI_BFA_FC
> config SCSI_VIRTIO
> tristate "virtio-scsi support"
> depends on VIRTIO
> - select BLK_DEV_INTEGRITY
> help
>This is the virtual HBA driver for virtio. If the kernel will
>be used in a virtual machine, say Y or M.
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index f164f24..0f2bb5b 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -501,6 +501,7 @@ static void virtio_scsi_init_hdr(struct virtio_device
> *vdev,
> cmd->crn = 0;
> }
>
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
> struct virtio_scsi_cmd_req_pi *cmd_pi,
> struct scsi_cmnd *sc)
> @@ -524,6 +525,7 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device
> *vdev,
> blk_rq_sectors(rq) *
> bi->tuple_size);
> }
> +#endif
>
> static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>struct virtio_scsi_vq *req_vq,
> @@ -546,11 +548,14 @@ static int virtscsi_queuecommand(struct virtio_scsi
> *vscsi,
>
> BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
> virtio_scsi_init_hdr_pi(vscsi->vdev, &cmd->req.cmd_pi, sc);
> memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
> req_size = sizeof(cmd->req.cmd_pi);
> - } else {
> + } else
> +#endif
> + {
All good so far.
> virtio_scsi_init_hdr(vscsi->vdev, &cmd->req.cmd, sc);
> memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
> req_size = sizeof(cmd->req.cmd);
> @@ -1002,6 +1007,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
> shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> shost->nr_hw_queues = num_queues;
>
> +#ifdef VIRTIO_SCSI_F_T10_PI
This symbol is always defined; it is part of the uapi/ header.
I think you wanted #ifdef CONFIG_BLK_DEV_INTEGRITY here as well.
However, you can remove this #ifdef completely. This is because...
> if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
... this condition should always be false unless the feature is
negotiated, and the feature should not be negotiated when
CONFIG_BLK_DEV_INTEGRITY is disabled.
Maybe you can add a "WARN_ON(!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY))"
depending on your taste.
> host_prot = SHOST_DIF_TYPE1_PROTECTION |
> SHOST_DIF_TYPE2_PROTECTION |
> SHOST_DIF_TYPE3_PROTECTION |
> SHOST_DIX_TYPE1_PROTECTION |
Also, if you leave the #ifdef, host_prot is now unused if
CONFIG_BLK_DEV_INTEGRITY is disabled. If you prefer to keep the #ifdef
you should also change host_prot to a macro (e.g. VIRTIO_SCSI_HOST_PROT)
to silence the compiler warning.
> @@ -1010,6 +1016,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
> scsi_host_set_prot(shost, host_prot);
> scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> }
> +#endif
>
> err = scsi_add_host(shost, &vdev->dev);
> if (err)
> @@ -1090,7 +1097,9 @@ static struct virtio_device_id id_table[] = {
> static unsigned int features[] = {
> VIRTIO_SCSI_F_HOTPLUG,
> VIRTIO_SCSI_F_CHANGE,
> +#ifdef VIRTIO_SCSI_F_T10_PI
This one definitely should be #ifdef CONFIG_BLK_DEV_INTEGRITY.
Thanks,
Paolo
> VIRTIO_SCSI_F_T10_PI,
> +#endif
> };
>
> static struct virtio_driver virtio_scsi_driver = {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down
On 24/03/2015 11:16, Fam Zheng wrote: > If issued right after link down, "blockdev --rereadpt" will be stuck for a > while and then return normally. Although the underlying capacity and partition > table are not correctly updated. And it means that userspace can't detect the > error at all. > > Fix this by propargating the error of "read capacity" command through the > stack, so that the ioctl could fail with -EIO. > > Fam Zheng (3): > block: Return error in rescan_partitions if revalidating disk failed > sd: Return error in sd_revalidate_disk if read capacity failed > sd: Return -EIO if read capacity failed > > block/partition-generic.c | 6 +++--- > drivers/scsi/sd.c | 22 +- > 2 files changed, 16 insertions(+), 12 deletions(-) > Reviewed-by: Paolo Bonzini Though patch 3 could be seen as a change in userspace ABI, so I'm less sure about it. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 05/03/2015 14:37, Paolo Bonzini wrote: > > > On 05/03/2015 14:33, Christoph Hellwig wrote: >> Any chance to get reviews for this series? Also we should at least >> expedite this first patch into 4.0-rc as it fixes scanning races >> in virtio_scsi. > > I reviewed 1 and 3, but I'm not really qualified for patch 2. Christoph, any news about these patches? Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 05/03/2015 14:33, Christoph Hellwig wrote:
> Any chance to get reviews for this series? Also we should at least
> expedite this first patch into 4.0-rc as it fixes scanning races
> in virtio_scsi.
I reviewed 1 and 3, but I'm not really qualified for patch 2.
Paolo
> On Mon, Feb 02, 2015 at 02:01:24PM +0100, Christoph Hellwig wrote:
>> Lock the device embedded in the scsi_device to protect against
>> concurrent calls to ->remove.
>>
>> Signed-off-by: Christoph Hellwig
>> Acked-by: Alan Stern
>> ---
>> drivers/scsi/scsi_scan.c | 7 +++
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 983aed1..523faee 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
>>
>> void scsi_rescan_device(struct device *dev)
>> {
>> -if (!dev->driver)
>> -return;
>> -
>> -if (try_module_get(dev->driver->owner)) {
>> +device_lock(dev);
>> +if (dev->driver && try_module_get(dev->driver->owner)) {
>> struct scsi_driver *drv = to_scsi_driver(dev->driver);
>>
>> if (drv->rescan)
>> drv->rescan(dev);
>> module_put(dev->driver->owner);
>> }
>> +device_unlock(dev);
>> }
>> EXPORT_SYMBOL(scsi_rescan_device);
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> ---end quoted text---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 02/02/2015 14:01, Christoph Hellwig wrote:
> Lock the device embedded in the scsi_device to protect against
> concurrent calls to ->remove.
>
> Signed-off-by: Christoph Hellwig
> Acked-by: Alan Stern
> ---
> drivers/scsi/scsi_scan.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 983aed1..523faee 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
>
> void scsi_rescan_device(struct device *dev)
> {
> - if (!dev->driver)
> - return;
> -
> - if (try_module_get(dev->driver->owner)) {
> + device_lock(dev);
> + if (dev->driver && try_module_get(dev->driver->owner)) {
> struct scsi_driver *drv = to_scsi_driver(dev->driver);
>
> if (drv->rescan)
> drv->rescan(dev);
> module_put(dev->driver->owner);
> }
> + device_unlock(dev);
> }
> EXPORT_SYMBOL(scsi_rescan_device);
>
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] scsi: proper state checking and module refcount handling in scsi_device_get
On 02/02/2015 14:01, Christoph Hellwig wrote:
> This effectively reverts commits 85b6c7 ("[SCSI] sd: fix cache flushing on
> module removal (and individual device removal)" and dc4515ea ("scsi: always
> increment reference count").
>
> We now never call scsi_device_get from the shutdown path, and the fact
> that we started grabbing reference there in commit 85b6c7 turned out
> turned out to create more problems than it solves, and required
> workarounds for workarounds for workarounds. Move back to properly checking
> the device state and carefully handle module refcounting.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/scsi.c | 20 +---
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 9b38299..9b7fd0b 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -979,18 +979,24 @@ EXPORT_SYMBOL(scsi_report_opcode);
> * Description: Gets a reference to the scsi_device and increments the use
> count
> * of the underlying LLDD module. You must hold host_lock of the
> * parent Scsi_Host or already have a reference when calling this.
> + *
> + * This will fail if a device is deleted or cancelled, or when the LLD module
> + * is in the process of being unloaded.
> */
> int scsi_device_get(struct scsi_device *sdev)
> {
> - if (sdev->sdev_state == SDEV_DEL)
> - return -ENXIO;
> + if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
> + goto fail;
> if (!get_device(&sdev->sdev_gendev))
> - return -ENXIO;
> - /* We can fail try_module_get if we're doing SCSI operations
> - * from module exit (like cache flush) */
> - __module_get(sdev->host->hostt->module);
> -
> + goto fail;
> + if (!try_module_get(sdev->host->hostt->module))
> + goto fail_put_device;
> return 0;
> +
> +fail_put_device:
> + put_device(&sdev->sdev_gendev);
> +fail:
> + return -ENXIO;
> }
> EXPORT_SYMBOL(scsi_device_get);
>
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 02/02/2015 13:59, Christoph Hellwig wrote: > On Fri, Jan 30, 2015 at 10:46:17AM +0100, Paolo Bonzini wrote: >> > Great, we might want to revert that patch in 3.21. > Is that fix in any tree yet? Seems like I missed it for the scsi > tree at least. So unless you want it for 3.19/stable we might as well > ust skip that patch. Yes, I agree. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 30/01/2015 02:08, Fam Zheng wrote: > On Fri, 01/30 00:11, Paolo Bonzini wrote: >> >> >> On 29/01/2015 00:00, Christoph Hellwig wrote: >>> Lock the device embedded in the scsi_device to protect against >>> concurrent calls to ->remove. >>> >>> Signed-off-by: Christoph Hellwig >> >> I wonder if this makes this problem: https://lkml.org/lkml/2015/1/5/9 go >> away. > > A quick test says yes. Great, we might want to revert that patch in 3.21. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] lib/iovec: Add memcpy_fromiovec_out library function
On 30/01/2015 09:12, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger
>
> This patch adds a new memcpy_fromiovec_out() library function which modifies
> the passed *iov following memcpy_fromiovec(), but also returns the next
> current
> iovec pointer via **iov_out.
>
> This is useful for vhost ANY_LAYOUT support when guests are allowed to
> generate
> incoming virtio request headers combined with subsequent SGL payloads into a
> single iovec.
>
> Cc: Michael S. Tsirkin
> Cc: Paolo Bonzini
> Signed-off-by: Nicholas Bellinger
> ---
> include/linux/uio.h | 2 ++
> lib/iovec.c | 27 +++
> 2 files changed, 29 insertions(+)
>
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 1c5e453..3e4473d 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -136,6 +136,8 @@ size_t csum_and_copy_to_iter(void *addr, size_t bytes,
> __wsum *csum, struct iov_
> size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum,
> struct iov_iter *i);
>
> int memcpy_fromiovec(unsigned char *kdata, struct iovec *iov, int len);
> +int memcpy_fromiovec_out(unsigned char *kdata, struct iovec *iov,
> + struct iovec **iov_out, int len);
> int memcpy_fromiovecend(unsigned char *kdata, const struct iovec *iov,
> int offset, int len);
> int memcpy_toiovecend(const struct iovec *v, unsigned char *kdata,
> diff --git a/lib/iovec.c b/lib/iovec.c
> index 2d99cb4..6a813dd 100644
> --- a/lib/iovec.c
> +++ b/lib/iovec.c
> @@ -28,6 +28,33 @@ int memcpy_fromiovec(unsigned char *kdata, struct iovec
> *iov, int len)
> EXPORT_SYMBOL(memcpy_fromiovec);
>
> /*
> + * Copy iovec to kernel, saving the current iov to *iov_out.
> + * Returns -EFAULT on error.
Perhaps document that iov is modified, zeroing everything in [iov,
*iov_out) and possibly removing the front of *iov_out?
Paolo
> + */
> +
> +int memcpy_fromiovec_out(unsigned char *kdata, struct iovec *iov,
> + struct iovec **iov_out, int len)
> +{
> + while (len > 0) {
> + if (iov->iov_len) {
> + int copy = min_t(unsigned int, len, iov->iov_len);
> + if (copy_from_user(kdata, iov->iov_base, copy))
> + return -EFAULT;
> + len -= copy;
> + kdata += copy;
> + iov->iov_base += copy;
> + iov->iov_len -= copy;
> + }
> + if (!iov->iov_len)
> + iov++;
> + }
> + *iov_out = iov;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(memcpy_fromiovec_out);
> +
> +/*
> * Copy kernel to iovec. Returns -EFAULT on error.
> */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: serialize ->rescan against ->remove
On 29/01/2015 00:00, Christoph Hellwig wrote:
> Lock the device embedded in the scsi_device to protect against
> concurrent calls to ->remove.
>
> Signed-off-by: Christoph Hellwig
I wonder if this makes this problem: https://lkml.org/lkml/2015/1/5/9 go
away.
Paolo
> ---
> drivers/scsi/scsi_scan.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 983aed1..523faee 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1593,16 +1593,15 @@ EXPORT_SYMBOL(scsi_add_device);
>
> void scsi_rescan_device(struct device *dev)
> {
> - if (!dev->driver)
> - return;
> -
> - if (try_module_get(dev->driver->owner)) {
> + device_lock(dev);
> + if (dev->driver && try_module_get(dev->driver->owner)) {
> struct scsi_driver *drv = to_scsi_driver(dev->driver);
>
> if (drv->rescan)
> drv->rescan(dev);
> module_put(dev->driver->owner);
> }
> + device_unlock(dev);
> }
> EXPORT_SYMBOL(scsi_rescan_device);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] scsi: Fix max transfer length for 4k disks
On 29/01/2015 22:54, Brian King wrote: > The following patch fixes an issue observed with 4k sector disks > where the max_hw_sectors attribute was getting set too large in > sd_revalidate_disk. Since sdkp->max_xfer_blocks is in units > of SCSI logical blocks and queue_max_hw_sectors is in units of > 512 byte blocks, on a 4k sector disk, every time we went through > sd_revalidate_disk, we were taking the current value of > queue_max_hw_sectors and increasing it by a factor of 8. Fix > this by only shifting sdkp->max_xfer_blocks. > > Cc: stable > Signed-off-by: Brian King > --- > > drivers/scsi/sd.c |6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff -puN drivers/scsi/sd.c~sd_revalidate_4k drivers/scsi/sd.c > --- linux/drivers/scsi/sd.c~sd_revalidate_4k 2015-01-29 14:44:23.316171187 > -0600 > +++ linux-bjking1/drivers/scsi/sd.c 2015-01-29 14:51:05.846126392 -0600 > @@ -2800,9 +2800,11 @@ static int sd_revalidate_disk(struct gen >*/ > sd_set_flush_flag(sdkp); > > - max_xfer = min_not_zero(queue_max_hw_sectors(sdkp->disk->queue), > - sdkp->max_xfer_blocks); > + max_xfer = sdkp->max_xfer_blocks; > max_xfer <<= ilog2(sdp->sector_size) - 9; > + > + max_xfer = min_not_zero(queue_max_hw_sectors(sdkp->disk->queue), > + max_xfer); > blk_queue_max_hw_sectors(sdkp->disk->queue, max_xfer); > set_capacity(disk, sdkp->capacity); > sd_config_write_same(sdkp); > _ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Reviewed-by: Paolo Bonzini -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] virtio-scsi: Fix the race condition in virtscsi_handle_event
On 08/01/2015 03:57, Fam Zheng wrote:
> There is a race condition in virtscsi_handle_event, when many device
> hotplug/unplug events flush in quickly.
>
> The scsi_remove_device in virtscsi_handle_transport_reset may trigger
> the BUG_ON in scsi_target_reap, because the state is altered behind it,
> probably by scsi_scan_host of another event. I'm able to reproduce it by
> repeatedly plugging and unplugging a scsi disk with the same lun number.
>
> To fix this, a single thread workqueue (local to the module) is added,
> which makes the scan work serialized. With this change, the panic goes
> away.
>
> Signed-off-by: Fam Zheng
Reviewed-by: Paolo Bonzini
> ---
>
> v4: Addressing MST's comments:
> Use ordered workqueue, with WQ_FREEZABLE and WQ_MEM_RECLAIM flags.
> Coding style fixes.
>
> v3: Fix spacing and destroy order. (MST)
> ---
> drivers/scsi/virtio_scsi.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index c52bb5d..0db63b5 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -120,6 +120,7 @@ struct virtio_scsi {
>
> static struct kmem_cache *virtscsi_cmd_cache;
> static mempool_t *virtscsi_cmd_pool;
> +static struct workqueue_struct *virtscsi_scan_wq;
>
> static inline struct Scsi_Host *virtio_scsi_host(struct virtio_device *vdev)
> {
> @@ -404,7 +405,7 @@ static void virtscsi_complete_event(struct virtio_scsi
> *vscsi, void *buf)
> struct virtio_scsi_event_node *event_node = buf;
>
> if (!vscsi->stop_events)
> - queue_work(system_freezable_wq, &event_node->work);
> + queue_work(virtscsi_scan_wq, &event_node->work);
> }
>
> static void virtscsi_event_done(struct virtqueue *vq)
> @@ -1119,6 +1120,14 @@ static int __init init(void)
> pr_err("mempool_create() for virtscsi_cmd_pool failed\n");
> goto error;
> }
> +
> + virtscsi_scan_wq =
> + alloc_ordered_workqueue("virtscsi-scan", WQ_FREEZABLE |
> WQ_MEM_RECLAIM);
> + if (!virtscsi_scan_wq) {
> + pr_err("create_singlethread_workqueue() for virtscsi_scan_wq
> failed\n");
> + goto error;
> + }
> +
> ret = register_virtio_driver(&virtio_scsi_driver);
> if (ret < 0)
> goto error;
> @@ -1126,6 +1135,8 @@ static int __init init(void)
> return 0;
>
> error:
> + if (virtscsi_scan_wq)
> + destroy_workqueue(virtscsi_scan_wq);
> if (virtscsi_cmd_pool) {
> mempool_destroy(virtscsi_cmd_pool);
> virtscsi_cmd_pool = NULL;
> @@ -1140,6 +1151,7 @@ error:
> static void __exit fini(void)
> {
> unregister_virtio_driver(&virtio_scsi_driver);
> + destroy_workqueue(virtscsi_scan_wq);
> mempool_destroy(virtscsi_cmd_pool);
> kmem_cache_destroy(virtscsi_cmd_cache);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] libata: Whitelist SSDs that are known to properly return zeroes after TRIM
On 07/11/2014 06:08, Martin K. Petersen wrote:
> The whitelist is only meant as a starting point and is by no means
> comprehensive:
>
>- All intel SSD models except for 510
>- Micron M5*
>- Samsung SSDs
>- Seagate SSDs
>
> Signed-off-by: Martin K. Petersen
> ---
> drivers/ata/libata-core.c | 18 ++
> drivers/ata/libata-scsi.c | 10 ++
> include/linux/libata.h| 1 +
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index c5ba15af87d3..f41f24a8bc21 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4225,10 +4225,20 @@ static const struct ata_blacklist_entry
> ata_device_blacklist [] = {
> { "PIONEER DVD-RW DVR-216D", NULL, ATA_HORKAGE_NOSETXFER },
>
> /* devices that don't properly handle queued TRIM commands */
> - { "Micron_M500*", NULL, ATA_HORKAGE_NO_NCQ_TRIM, },
> - { "Crucial_CT???M500SSD*", NULL, ATA_HORKAGE_NO_NCQ_TRIM, },
> - { "Micron_M550*", NULL, ATA_HORKAGE_NO_NCQ_TRIM, },
> - { "Crucial_CT*M550SSD*",NULL, ATA_HORKAGE_NO_NCQ_TRIM, },
> + { "Micron_M5?0*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
> + ATA_HORKAGE_ZERO_AFTER_TRIM, },
> + { "Crucial_CT???M5?0SSD*", NULL, ATA_HORKAGE_NO_NCQ_TRIM, },
I have a Crucial_CT256MX1 (i.e. MX100) and it does reliably zero.
BTW. it's the same hardware as the M550, so probably the same set of
quirks should apply to both.
Paolo
> +
> + /*
> + * DRAT/RZAT are weak guarantees. Explicitly black/whitelist
> + * SSDs that provide reliable zero after TRIM.
> + */
> + { "INTEL*SSDSC2MH*",NULL, 0, }, /* Blacklist intel 510 */
> + { "INTEL*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
> + { "SSD*INTEL*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
> + { "Samsung*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
> + { "SAMSUNG*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
> + { "ST[1248][0248]0[FH]*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
>
> /*
>* Some WD SATA-I drives spin up and down erratically when the link
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 0586f66d70fa..deaa6e34ed4d 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2515,13 +2515,15 @@ static unsigned int ata_scsiop_read_cap(struct
> ata_scsi_args *args, u8 *rbuf)
> rbuf[15] = lowest_aligned;
>
> if (ata_id_has_trim(args->id)) {
> - rbuf[14] |= 0x80; /* TPE */
> + rbuf[14] |= 0x80; /* LBPME */
>
> - if (ata_id_has_zero_after_trim(args->id))
> - rbuf[14] |= 0x40; /* TPRZ */
> + if (ata_id_has_zero_after_trim(args->id) &&
> + dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM) {
> + ata_dev_warn(dev, "Enabling
> discard_zeroes_data\n");
> + rbuf[14] |= 0x40; /* LBPRZ */
> + }
> }
> }
> -
> return 0;
> }
>
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index bd5fefeaf548..45ac825b8366 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -421,6 +421,7 @@ enum {
> ATA_HORKAGE_NO_NCQ_TRIM = (1 << 19),/* don't use queued TRIM */
> ATA_HORKAGE_NOLPM = (1 << 20),/* don't use LPM */
> ATA_HORKAGE_WD_BROKEN_LPM = (1 << 21), /* some WDs have broken LPM */
> + ATA_HORKAGE_ZERO_AFTER_TRIM = (1 << 22),/* guarantees zero after trim */
>
>/* DMA mask for user DMA control: User visible values; DO NOT
> renumber */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On 05/12/2014 14:05, Ming Lei wrote: > > [ 50.112885] sd 0:0:1:0: [sda] FAILED Result: hostbyte=DID_OK > driverbyte=DRIVER_SENSE > [ 50.113859] sd 0:0:1:0: [sda] Sense Key : Illegal Request [current] > [ 50.113859] sd 0:0:1:0: [sda] Add. Sense: Invalid field in cdb > [ 50.113859] sd 0:0:1:0: [sda] CDB: > [ 50.113859] Write same(16): 93 08 00 00 00 00 00 00 80 00 00 40 00 00 00 00 > [ 50.113859] blk_update_request: critical target error, dev sda, sector > 32768 So this command is zeroing 2^22 sectors (2GB) starting from sector 128. How did you run QEMU and what command produced this request? Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On 05/12/2014 14:58, Martin K. Petersen wrote:
>>>>>> "Ming" == Ming Lei writes:
>
>>> What about in READ CAPACITY(16)?
>
> Ming> It isn't set too.
>
> Please try the following patch:
>
>
> [SCSI] sd: Tweak discard heuristics to work around QEMU SCSI issue
>
> 7985090aa020 changed the discard heuristics to give preference to the
> WRITE SAME commands that (unlike UNMAP) guarantee deterministic results.
>
> Ming Lei discovered that QEMU SCSI's WRITE SAME implementation
> internally relied on limits that were only communicated for the UNMAP
> case. And therefore discard commands backed by WRITE SAME would fail.
>
> Tweak the heuristics so we still pick UNMAP in the LBPRZ=0 case and only
> prefer the WRITE SAME variants if the device has the LBPRZ flag set.
>
> Reported-by: Ming Lei
> Signed-off-by: Martin K. Petersen
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 95bfb7bfbb9d..76c5d1388417 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2623,8 +2623,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
> sd_config_discard(sdkp, SD_LBP_WS16);
>
> } else {/* LBP VPD page tells us what to use */
> -
> - if (sdkp->lbpws)
> + if (sdkp->lbpu && sdkp->max_unmap_blocks &&
> !sdkp->lbprz)
> + sd_config_discard(sdkp, SD_LBP_UNMAP);
> + else if (sdkp->lbpws)
> sd_config_discard(sdkp, SD_LBP_WS16);
> else if (sdkp->lbpws10)
> sd_config_discard(sdkp, SD_LBP_WS10);
>
This is the right fix. Ming, how do you reproduce the QEMU bug?
Acked-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] scsi: add 'am53c974' driver
return;
> + }
> + esp->scsi_id = EEbuf[DC390_EE_ADAPT_SCSI_ID];
> + esp->num_tags = 2 << EEbuf[DC390_EE_TAG_CMD_NUM];
> +}
> +
> +static int pci_esp_probe_one(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct scsi_host_template *hostt = &scsi_esp_template;
> + int err = -ENODEV;
> + struct Scsi_Host *shost;
> + struct esp *esp;
> + struct pci_esp_priv *pep;
> +
> + if (pci_enable_device(pdev)) {
> + dev_printk(KERN_INFO, &pdev->dev, "cannot enable device\n");
> + return -ENODEV;
> + }
> +
> + if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32))) {
> + dev_printk(KERN_INFO, &pdev->dev,
> +"failed to set 32bit DMA mask\n");
> + goto fail_disable_device;
> + }
> +
> + shost = scsi_host_alloc(hostt, sizeof(struct esp));
> + if (!shost) {
> + dev_printk(KERN_INFO, &pdev->dev,
> +"failed to allocate scsi host\n");
> + err = -ENOMEM;
> + goto fail_disable_device;
> + }
> +
> + pep = kzalloc(sizeof(struct pci_esp_priv), GFP_KERNEL);
> + if (!pep) {
> + dev_printk(KERN_INFO, &pdev->dev,
> +"failed to allocate esp_priv\n");
> + err = -ENOMEM;
> + goto fail_host_alloc;
> + }
> +
> + esp = shost_priv(shost);
> + esp->host = shost;
> + esp->dev = pdev;
> + esp->ops = &pci_esp_ops;
> + /*
> + * The am53c974 HBA has a design flaw of generating
> + * spurious DMA completion interrupts when using
> + * DMA for command submission.
> + */
> + esp->flags |= ESP_FLAG_USE_FIFO;
> + pep->esp = esp;
> +
> + if (pci_request_regions(pdev, DRV_MODULE_NAME)) {
> + dev_printk(KERN_ERR, &pdev->dev,
> +"pci memory selection failed\n");
> + goto fail_priv_alloc;
> + }
> +
> + esp->regs = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> + if (!esp->regs) {
> + dev_printk(KERN_ERR, &pdev->dev, "pci I/O map failed\n");
> + goto fail_release_regions;
> + }
> + esp->dma_regs = esp->regs;
> +
> + pci_set_master(pdev);
> +
> + esp->command_block = pci_alloc_consistent(pdev, 16,
> + &esp->command_block_dma);
> + if (!esp->command_block) {
> + dev_printk(KERN_ERR, &pdev->dev,
> +"failed to allocate command block\n");
> + err = -ENOMEM;
> + goto fail_unmap_regs;
> + }
> +
> + if (request_irq(pdev->irq, scsi_esp_intr, IRQF_SHARED,
> + DRV_MODULE_NAME, esp)) {
> + dev_printk(KERN_ERR, &pdev->dev, "failed to register IRQ\n");
> + goto fail_unmap_command_block;
> + }
> +
> + esp->scsi_id = 7;
> + dc390_check_eeprom(esp);
> +
> + shost->this_id = esp->scsi_id;
> + shost->max_id = 8;
> + shost->irq = pdev->irq;
> + shost->io_port = pci_resource_start(pdev, 0);
> + shost->n_io_port = pci_resource_len(pdev, 0);
> + shost->unique_id = shost->io_port;
> + esp->scsi_id_mask = (1 << esp->scsi_id);
> + /* Assume 40MHz clock */
> + esp->cfreq = 4000;
> +
> + pci_set_drvdata(pdev, pep);
> +
> + err = scsi_esp_register(esp, &pdev->dev);
> + if (err)
> + goto fail_free_irq;
> +
> + return 0;
> +
> +fail_free_irq:
> + free_irq(pdev->irq, esp);
> +fail_unmap_command_block:
> + pci_free_consistent(pdev, 16, esp->command_block,
> + esp->command_block_dma);
> +fail_unmap_regs:
> + pci_iounmap(pdev, esp->regs);
> +fail_release_regions:
> + pci_release_regions(pdev);
> +fail_priv_alloc:
> + kfree(pep);
> +fail_host_alloc:
> + scsi_host_put(shost);
> +fail_disable_device:
> + pci_disable_device(pdev);
> +
> + return err;
> +}
> +
> +static void pci_esp_remove_one(struct pci_dev *pdev)
> +{
> + struct pci_esp_priv *pep = pci_get_drvdata(pdev);
> + struct esp *esp = pep->esp;
> +
> + scsi_esp_unregister(esp);
> + free_irq(pdev->irq, esp);
> + pci_free_consistent(pdev, 16, esp->command_block,
> + esp->command_block_dma);
> + pci_iounmap(pdev, esp->regs);
> + pci_release_regions(pdev);
> + pci_disable_device(pdev);
> + kfree(pep);
> +
> + scsi_host_put(esp->host);
> +}
> +
> +static struct pci_device_id am53c974_pci_tbl[] = {
> + { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_SCSI,
> + PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(pci, am53c974_pci_tbl);
> +
> +static struct pci_driver am53c974_driver = {
> + .name = DRV_MODULE_NAME,
> + .id_table = am53c974_pci_tbl,
> + .probe = pci_esp_probe_one,
> + .remove = pci_esp_remove_one,
> +};
> +
> +static int __init am53c974_module_init(void)
> +{
> + return pci_register_driver(&am53c974_driver);
> +}
> +
> +static void __exit am53c974_module_exit(void)
> +{
> + pci_unregister_driver(&am53c974_driver);
> +}
> +
> +MODULE_DESCRIPTION("AM53C974 SCSI driver");
> +MODULE_AUTHOR("Hannes Reinecke ");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_MODULE_VERSION);
> +
> +module_init(am53c974_module_init);
> +module_exit(am53c974_module_exit);
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] esp: enable CONFIG2_FENAB for am53c974
On 21/11/2014 13:41, Hannes Reinecke wrote:
> CONFIG2_FENAB ('feature enable') changed definition between chip
> revisions, from 'Latch SCSI Phase' to 'Latch SCSI Phase, display
> chip ID upon reset, and enable 24 bit addresses'.
> So only enable it for am53c974 where we know what it's doing.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 41 +
> drivers/scsi/esp_scsi.c | 2 ++
> 2 files changed, 43 insertions(+)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index 340cb2f..55622eb 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -17,6 +17,8 @@
> #define DRV_MODULE_NAME "am53c974"
> #define DRV_MODULE_VERSION "1.00"
>
> +static bool am53c974_fenab = true;
> +
> // #define ESP_DMA_DEBUG
>
> #define ESP_DMA_CMD 0x10
> @@ -254,6 +256,8 @@ static void pci_esp_send_dma_cmd(struct esp *esp, u32
> addr, u32 esp_count,
>
> pci_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> pci_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> + if (esp->config2 & ESP_CONFIG2_FENAB)
> + pci_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
> pci_esp_write32(esp, esp_count, ESP_DMA_STC);
> pci_esp_write32(esp, addr, ESP_DMA_SPA);
> @@ -267,6 +271,33 @@ static void pci_esp_send_dma_cmd(struct esp *esp, u32
> addr, u32 esp_count,
> pci_esp_write8(esp, ESP_DMA_CMD_START | val, ESP_DMA_CMD);
> }
>
> +static u32 pci_esp_dma_length_limit(struct esp *esp, u32 dma_addr, u32
> dma_len)
> +{
> + int dma_limit = 16;
> + u32 base, end;
> +
> + /*
> + * If CONFIG2_FENAB is set we can
> + * handle up to 24 bit addresses
> + */
> + if (esp->config2 & ESP_CONFIG2_FENAB)
> + dma_limit = 24;
> +
> + if (dma_len > (1U << dma_limit))
> + dma_len = (1U << dma_limit);
> +
> + /*
> + * Prevent crossing a 24-bit address boundary.
> + */
> + base = dma_addr & ((1U << 24) - 1U);
> + end = base + dma_len;
> + if (end > (1U << 24))
> + end = (1U <<24);
> + dma_len = end - base;
> +
> + return dma_len;
> +}
> +
> static const struct esp_driver_ops pci_esp_ops = {
> .esp_write8 = pci_esp_write8,
> .esp_read8 = pci_esp_read8,
> @@ -280,6 +311,7 @@ static const struct esp_driver_ops pci_esp_ops = {
> .dma_invalidate = pci_esp_dma_invalidate,
> .send_dma_cmd = pci_esp_send_dma_cmd,
> .dma_error = pci_esp_dma_error,
> + .dma_length_limit = pci_esp_dma_length_limit,
> };
>
> /*
> @@ -417,6 +449,12 @@ static int pci_esp_probe_one(struct pci_dev *pdev,
>* DMA for command submission.
>*/
> esp->flags |= ESP_FLAG_USE_FIFO;
> + /*
> + * Enable CONFIG2_FENAB to allow for large DMA transfers
> + */
> + if (am53c974_fenab)
> + esp->config2 |= ESP_CONFIG2_FENAB;
> +
> pep->esp = esp;
>
> if (pci_request_regions(pdev, DRV_MODULE_NAME)) {
> @@ -535,5 +573,8 @@ MODULE_AUTHOR("Hannes Reinecke ");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(DRV_MODULE_VERSION);
>
> +module_param(am53c974_fenab, bool, 0444);
> +MODULE_PARM_DESC(am53c974_fenab, "Enable 24-bit DMA transfer sizes");
> +
> module_init(am53c974_module_init);
> module_exit(am53c974_module_exit);
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bd17516..56f361e 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -1343,6 +1343,8 @@ static int esp_data_bytes_sent(struct esp *esp, struct
> esp_cmd_entry *ent,
> (((unsigned int)esp_read8(ESP_TCMED)) << 8));
> if (esp->rev == FASHME)
> ecount |= ((unsigned int)esp_read8(FAS_RLO)) << 16;
> + if (esp->rev == PCSCSI && (esp->config2 & ESP_CONFIG2_FENAB))
> + ecount |= ((unsigned int)esp_read8(ESP_TCHI)) << 16;
> }
>
> bytes_sent = esp->data_dma_len;
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/12] am53c974: BLAST residual handling
On 21/11/2014 13:41, Hannes Reinecke wrote:
> The am53c974 has an design issue where a single byte might be
> left in the SCSI FIFO after a DMA transfer.
> As the handling code is currently untested add a WARN_ON()
> statement here.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 6 ++
> drivers/scsi/esp_scsi.c | 29 +
> drivers/scsi/esp_scsi.h | 1 +
> 3 files changed, 36 insertions(+)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index 86c4c42..1c1312f 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -197,6 +197,12 @@ static void pci_esp_dma_drain(struct esp *esp)
> shost_printk(KERN_INFO, esp->host,
>"DMA blast done (%d tries, %d bytes left)\n", lim, resid);
> #endif
> + /* BLAST residual handling is currently untested */
> + if (WARN_ON_ONCE(resid == 1)) {
> + struct esp_cmd_entry *ent = esp->active_cmd;
> +
> + ent->flags |= ESP_CMD_FLAG_RESIDUAL;
> + }
> }
>
> static void pci_esp_dma_invalidate(struct esp *esp)
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 2a3277d..07b4d93 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -1334,6 +1334,35 @@ static int esp_data_bytes_sent(struct esp *esp, struct
> esp_cmd_entry *ent,
> bytes_sent = esp->data_dma_len;
> bytes_sent -= ecount;
>
> + /*
> + * The am53c974 has a DMA 'pecularity'. The doc states:
> + * In some odd byte conditions, one residual byte will
> + * be left in the SCSI FIFO, and the FIFO Flags will
> + * never count to '0 '. When this happens, the residual
> + * byte should be retrieved via PIO following completion
> + * of the BLAST operation.
> + */
> + if (fifo_cnt == 1 && ent->flags & ESP_CMD_FLAG_RESIDUAL) {
> + size_t count = 1;
> + size_t offset = bytes_sent;
> + u8 bval = esp_read8(ESP_FDATA);
> +
> + if (ent->flags & ESP_CMD_FLAG_AUTOSENSE)
> + ent->sense_ptr[bytes_sent] = bval;
> + else {
> + struct esp_cmd_priv *p = ESP_CMD_PRIV(cmd);
> + u8 *ptr;
> +
> + ptr = scsi_kmap_atomic_sg(p->cur_sg, p->u.num_sg,
> + &offset, &count);
> + if (likely(ptr)) {
> + *(ptr + offset) = bval;
> + scsi_kunmap_atomic_sg(ptr);
> + }
> + }
> + bytes_sent += fifo_cnt;
> + ent->flags &= ~ESP_CMD_FLAG_RESIDUAL;
> + }
> if (!(ent->flags & ESP_CMD_FLAG_WRITE))
> bytes_sent -= fifo_cnt;
>
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 27dcaf8..5fa456c 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -269,6 +269,7 @@ struct esp_cmd_entry {
> #define ESP_CMD_FLAG_WRITE 0x01 /* DMA is a write */
> #define ESP_CMD_FLAG_ABORT 0x02 /* being aborted */
> #define ESP_CMD_FLAG_AUTOSENSE 0x04 /* Doing automatic REQUEST_SENSE */
> +#define ESP_CMD_FLAG_RESIDUAL0x08 /* AM53c974 BLAST residual */
>
> u8 tag[2];
> u8 orig_tag[2];
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] esp_scsi: use FIFO for command submission
->send_dma_cmd(esp, esp->command_block_dma,
> -esp->cmd_bytes_left, 16, 0,
> -ESP_CMD_DMA | ESP_CMD_TI);
> + esp_send_dma_cmd(esp, esp->cmd_bytes_left, 16, ESP_CMD_TI);
> esp_event(esp, ESP_EVENT_CMD_DONE);
> esp->flags |= ESP_FLAG_QUICKIRQ_CHECK;
> break;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 975d293..27dcaf8 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -478,6 +478,7 @@ struct esp {
> #define ESP_FLAG_WIDE_CAPABLE0x0008
> #define ESP_FLAG_QUICKIRQ_CHECK 0x0010
> #define ESP_FLAG_DISABLE_SYNC0x0020
> +#define ESP_FLAG_USE_FIFO0x0040
>
> u8 select_state;
> #define ESP_SELECT_NONE 0x00 /* Not selecting */
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] esp: enable CONFIG2_FENAB for am53c974
On 21/11/2014 11:22, Hannes Reinecke wrote:
> On 11/21/2014 11:08 AM, Paolo Bonzini wrote:
>>
>>
>> On 21/11/2014 10:27, Hannes Reinecke wrote:
>>> CONFIG2_FENAB ('feature enable') changed definition between chip
>>> revisions, from 'Latch SCSI Phase' to 'Latch SCSI Phase, display
>>> chip ID upon reset, and enable 24 bit addresses'.
>>> So only enable it for am53c974 where we know what it's doing.
>>>
>>> Signed-off-by: Hannes Reinecke
>>> ---
>>> drivers/scsi/am53c974.c | 30 ++
>>> drivers/scsi/esp_scsi.c | 4
>>> 2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
>>> index 0452ed1..722e781 100644
>>> --- a/drivers/scsi/am53c974.c
>>> +++ b/drivers/scsi/am53c974.c
>>> @@ -252,6 +252,8 @@ static void pci_esp_send_dma_cmd(struct esp *esp, u32
>>> addr, u32 esp_count,
>>>
>>> pci_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>>> pci_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
>>> + if (esp->config2 & ESP_CONFIG2_FENAB)
>>> + pci_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>>
>> Why do this conditionally? We know that FENAB is true here, don't we?
>>
>> (Maybe I'm missing something obvious though).
>>
> Not really. Point is that 'FENAB' does actually three things:
> - Enable TCHI for 24-bit DMA transfer lengths
> - Provide Chip ID in TCHI after reset
> - Latch SCSI phase after completion in SCSI STATUS
> So we _might_ run into timing issues due to the last point, so I've
> made it conditional in case we'd have to disable it.
Maybe make it a module parameter maybe?
Something like this lets you set esp->config2 from the driver. Look at
it with -b to avoid insanity, it changes half a dozen line modulo the
reindendation.
-- 8<
From: Paolo Bonzini
Subject: [PATCH] esp_scsi: let DMA driver provide a config2 value
On PCscsi, the FENAB configuration also enables 24-bit DMA transfer lengths
(and provides the chip id in TCHI after reset). We want this to be available
as a module parameter.
Check if the caller of scsi_esp_register provided a value for esp->config2.
If this is the case, assume this is not an ESP100, skip the detection
phase and leave esp->config2 untouched. It will be used in esp_reset_esp.
Signed-off-by: Paolo Bonzini
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 55548dc5cec3..123edcf439c0 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2149,47 +2149,50 @@ static void esp_get_revision(struct esp *esp)
u8 val;
esp->config1 = (ESP_CONFIG1_PENABLE | (esp->scsi_id & 7));
- esp->config2 = (ESP_CONFIG2_SCSI2ENAB | ESP_CONFIG2_REGPARITY);
+ if (esp->config2 == 0) {
+ esp->config2 = (ESP_CONFIG2_SCSI2ENAB | ESP_CONFIG2_REGPARITY);
+ esp_write8(esp->config2, ESP_CFG2);
+
+ val = esp_read8(ESP_CFG2);
+ val &= ~ESP_CONFIG2_MAGIC;
+
+ esp->config2 = 0;
+ if (val != (ESP_CONFIG2_SCSI2ENAB | ESP_CONFIG2_REGPARITY)) {
+ /* If what we write to cfg2 does not come back, cfg2 is
not
+* implemented, therefore this must be a plain esp100.
+*/
+ esp->rev = ESP100;
+ return;
+ }
+ }
+
+ esp_set_all_config3(esp, 5);
+ esp->prev_cfg3 = 5;
esp_write8(esp->config2, ESP_CFG2);
+ esp_write8(0, ESP_CFG3);
+ esp_write8(esp->prev_cfg3, ESP_CFG3);
- val = esp_read8(ESP_CFG2);
- val &= ~ESP_CONFIG2_MAGIC;
- if (val != (ESP_CONFIG2_SCSI2ENAB | ESP_CONFIG2_REGPARITY)) {
- /* If what we write to cfg2 does not come back, cfg2 is not
-* implemented, therefore this must be a plain esp100.
+ val = esp_read8(ESP_CFG3);
+ if (val != 5) {
+ /* The cfg2 register is implemented, however
+* cfg3 is not, must be esp100a.
*/
- esp->rev = ESP100;
+ esp->rev = ESP100A;
} else {
- esp->config2 = 0;
- esp_set_all_config3(esp, 5);
- esp->prev_cfg3 = 5;
- esp_write8(esp->config2, ESP_CFG2);
- esp_write8(0, ESP_CFG3);
+ esp_set_all_config3(esp, 0);
+ esp->prev_cfg3 = 0;
esp_write8(esp->prev_cfg3, ESP_CFG3);
- val = esp_read8(
Re: [PATCH 07/10] esp: Use FIFO for command submission
On 21/11/2014 11:38, Hannes Reinecke wrote:
>>> esp->msg_out_len = 0;
>>>
>>> *p++ = IDENTIFY(0, lun);
>>> @@ -648,12 +651,21 @@ static void esp_autosense(struct esp *esp, struct
>>> esp_cmd_entry *ent)
>>> esp_write_tgt_sync(esp, tgt);
>>> esp_write_tgt_config3(esp, tgt);
>>>
>>> - val = (p - esp->command_block);
>>> + if (esp->flags & ESP_FLAG_USE_FIFO)
>>> + val = (p - esp->fifo);
>>> + else
>>> + val = (p - esp->command_block);
>>>
>>> if (esp->rev == FASHME)
>>> scsi_esp_cmd(esp, ESP_CMD_FLUSH);
>>
>> For consistency with what you do elsewhere, please move this in the "else".
>>
> No. The 'USE_FIFO' mechanism is a general feature which _should_
> work on all chips. The above 'if' condition is a chipset-specific
> feature which should be treated separately.
Yup, but USE_FIFO always sends a flush anyway. Sending two is useless.
>>> - esp->ops->send_dma_cmd(esp, esp->command_block_dma,
>>> - val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
>>> + if (esp->flags & ESP_FLAG_USE_FIFO) {
>>> + scsi_esp_cmd(esp, ESP_CMD_FLUSH);
>>> + for (i = 0; i < val; i++)
>>> + esp_write8(esp->fifo[i], ESP_FDATA);
>>> + scsi_esp_cmd(esp, ESP_CMD_SELA);
>>> + } else
>>> + esp->ops->send_dma_cmd(esp, esp->command_block_dma,
>>> + val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
>>> }
>>
>> Hmm, what about a wrapper
>>
>> __send_cmd(esp, data, esp_count, dma_count, cmd, force_flush,
>>force_fifo)
>> {
>> use_fifo =
>> force_fifo || (esp->flags & ESP_FLAG_USE_FIFO) ||
>> esp_count == 1;
>> if (force_flush || use_fifo || esp->rev == FASHME)
>> scsi_esp_cmd(esp, ESP_CMD_FLUSH);
>> if (use_fifo) {
>> for (i = 0; i < val; i++)
>> esp_write8(esp->fifo[i], ESP_FDATA);
>> scsi_esp_cmd(esp, cmd);
>> } else {
>> if (data != esp->command_block)
>> memcpy(esp->command_block, data, length)
>> esp->ops->send_dma_cmd(esp,
>>esp->command_block_dma,
>>esp_count, dma_count, 0,
>>cmd | ESP_CMD_DMA);
>> }
>> }
>>
>> send_cmd(esp, data, esp_count, dma_count, cmd)
>> {
>> __send_cmd(esp, data, esp_count, dma_count, cmd, false, false);
>> }
>>
>> This would be:
>>
>> send_cmd(esp, esp->command_block, val, 16, ESP_CMD_SELA);
>>
> Good point.
(Note this was also why I was suggesting just using esp->command_block
unconditionally).
Paolo
> Will be updating the patch.
>
> Cheers,
>
> Hannes
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/10] scsi: add 'am53c974' driver
Oops, hit send too early. A small nit:
On 21/11/2014 10:27, Hannes Reinecke wrote:
> +static void pci_esp_dma_drain(struct esp *esp)
> +{
> + u8 resid;
> + int lim = 1000;
> +
> +
> + if ((esp->sreg & ESP_STAT_PMASK) == ESP_DOP ||
> + (esp->sreg & ESP_STAT_PMASK) == ESP_DIP)
> + /* Data-In or Data-Out, nothing to be done */
> + return;
> +
> + while (--lim > 0) {
> + resid = pci_esp_read8(esp, ESP_FFLAGS) & ESP_FF_FBYTES;
> + if (resid <= 1)
> + break;
cpu_relax()?
> + }
> + if (resid > 1) {
> + /* FIFO not cleared */
> + shost_printk(KERN_INFO, esp->host,
> + "FIFO not cleared, %d bytes left\n",
> + resid);
> + }
> +
> + /*
> + * When there is a residual BCMPLT will never be set
> + * (obviously). But we still have to issue the BLAST
> + * command, otherwise the data will not being transferred.
> + * But we'll never know when the BLAST operation is
> + * finished. So check for some time and give up eventually.
> + */
> + lim = 1000;
> + pci_esp_write8(esp, ESP_DMA_CMD_DIR | ESP_DMA_CMD_BLAST, ESP_DMA_CMD);
> + while (pci_esp_read8(esp, ESP_DMA_STATUS) & ESP_DMA_STAT_BCMPLT) {
> + if (--lim == 0)
> + break;
cpu_relax()?
Otherwise looks sane.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/10] scsi: add 'am53c974' driver
On 21/11/2014 10:27, Hannes Reinecke wrote:
> This patch adds a new implementation for the Tekram DC-390T /
> AMD AM53c974 SCSI controller, based on the generic
> esp_scsi infrastructure.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/Kconfig| 18 ++
> drivers/scsi/Makefile | 1 +
> drivers/scsi/am53c974.c | 523
>
> 3 files changed, 542 insertions(+)
> create mode 100644 drivers/scsi/am53c974.c
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 21bc674..497e7d5 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1357,6 +1357,24 @@ config SCSI_DC390T
> To compile this driver as a module, choose M here: the
> module will be called tmscsim.
>
> +config SCSI_AM53C974
> + tristate "Tekram DC390(T) and Am53/79C974 SCSI support (new driver)"
> + depends on PCI && SCSI
Perhaps make it a choice with SCSI_DC390, since they match on the same
PCI vendor/device IDs?
Paolo
> + select SCSI_SPI_ATTRS
> + ---help---
> + This driver supports PCI SCSI host adapters based on the Am53C974A
> + chip, e.g. Tekram DC390(T), DawiControl 2974 and some onboard
> + PCscsi/PCnet (Am53/79C974) solutions.
> + This is a new implementation base on the generic esp_scsi driver.
> +
> + Documentation can be found in .
> +
> + Note that this driver does NOT support Tekram DC390W/U/F, which are
> + based on NCR/Symbios chips. Use "NCR53C8XX SCSI support" for those.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called am53c974.
> +
> config SCSI_T128
> tristate "Trantor T128/T128F/T228 SCSI support"
> depends on ISA && SCSI
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 76f8932..7f974fc 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_SCSI_7000FASST) += wd7000.o
> obj-$(CONFIG_SCSI_EATA) += eata.o
> obj-$(CONFIG_SCSI_DC395x)+= dc395x.o
> obj-$(CONFIG_SCSI_DC390T)+= tmscsim.o
> +obj-$(CONFIG_SCSI_AM53C974) += esp_scsi.o am53c974.o
> obj-$(CONFIG_MEGARAID_LEGACY)+= megaraid.o
> obj-$(CONFIG_MEGARAID_NEWGEN)+= megaraid/
> obj-$(CONFIG_MEGARAID_SAS) += megaraid/
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> new file mode 100644
> index 000..b9af8b0
> --- /dev/null
> +++ b/drivers/scsi/am53c974.c
> @@ -0,0 +1,523 @@
> +/*
> + * AMD am53c974 driver.
> + * Copyright (c) 2014 Hannes Reinecke, SUSE Linux GmbH
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include "esp_scsi.h"
> +
> +#define DRV_MODULE_NAME "am53c974"
> +#define DRV_MODULE_VERSION "1.00"
> +
> +// #define ESP_DMA_DEBUG
> +
> +#define ESP_DMA_CMD 0x10
> +#define ESP_DMA_STC 0x11
> +#define ESP_DMA_SPA 0x12
> +#define ESP_DMA_WBC 0x13
> +#define ESP_DMA_WAC 0x14
> +#define ESP_DMA_STATUS 0x15
> +#define ESP_DMA_SMDLA 0x16
> +#define ESP_DMA_WMAC 0x17
> +
> +#define ESP_DMA_CMD_IDLE 0x00
> +#define ESP_DMA_CMD_BLAST 0x01
> +#define ESP_DMA_CMD_ABORT 0x02
> +#define ESP_DMA_CMD_START 0x03
> +#define ESP_DMA_CMD_MASK 0x03
> +#define ESP_DMA_CMD_DIAG 0x04
> +#define ESP_DMA_CMD_MDL 0x10
> +#define ESP_DMA_CMD_INTE_P 0x20
> +#define ESP_DMA_CMD_INTE_D 0x40
> +#define ESP_DMA_CMD_DIR 0x80
> +
> +#define ESP_DMA_STAT_PWDN 0x01
> +#define ESP_DMA_STAT_ERROR 0x02
> +#define ESP_DMA_STAT_ABORT 0x04
> +#define ESP_DMA_STAT_DONE 0x08
> +#define ESP_DMA_STAT_SCSIINT 0x10
> +#define ESP_DMA_STAT_BCMPLT 0x20
> +
> +/* EEPROM is accessed with 16-bit values */
> +#define DC390_EEPROM_READ 0x80
> +#define DC390_EEPROM_LEN 0x40
> +
> +/*
> + * DC390 EEPROM
> + *
> + * 8 * 4 bytes of per-device options
> + * followed by HBA specific options
> + */
> +
> +/* Per-device options */
> +#define DC390_EE_MODE1 0x00
> +#define DC390_EE_SPEED 0x01
> +
> +/* HBA-specific options */
> +#define DC390_EE_ADAPT_SCSI_ID 0x40
> +#define DC390_EE_MODE2 0x41
> +#define DC390_EE_DELAY 0x42
> +#define DC390_EE_TAG_CMD_NUM 0x43
> +
> +#define DC390_EE_MODE1_PARITY_CHK 0x01
> +#define DC390_EE_MODE1_SYNC_NEGO0x02
> +#define DC390_EE_MODE1_EN_DISC 0x04
> +#define DC390_EE_MODE1_SEND_START 0x08
> +#define DC390_EE_MODE1_TCQ 0x10
> +
> +#define DC390_EE_MODE2_MORE_2DRV0x01
> +#define DC390_EE_MODE2_GREATER_1G 0x02
> +#define DC390_EE_MODE2_RST_SCSI_BUS 0x04
> +#define DC390_EE_MODE2_ACTIVE_NEGATION 0x08
> +#define DC390_EE_MODE2_NO_SEEK 0x10
> +#define DC390_EE_MODE2_LUN_CHECK0x20
> +
> +struct pci_esp_priv {
> + struct esp *esp;
> + u8 dma_status;
> +};
> +
> +#define PCI_ESP_GET_PRIV(esp) ((struct pci_esp_priv *) \
> +pci_get_drvdata((struct pci_dev *) \
> +(esp)->dev))
> +
> +static void pci_esp_dma_drain(struct esp *esp);
> +
> +static void pci_esp_
Re: [PATCH 09/10] esp: correctly detect am53c974
On 21/11/2014 10:27, Hannes Reinecke wrote:
> The am53c974 returns the same ID as the FAS236, but implements
> things slightly differently. So detect the am53c974 by checking
> for ESP_CONFIG4 register.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 2 ++
> drivers/scsi/esp_scsi.c | 17 -
> drivers/scsi/esp_scsi.h | 15 +++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index 0b7643e..0452ed1 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -365,6 +365,8 @@ static void dc390_check_eeprom(struct esp *esp)
> }
> esp->scsi_id = EEbuf[DC390_EE_ADAPT_SCSI_ID];
> esp->num_tags = 2 << EEbuf[DC390_EE_TAG_CMD_NUM];
> + if (EEbuf[DC390_EE_MODE2] & DC390_EE_MODE2_ACTIVE_NEGATION)
> + esp->config4 |= ESP_CONFIG4_RADE | ESP_CONFIG4_RAE;
> }
>
> static int pci_esp_probe_one(struct pci_dev *pdev,
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 88272bb..01753f5 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -250,6 +250,19 @@ static void esp_reset_esp(struct esp *esp)
> } else {
> esp->min_period = ((5 * esp->ccycle) / 1000);
> }
> + if (esp->rev == FAS236) {
> + /*
> + * The AM53c974 chip returns the same ID as FAS236;
> + * try to configure glitch eater.
> + */
> + u8 config4 = ESP_CONFIG4_GE1;
> + esp_write8(config4, ESP_CFG4);
> + config4 = esp_read8(ESP_CFG4);
> + if ((config4 & ESP_CONFIG4_GE1) == ESP_CONFIG4_GE1) {
> + esp->rev = PCSCSI;
> + esp_write8(esp->config4, ESP_CFG4);
> + }
> + }
> esp->max_period = (esp->max_period + 3)>>2;
> esp->min_period = (esp->min_period + 3)>>2;
>
> @@ -275,7 +288,8 @@ static void esp_reset_esp(struct esp *esp)
> /* fallthrough... */
>
> case FAS236:
> - /* Fast 236 or HME */
> + case PCSCSI:
> + /* Fast 236, AM53c974 or HME */
> esp_write8(esp->config2, ESP_CFG2);
> if (esp->rev == FASHME) {
> u8 cfg3 = esp->target[0].esp_config3;
> @@ -2397,6 +2411,7 @@ static const char *esp_chip_names[] = {
> "FAS100A",
> "FAST",
> "FASHME",
> + "AM53C974",
> };
>
> static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 5fa456c..84dcbe4 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -25,6 +25,7 @@
> #define ESP_CTEST0x0aUL /* wo Chip test register 0x28 */
> #define ESP_CFG2 0x0bUL /* rw Second cfg register 0x2c */
> #define ESP_CFG3 0x0cUL /* rw Third cfg register 0x30 */
> +#define ESP_CFG4 0x0dUL /* rw Fourth cfg register 0x34 */
> #define ESP_TCHI 0x0eUL /* rw High bits transf count 0x38 */
> #define ESP_UID ESP_TCHI/* ro Unique ID code
> 0x38 */
> #define FAS_RLO ESP_TCHI/* rw HME extended counter
> 0x38 */
> @@ -76,6 +77,18 @@
> #define ESP_CONFIG3_IMS 0x80 /* ID msg chk'ng(esp/fas236)
> */
> #define ESP_CONFIG3_OBPUSH0x80 /* Push odd-byte to dma (hme)
> */
>
> +/* ESP config register 4 read-write, found only on am53c974 chips */
> +#define ESP_CONFIG4_RADE 0x04 /* Active negation */
> +#define ESP_CONFIG4_RAE 0x08 /* Active negation on REQ and ACK */
> +#define ESP_CONFIG4_PWD 0x20 /* Reduced power feature */
> +#define ESP_CONFIG4_GE0 0x40 /* Glitch eater bit 0 */
> +#define ESP_CONFIG4_GE1 0x80 /* Glitch eater bit 1 */
> +
> +#define ESP_CONFIG_GE_12NS(0)
> +#define ESP_CONFIG_GE_25NS(ESP_CONFIG_GE1)
> +#define ESP_CONFIG_GE_35NS(ESP_CONFIG_GE0)
> +#define ESP_CONFIG_GE_0NS (ESP_CONFIG_GE0 | ESP_CONFIG_GE1)
> +
> /* ESP command register read-write */
> /* Group 1 commands: These may be sent at any point in time to the ESP
> *chip. None of them can generate interrupts 'cept
> @@ -254,6 +267,7 @@ enum esp_rev {
> FAS100A= 0x04,
> FAST = 0x05,
> FASHME = 0x06,
> + PCSCSI = 0x07, /* AM53c974 */
> };
>
> struct esp_cmd_entry {
> @@ -466,6 +480,7
Re: [PATCH 08/10] am53c974: BLAST residual handling
On 21/11/2014 10:27, Hannes Reinecke wrote:
> The am53c974 has an design issue where a single byte might be
> left in the SCSI FIFO after a DMA transfer.
> As the handling code is currently untested add a WARN_ON()
> statement here.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 6 ++
> drivers/scsi/esp_scsi.c | 29 +
> drivers/scsi/esp_scsi.h | 1 +
> 3 files changed, 36 insertions(+)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index 652762e..0b7643e 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -195,6 +195,12 @@ static void pci_esp_dma_drain(struct esp *esp)
> shost_printk(KERN_INFO, esp->host,
>"DMA blast done (%d tries, %d bytes left)\n", lim, resid);
> #endif
> + /* BLAST residual handling is currently untested */
> + if (WARN_ON(resid == 1)) {
WARN_ON_ONCE?
Paolo
> + struct esp_cmd_entry *ent = esp->active_cmd;
> +
> + ent->flags |= ESP_CMD_FLAG_RESIDUAL;
> + }
> }
>
> static void pci_esp_dma_invalidate(struct esp *esp)
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index ab7d2bc..88272bb 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -1353,6 +1353,35 @@ static int esp_data_bytes_sent(struct esp *esp, struct
> esp_cmd_entry *ent,
> bytes_sent = esp->data_dma_len;
> bytes_sent -= ecount;
>
> + /*
> + * The am53c974 has a DMA 'pecularity'. The doc states:
> + * In some odd byte conditions, one residual byte will
> + * be left in the SCSI FIFO, and the FIFO Flags will
> + * never count to '0 '. When this happens, the residual
> + * byte should be retrieved via PIO following completion
> + * of the BLAST operation.
> + */
> + if (fifo_cnt == 1 && ent->flags & ESP_CMD_FLAG_RESIDUAL) {
> + size_t count = 1;
> + size_t offset = bytes_sent;
> + u8 bval = esp_read8(ESP_FDATA);
> +
> + if (ent->flags & ESP_CMD_FLAG_AUTOSENSE)
> + ent->sense_ptr[bytes_sent] = bval;
> + else {
> + struct esp_cmd_priv *p = ESP_CMD_PRIV(cmd);
> + u8 *ptr;
> +
> + ptr = scsi_kmap_atomic_sg(p->cur_sg, p->u.num_sg,
> + &offset, &count);
> + if (likely(ptr)) {
> + *(ptr + offset) = bval;
> + scsi_kunmap_atomic_sg(ptr);
> + }
> + }
> + bytes_sent += fifo_cnt;
> + ent->flags &= ~ESP_CMD_FLAG_RESIDUAL;
> + }
> if (!(ent->flags & ESP_CMD_FLAG_WRITE))
> bytes_sent -= fifo_cnt;
>
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 27dcaf8..5fa456c 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -269,6 +269,7 @@ struct esp_cmd_entry {
> #define ESP_CMD_FLAG_WRITE 0x01 /* DMA is a write */
> #define ESP_CMD_FLAG_ABORT 0x02 /* being aborted */
> #define ESP_CMD_FLAG_AUTOSENSE 0x04 /* Doing automatic REQUEST_SENSE */
> +#define ESP_CMD_FLAG_RESIDUAL0x08 /* AM53c974 BLAST residual */
>
> u8 tag[2];
> u8 orig_tag[2];
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] esp: enable CONFIG2_FENAB for am53c974
On 21/11/2014 10:27, Hannes Reinecke wrote:
> CONFIG2_FENAB ('feature enable') changed definition between chip
> revisions, from 'Latch SCSI Phase' to 'Latch SCSI Phase, display
> chip ID upon reset, and enable 24 bit addresses'.
> So only enable it for am53c974 where we know what it's doing.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 30 ++
> drivers/scsi/esp_scsi.c | 4
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index 0452ed1..722e781 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -252,6 +252,8 @@ static void pci_esp_send_dma_cmd(struct esp *esp, u32
> addr, u32 esp_count,
>
> pci_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> pci_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> + if (esp->config2 & ESP_CONFIG2_FENAB)
> + pci_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
Why do this conditionally? We know that FENAB is true here, don't we?
(Maybe I'm missing something obvious though).
Paolo
>
> pci_esp_write32(esp, esp_count, ESP_DMA_STC);
> pci_esp_write32(esp, addr, ESP_DMA_SPA);
> @@ -265,6 +267,33 @@ static void pci_esp_send_dma_cmd(struct esp *esp, u32
> addr, u32 esp_count,
> pci_esp_write8(esp, ESP_DMA_CMD_START | val, ESP_DMA_CMD);
> }
>
> +static u32 pci_esp_dma_length_limit(struct esp *esp, u32 dma_addr, u32
> dma_len)
> +{
> + int dma_limit = 16;
> + u32 base, end;
> +
> + /*
> + * If CONFIG2_FENAB is set we can
> + * handle up to 24 bit addresses
> + */
> + if (esp->config2 & ESP_CONFIG2_FENAB)
> + dma_limit = 24;
> +
> + if (dma_len > (1U << dma_limit))
> + dma_len = (1U << dma_limit);
> +
> + /*
> + * Prevent crossing a 24-bit address boundary.
> + */
> + base = dma_addr & ((1U << 24) - 1U);
> + end = base + dma_len;
> + if (end > (1U << 24))
> + end = (1U <<24);
> + dma_len = end - base;
> +
> + return dma_len;
> +}
> +
> static const struct esp_driver_ops pci_esp_ops = {
> .esp_write8 = pci_esp_write8,
> .esp_read8 = pci_esp_read8,
> @@ -278,6 +307,7 @@ static const struct esp_driver_ops pci_esp_ops = {
> .dma_invalidate = pci_esp_dma_invalidate,
> .send_dma_cmd = pci_esp_send_dma_cmd,
> .dma_error = pci_esp_dma_error,
> + .dma_length_limit = pci_esp_dma_length_limit,
> };
>
> /*
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 01753f5..d0b7b32 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -289,6 +289,8 @@ static void esp_reset_esp(struct esp *esp)
>
> case FAS236:
> case PCSCSI:
> + if (esp->rev == PCSCSI)
> + esp->config2 |= ESP_CONFIG2_FENAB;
> /* Fast 236, AM53c974 or HME */
> esp_write8(esp->config2, ESP_CFG2);
> if (esp->rev == FASHME) {
> @@ -1362,6 +1364,8 @@ static int esp_data_bytes_sent(struct esp *esp, struct
> esp_cmd_entry *ent,
> (((unsigned int)esp_read8(ESP_TCMED)) << 8));
> if (esp->rev == FASHME)
> ecount |= ((unsigned int)esp_read8(FAS_RLO)) << 16;
> + if (esp->rev == PCSCSI && (esp->config2 & ESP_CONFIG2_FENAB))
> + ecount |= ((unsigned int)esp_read8(ESP_TCHI)) << 16;
> }
>
> bytes_sent = esp->data_dma_len;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/10] esp: Use FIFO for command submission
On 21/11/2014 10:27, Hannes Reinecke wrote:
> The am53c974 has a design flaw causing it to throw an
> DMA interrupt whenever a DMA transmission completed,
> even though DMA interrupt reporting is disabled.
> This confuses the esp routines as it would register
> a DMA interrupt whenever a cdb has been transmitted
> to the drive.
> So implement an option to use the FIFO for command
> submission and enable it for am53c974.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/am53c974.c | 1 +
> drivers/scsi/esp_scsi.c | 83
> ++---
> drivers/scsi/esp_scsi.h | 1 +
> 3 files changed, 66 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
> index b9af8b0..652762e 100644
> --- a/drivers/scsi/am53c974.c
> +++ b/drivers/scsi/am53c974.c
> @@ -401,6 +401,7 @@ static int pci_esp_probe_one(struct pci_dev *pdev,
> esp->host = shost;
> esp->dev = pdev;
> esp->ops = &pci_esp_ops;
> + esp->flags |= ESP_FLAG_USE_FIFO;
Why not invert this patch and the previous one, and add this line
directly when the am53c974 driver is born?
> pep->esp = esp;
>
> if (pci_request_regions(pdev, DRV_MODULE_NAME)) {
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 92ab921..ab7d2bc 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -605,7 +605,7 @@ static void esp_autosense(struct esp *esp, struct
> esp_cmd_entry *ent)
> {
> struct scsi_cmnd *cmd = ent->cmd;
> struct scsi_device *dev = cmd->device;
> - int tgt, lun;
> + int tgt, lun, i;
> u8 *p, val;
>
> tgt = dev->id;
> @@ -626,7 +626,10 @@ static void esp_autosense(struct esp *esp, struct
> esp_cmd_entry *ent)
>
> esp->active_cmd = ent;
>
> - p = esp->command_block;
> + if (esp->flags & ESP_FLAG_USE_FIFO)
> + p = esp->fifo;
> + else
> + p = esp->command_block;
Any reason not to use esp->command_block unconditionally?
> esp->msg_out_len = 0;
>
> *p++ = IDENTIFY(0, lun);
> @@ -648,12 +651,21 @@ static void esp_autosense(struct esp *esp, struct
> esp_cmd_entry *ent)
> esp_write_tgt_sync(esp, tgt);
> esp_write_tgt_config3(esp, tgt);
>
> - val = (p - esp->command_block);
> + if (esp->flags & ESP_FLAG_USE_FIFO)
> + val = (p - esp->fifo);
> + else
> + val = (p - esp->command_block);
>
> if (esp->rev == FASHME)
> scsi_esp_cmd(esp, ESP_CMD_FLUSH);
For consistency with what you do elsewhere, please move this in the "else".
> - esp->ops->send_dma_cmd(esp, esp->command_block_dma,
> -val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
> + if (esp->flags & ESP_FLAG_USE_FIFO) {
> + scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> + for (i = 0; i < val; i++)
> + esp_write8(esp->fifo[i], ESP_FDATA);
> + scsi_esp_cmd(esp, ESP_CMD_SELA);
> + } else
> + esp->ops->send_dma_cmd(esp, esp->command_block_dma,
> +val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
> }
Hmm, what about a wrapper
__send_cmd(esp, data, esp_count, dma_count, cmd, force_flush,
force_fifo)
{
use_fifo =
force_fifo || (esp->flags & ESP_FLAG_USE_FIFO) ||
esp_count == 1;
if (force_flush || use_fifo || esp->rev == FASHME)
scsi_esp_cmd(esp, ESP_CMD_FLUSH);
if (use_fifo) {
for (i = 0; i < val; i++)
esp_write8(esp->fifo[i], ESP_FDATA);
scsi_esp_cmd(esp, cmd);
} else {
if (data != esp->command_block)
memcpy(esp->command_block, data, length)
esp->ops->send_dma_cmd(esp,
esp->command_block_dma,
esp_count, dma_count, 0,
cmd | ESP_CMD_DMA);
}
}
send_cmd(esp, data, esp_count, dma_count, cmd)
{
__send_cmd(esp, data, esp_count, dma_count, cmd, false, false);
}
This would be:
send_cmd(esp, esp->command_block, val, 16, ESP_CMD_SELA);
> static struct esp_cmd_entry *find_and_prep_issuable_command(struct esp *esp)
> @@ -727,7 +739,10 @@ static void esp_maybe_execute_command(struct esp *esp)
>
> esp_check_command_len(esp, cmd);
>
> - p = esp->command_block;
> + if (esp->flags & ESP_FLAG_USE_FIFO)
> + p = esp->fifo;
Any reason not to use esp->command_block unconditionally?
> + else
> + p = esp->command_block;
>
> esp->msg_out_len = 0;
> if (tp->flags & ESP_TGT_CHECK_NEGO) {
> @@ -789,13 +804,15 @@ build_identify:
> }
>
> if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
> - start_cmd = ESP_CMD_DMA | ESP_CMD_SELA;
> + start_cmd = ESP_CMD_SELA;
> if (ent->tag[0]) {
>
Re: [PATCH 05/10] esp_scsi: read status registers
On 21/11/2014 10:27, Hannes Reinecke wrote:
> A read to ESP_INTRPT will clear ESP_STATUS and ESP_SSTEP. So read
> all status registers in one go to avoid losing information.
(ESP_STAT_TCNT is actually kept in the status register, it is cleared by
writing TCLO/MID/HI).
Reviewed-by: Paolo Bonzini
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/esp_scsi.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index fe3278e..92ab921 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -982,7 +982,6 @@ static int esp_check_spur_intr(struct esp *esp)
>
> default:
> if (!(esp->sreg & ESP_STAT_INTR)) {
> - esp->ireg = esp_read8(ESP_INTRPT);
> if (esp->ireg & ESP_INTR_SR)
> return 1;
>
> @@ -2056,7 +2055,12 @@ static void __esp_interrupt(struct esp *esp)
> int finish_reset, intr_done;
> u8 phase;
>
> + /*
> + * Once INTRPT is read STATUS and SSTEP are cleared.
> + */
> esp->sreg = esp_read8(ESP_STATUS);
> + esp->seqreg = esp_read8(ESP_SSTEP);
> + esp->ireg = esp_read8(ESP_INTRPT);
>
> if (esp->flags & ESP_FLAG_RESETTING) {
> finish_reset = 1;
> @@ -2069,8 +2073,6 @@ static void __esp_interrupt(struct esp *esp)
> return;
> }
>
> - esp->ireg = esp_read8(ESP_INTRPT);
> -
> if (esp->ireg & ESP_INTR_SR)
> finish_reset = 1;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/10] esp_scsi: convert to dev_printk
;ESP: Disconnecting tgt[%d] "
> -"tag[%x:%x]\n",
> + esp_log_disconnect("Disconnecting tgt[%d] tag[%x:%x]\n",
> cmd->device->id,
> ent->tag[0], ent->tag[1]);
>
> esp->active_cmd = NULL;
> esp_maybe_execute_command(esp);
> } else {
> - printk("ESP: Unexpected message %x in freebus\n",
> -ent->message);
> + shost_printk(KERN_INFO, esp->host,
> + "Unexpected message %x in freebus\n",
> + ent->message);
> esp_schedule_reset(esp);
> return 0;
> }
> @@ -1917,7 +1913,7 @@ again:
> val = esp_read8(ESP_FDATA);
> esp->msg_in[esp->msg_in_len++] = val;
>
> - esp_log_msgin("ESP: Got msgin byte %x\n", val);
> + esp_log_msgin("Got msgin byte %x\n", val);
>
> if (!esp_msgin_process(esp))
> esp->msg_in_len = 0;
> @@ -1930,7 +1926,8 @@ again:
> if (esp->event != ESP_EVENT_FREE_BUS)
> esp_event(esp, ESP_EVENT_CHECK_PHASE);
> } else {
> - printk("ESP: MSGIN neither BSERV not FDON, resetting");
> + shost_printk(KERN_INFO, esp->host,
> + "MSGIN neither BSERV not FDON, resetting");
> esp_schedule_reset(esp);
> return 0;
> }
> @@ -1961,8 +1958,8 @@ again:
> break;
>
> default:
> - printk("ESP: Unexpected event %x, resetting\n",
> -esp->event);
> + shost_printk(KERN_INFO, esp->host,
> + "Unexpected event %x, resetting\n", esp->event);
> esp_schedule_reset(esp);
> return 0;
> break;
> @@ -2085,14 +2082,15 @@ static void __esp_interrupt(struct esp *esp)
> }
> }
>
> - esp_log_intr("ESP: intr sreg[%02x] seqreg[%02x] "
> + esp_log_intr("intr sreg[%02x] seqreg[%02x] "
>"sreg2[%02x] ireg[%02x]\n",
>esp->sreg, esp->seqreg, esp->sreg2, esp->ireg);
>
> intr_done = 0;
>
> if (esp->ireg & (ESP_INTR_S | ESP_INTR_SATN | ESP_INTR_IC)) {
> - printk("ESP: unexpected IREG %02x\n", esp->ireg);
> + shost_printk(KERN_INFO, esp->host,
> + "unexpected IREG %02x\n", esp->ireg);
> if (esp->ireg & ESP_INTR_IC)
> esp_dump_cmd_log(esp);
>
> @@ -2332,12 +2330,13 @@ int scsi_esp_register(struct esp *esp, struct device
> *dev)
>
> esp_bootup_reset(esp);
>
> - printk(KERN_INFO PFX "esp%u, regs[%1p:%1p] irq[%u]\n",
> -esp->host->unique_id, esp->regs, esp->dma_regs,
> -esp->host->irq);
> - printk(KERN_INFO PFX "esp%u is a %s, %u MHz (ccf=%u), SCSI ID %u\n",
> -esp->host->unique_id, esp_chip_names[esp->rev],
> -esp->cfreq / 100, esp->cfact, esp->scsi_id);
> + dev_printk(KERN_INFO, dev, "esp%u: regs[%1p:%1p] irq[%u]\n",
> +esp->host->unique_id, esp->regs, esp->dma_regs,
> +esp->host->irq);
> + dev_printk(KERN_INFO, dev,
> +"esp%u: is a %s, %u MHz (ccf=%u), SCSI ID %u\n",
> +esp->host->unique_id, esp_chip_names[esp->rev],
> +esp->cfreq / 100, esp->cfact, esp->scsi_id);
>
> /* Let the SCSI bus reset settle. */
> ssleep(esp_bus_reset_settle);
> @@ -2435,19 +2434,20 @@ static int esp_eh_abort_handler(struct scsi_cmnd *cmd)
>* XXX much for the final driver.
>*/
> spin_lock_irqsave(esp->host->host_lock, flags);
> - printk(KERN_ERR PFX "esp%d: Aborting command [%p:%02x]\n",
> -esp->host->unique_id, cmd, cmd->cmnd[0]);
> + shost_printk(KERN_ERR, esp->host, "Aborting command [%p:%02x]\n",
> + cmd, cmd->cmnd[0]);
> ent = esp->active_cmd;
> if (ent)
> - printk(KERN_ERR PFX "esp%d: Current command [%p:%02x]\n",
> -esp->host->unique_id, ent->cmd, ent->cmd->cmnd[0]);
> + shost_printk(KERN_ERR, esp->host,
> + "Current command [%p:%02x]\n",
> + ent->cmd, ent->cmd->cmnd[0]);
> list_for_each_entry(ent, &esp->queued_cmds, list) {
> - printk(KERN_ERR PFX "esp%d: Queued command [%p:%02x]\n",
> -esp->host->unique_id, ent->cmd, ent->cmd->cmnd[0]);
> + shost_printk(KERN_ERR, esp->host, "Queued command [%p:%02x]\n",
> + ent->cmd, ent->cmd->cmnd[0]);
> }
> list_for_each_entry(ent, &esp->active_cmds, list) {
> - printk(KERN_ERR PFX "esp%d: Active command [%p:%02x]\n",
> -esp->host->unique_id, ent->cmd, ent->cmd->cmnd[0]);
> + shost_printk(KERN_ERR, esp->host, " Active command [%p:%02x]\n",
> + ent->cmd, ent->cmd->cmnd[0]);
> }
> esp_dump_cmd_log(esp);
> spin_unlock_irqrestore(esp->host->host_lock, flags);
>
Looks good.
Reviewed-by: Paolo Bonzini
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] esp_scsi: debug event and command
On 21/11/2014 10:27, Hannes Reinecke wrote:
> Add new debug definitions for event and command logging.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/esp_scsi.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 492c51b..fe3278e 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -49,6 +49,8 @@ static u32 esp_debug;
> #define ESP_DEBUG_DATADONE 0x0100
> #define ESP_DEBUG_RECONNECT 0x0200
> #define ESP_DEBUG_AUTOSENSE 0x0400
> +#define ESP_DEBUG_EVENT 0x0800
> +#define ESP_DEBUG_COMMAND0x1000
>
> #define esp_log_intr(f, a...) \
> do { if (esp_debug & ESP_DEBUG_INTR) \
> @@ -100,6 +102,16 @@ do { if (esp_debug & ESP_DEBUG_AUTOSENSE) \
> shost_printk(KERN_DEBUG, esp->host, f, ## a); \
> } while (0)
>
> +#define esp_log_event(f, a...) \
> +do { if (esp_debug & ESP_DEBUG_EVENT) \
> + shost_printk(KERN_DEBUG, esp->host, f, ## a); \
> +} while (0)
> +
> +#define esp_log_command(f, a...) \
> +do { if (esp_debug & ESP_DEBUG_COMMAND)\
> + shost_printk(KERN_DEBUG, esp->host, f, ## a); \
> +} while (0)
> +
> #define esp_read8(REG) esp->ops->esp_read8(esp, REG)
> #define esp_write8(VAL,REG) esp->ops->esp_write8(esp, VAL, REG)
>
> @@ -126,6 +138,7 @@ void scsi_esp_cmd(struct esp *esp, u8 val)
>
> esp->esp_event_cur = (idx + 1) & (ESP_EVENT_LOG_SZ - 1);
>
> + esp_log_command("cmd[%02x]\n", val);
> esp_write8(val, ESP_CMD);
> }
> EXPORT_SYMBOL(scsi_esp_cmd);
> @@ -1638,6 +1651,8 @@ static int esp_process_event(struct esp *esp)
>
> again:
> write = 0;
> + esp_log_event("process event %d phase %x\n",
> + esp->event, esp->sreg & ESP_STAT_PMASK);
> switch (esp->event) {
> case ESP_EVENT_CHECK_PHASE:
> switch (esp->sreg & ESP_STAT_PMASK) {
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] esp_scsi: make number of tags configurable
On 21/11/2014 10:27, Hannes Reinecke wrote:
> Add a field 'num_tags' to the esp structure to allow drivers
> to overwrite the number of avialable tags if required.
> Default is ESP_DEFAULT_TAGS.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/esp_scsi.c | 10 --
> drivers/scsi/esp_scsi.h | 3 +--
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 38c23e0..99cd42f 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2317,6 +2317,8 @@ int scsi_esp_register(struct esp *esp, struct device
> *dev)
> static int instance;
> int err;
>
> + if (!esp->num_tags)
> + esp->num_tags = ESP_DEFAULT_TAGS;
> esp->host->transportt = esp_transport_template;
> esp->host->max_lun = ESP_MAX_LUN;
> esp->host->cmd_per_lun = 2;
> @@ -2403,12 +2405,8 @@ static int esp_slave_configure(struct scsi_device *dev)
> struct esp *esp = shost_priv(dev->host);
> struct esp_target_data *tp = &esp->target[dev->id];
>
> - if (dev->tagged_supported) {
> - /* XXX make this configurable somehow XXX */
> - int goal_tags = min(ESP_DEFAULT_TAGS, ESP_MAX_TAG);
min(16, 256) = 16
Might add a WARN_ON(esp->num_tags > ESP_MAX_TAG) after you assign
esp->num_tags.
> - scsi_adjust_queue_depth(dev, goal_tags);
> - }
> + if (dev->tagged_supported)
> + scsi_adjust_queue_depth(dev, esp->num_tags);
>
> tp->flags |= ESP_TGT_DISCONNECT;
>
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index b5862e4..975d293 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -283,7 +283,6 @@ struct esp_cmd_entry {
> struct completion *eh_done;
> };
>
> -/* XXX make this configurable somehow XXX */
> #define ESP_DEFAULT_TAGS 16
>
> #define ESP_MAX_TARGET 16
> @@ -445,7 +444,7 @@ struct esp {
> u8 prev_soff;
> u8 prev_stp;
> u8 prev_cfg3;
> - u8 __pad;
> + u8 num_tags;
>
> struct list_headesp_cmd_pool;
>
>
Reviewed-by: Paolo Bonzini
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
