Re: [PATCH] virtio_scsi: remove duplicate check if queue is broken
On Tue, 16 Jan 2024 12:58:36 +0800, Li RongQing wrote: > virtqueue_enable_cb() will call virtqueue_poll() which will check if > queue is broken at beginning, so remove the virtqueue_is_broken() call > > Applied to 6.8/scsi-fixes, thanks! [1/1] virtio_scsi: remove duplicate check if queue is broken https://git.kernel.org/mkp/scsi/c/d6b75ba52189 -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] virtio_scsi: remove duplicate check if queue is broken
> virtqueue_enable_cb() will call virtqueue_poll() which will check if > queue is broken at beginning, so remove the virtqueue_is_broken() call Applied to 6.8/scsi-staging, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] virtio_scsi: remove duplicate check if queue is broken
On Tue, Jan 16, 2024 at 12:58:36PM +0800, Li RongQing wrote: > virtqueue_enable_cb() will call virtqueue_poll() which will check if > queue is broken at beginning, so remove the virtqueue_is_broken() call > > Signed-off-by: Li RongQing Acked-by: Michael S. Tsirkin > --- > 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 9d1bdcd..e15b380 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -182,8 +182,6 @@ static void virtscsi_vq_done(struct virtio_scsi *vscsi, > while ((buf = virtqueue_get_buf(vq, &len)) != NULL) > fn(vscsi, buf); > > - if (unlikely(virtqueue_is_broken(vq))) > - break; > } while (!virtqueue_enable_cb(vq)); > spin_unlock_irqrestore(&virtscsi_vq->vq_lock, flags); > } > -- > 2.9.4
Re: [PATCH] virtio_scsi: remove duplicate check if queue is broken
On Tue, 16 Jan 2024 at 00:07, Li RongQing wrote: > > virtqueue_enable_cb() will call virtqueue_poll() which will check if > queue is broken at beginning, so remove the virtqueue_is_broken() call > > Signed-off-by: Li RongQing > --- > drivers/scsi/virtio_scsi.c | 2 -- > 1 file changed, 2 deletions(-) Reviewed-by: Stefan Hajnoczi > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 9d1bdcd..e15b380 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -182,8 +182,6 @@ static void virtscsi_vq_done(struct virtio_scsi *vscsi, > while ((buf = virtqueue_get_buf(vq, &len)) != NULL) > fn(vscsi, buf); > > - if (unlikely(virtqueue_is_broken(vq))) > - break; > } while (!virtqueue_enable_cb(vq)); > spin_unlock_irqrestore(&virtscsi_vq->vq_lock, flags); > } > -- > 2.9.4 > >
Re: [PATCH linux-firmware] bnx2x: Add FW 7.13.15.0.
On Tue, Oct 22, 2019 at 10:02 AM Sudarsana Reddy Kalluru wrote: > > This patch adds new FW for bnx2x, which addresses the following issues: > - TCP packet with padding can open TPA aggregation in GRO mode. > - Tx Silent Drops could cause HW error when statistics is not enabled for > client. > - Transmission of tunneled packets over tx-only clients (with cos>0 in this > case) followed by load/unload with DCB update (for instance), resulted in a > Tx path halt. > - FORWARD_SETUP ramrod yielded a FW assert (x_eth_fp_hsi_ver_invalid). > > The FW also adds support for direct update of RSS indirection table entry. > > Signed-off-by: Sudarsana Reddy Kalluru > Signed-off-by: Ameen Rahman > --- > WHENCE | 3 +++ > bnx2x/bnx2x-e1-7.13.15.0.fw | Bin 0 -> 170168 bytes > bnx2x/bnx2x-e1h-7.13.15.0.fw | Bin 0 -> 178608 bytes > bnx2x/bnx2x-e2-7.13.15.0.fw | Bin 0 -> 323360 bytes > 4 files changed, 3 insertions(+) > create mode 100644 bnx2x/bnx2x-e1-7.13.15.0.fw > create mode 100644 bnx2x/bnx2x-e1h-7.13.15.0.fw > create mode 100644 bnx2x/bnx2x-e2-7.13.15.0.fw Applied and pushed out. josh
Re: [PATCH V4 1/2] scsi: core: avoid host-wide host_busy counter for scsi_mq
On 09/10/2019 10:32, Ming Lei wrote:
It isn't necessary to check the host depth in scsi_queue_rq() any more
since it has been respected by blk-mq before calling scsi_queue_rq() via
getting driver tag.
Lots of LUNs may attach to same host and per-host IOPS may reach millions,
so we should avoid expensive atomic operations on the host-wide counter in
the IO path.
This patch implements scsi_host_busy() via blk_mq_tagset_busy_iter()
with one scsi command state for reading the count of busy IOs for scsi_mq.
It is observed that IOPS is increased by 15% in IO test on scsi_debug (32
LUNs, 32 submit queues, 1024 can_queue, libaio/dio) in a dual-socket
system.
V2:
- introduce SCMD_STATE_INFLIGHT for getting accurate host busy
via blk_mq_tagset_busy_iter()
- verified that original Jens's report[1] is fixed
- verified that SCSI timeout/abort works fine
[1] https://www.spinics.net/lists/linux-scsi/msg122867.html
[2] V1 & its revert:
d772a65d8a6c Revert "scsi: core: avoid host-wide host_busy counter for scsi_mq"
23aa8e69f2c6 Revert "scsi: core: fix scsi_host_queue_ready"
265d59aacbce scsi: core: fix scsi_host_queue_ready
328728630d9f scsi: core: avoid host-wide host_busy counter for scsi_mq
Cc: Jens Axboe
Cc: Ewan D. Milne
Cc: Omar Sandoval ,
Cc: "Martin K. Petersen" ,
Cc: James Bottomley ,
Cc: Christoph Hellwig ,
Cc: Kashyap Desai
Cc: Hannes Reinecke
Cc: Laurence Oberman
Cc: Bart Van Assche
Signed-off-by: Ming Lei
---
drivers/scsi/hosts.c | 19 ++-
drivers/scsi/scsi.c | 2 +-
drivers/scsi/scsi_lib.c | 35 +--
drivers/scsi/scsi_priv.h | 2 +-
include/scsi/scsi_cmnd.h | 1 +
include/scsi/scsi_host.h | 1 -
6 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 55522b7162d3..1d669e47b692 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
alphabetic ordering could be maintained
#include "scsi_priv.h"
#include "scsi_logging.h"
@@ -554,13 +555,29 @@ struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
}
EXPORT_SYMBOL(scsi_host_get);
+static bool scsi_host_check_in_flight(struct request *rq, void *data,
+ bool reserved)
+{
+ int *count = data;
+ struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
+
+ if (test_bit(SCMD_STATE_INFLIGHT, &cmd->state))
+ (*count)++;
+
+ return true;
+}
+
/**
* scsi_host_busy - Return the host busy counter
is this comment accurate now?
* @shost:Pointer to Scsi_Host to inc.
**/
int scsi_host_busy(struct Scsi_Host *shost)
{
- return atomic_read(&shost->host_busy);
+ int cnt = 0;
+
+ blk_mq_tagset_busy_iter(&shost->tag_set,
+ scsi_host_check_in_flight, &cnt);
When I check blk_mq_tagset_busy_iter(), it does not seem to lock the
tags for all hw queues against preemption for the counting, so how can
we guarantee that this count will be always accurate?
Or it never can be and you just don't care?
+ return cnt;
}
EXPORT_SYMBOL(scsi_host_busy);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1f5b5c8a7f72..ddc4ec6ea2a1 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -186,7 +186,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
struct scsi_driver *drv;
unsigned int good_bytes;
- scsi_device_unbusy(sdev);
+ scsi_device_unbusy(sdev, cmd);
/*
* Clear the flags that say that the device/target/host is no longer
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index dc210b9d4896..b6f66dcb15a5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -189,7 +189,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int
reason, bool unbusy)
* active on the host/device.
*/
if (unbusy)
- scsi_device_unbusy(device);
+ scsi_device_unbusy(device, cmd);
/*
* Requeue this command. It will go before all other commands > @@ -329,12 +329,12 @@ static void scsi_init_cmd_errh(struct scsi_cmnd
*cmd)
The comment for scsi_dec_host_busy() is "Decrement the host_busy counter
and ", so does this need to be fixed up?
I'm guessing that there's lots more places like this...
* host_failed counter or that it notices the shost state change made by
* scsi_eh_scmd_add().
*/
-static void scsi_dec_host_busy(struct Scsi_Host *shost)
+static void scsi_dec_host_busy(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
{
unsigned long flags;
rcu_read_lock();
- atomic_dec(&shost->host_busy);
+ __clear_bit(SCMD_STATE_INFLIGHT, &cmd->state);
if (unlikely(scsi_host_in_recovery(shost))) {
spin_lock_irqsave(shost->host_lock, flags);
if (shost->host_failed || shost->host_eh_s
RE: [RFC PATCH V4 2/2] scsi: core: don't limit per-LUN queue depth for SSD
V4 2/2] scsi: core: don't limit per-LUN queue depth
> for SSD
>
> On Fri, Oct 18, 2019 at 12:00:07AM +0530, Kashyap Desai wrote:
> > > On 10/9/19 2:32 AM, Ming Lei wrote:
> > > > @@ -354,7 +354,8 @@ void scsi_device_unbusy(struct scsi_device
> > > > *sdev,
> > > struct scsi_cmnd *cmd)
> > > > if (starget->can_queue > 0)
> > > > atomic_dec(&starget->target_busy);
> > > >
> > > > - atomic_dec(&sdev->device_busy);
> > > > + if (!blk_queue_nonrot(sdev->request_queue))
> > > > + atomic_dec(&sdev->device_busy);
> > > > }
> > > >
> > >
> > > Hi Ming,
> > >
> > > Does this patch impact the meaning of the queue_depth sysfs
> > > attribute (see also sdev_store_queue_depth()) and also the queue
> > > depth ramp up/down mechanism (see also
> scsi_handle_queue_ramp_up())?
> > > Have you considered to enable/disable busy tracking per LUN
> > > depending on whether or not sdev-
> > > >queue_depth < shost->can_queue?
> > >
> > > The megaraid and mpt3sas drivers read sdev->device_busy directly. Is
> > > the current version of this patch compatible with these drivers?
> >
> > We need to know per scsi device outstanding in mpt3sas and
> > megaraid_sas driver.
>
> Is the READ done in fast path or slow path? If it is on slow path, it
should be
> easy to do via blk_mq_in_flight_rw().
READ is done in fast path.
>
> > Can we get supporting API from block layer (through SML) ? something
> > similar to "atomic_read(&hctx->nr_active)" which can be derived from
> > sdev->request_queue->hctx ?
> > At least for those driver which is nr_hw_queue = 1, it will be useful
> > and we can avoid sdev->device_busy dependency.
>
> If you mean to add new atomic counter, we just move the .device_busy
into
> blk-mq, that can become new bottleneck.
How about below ? We define and use below API instead of
"atomic_read(&scp->device->device_busy) >" and it is giving expected
value. I have not captured performance impact on max IOPs profile.
Inline unsigned long sdev_nr_inflight_request(struct request_queue *q)
{
struct blk_mq_hw_ctx *hctx;
unsigned long nr_requests = 0;
int i;
queue_for_each_hw_ctx(q, hctx, i)
nr_requests += atomic_read(&hctx->nr_active);
return nr_requests;
}
Kashyap
>
>
> thanks,
> Ming
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/22 9:59, zhengbin (A) wrote: > On 2019/10/21 21:06, Hannes Reinecke wrote: >> On 10/21/19 3:49 AM, zhengbin (A) wrote: >>> On 2019/10/18 21:43, Martin K. Petersen wrote: Hannes, > The one thing which I patently don't like is the ambivalence between > DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ > of them is set? IE what would be the correct way of action if > DRIVER_SENSE is not set, but we have a valid sense code? Or the other > way around? I agree, it's a mess. (Sorry, zhengbin, you opened a can of worms. This is some of our oldest and most arcane code in SCSI) > But more important, from a quick glance not all drivers set the > DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is > never evaluated after this patchset. And yet we appear to have several code paths where sense evaluation is contingent on DRIVER_SENSE. So no matter what, behavior might change if we enforce consistent semantics. *sigh* >>> So what should we do to prevent unit-value access of sshdr? >>> >> Where do you see it? >> >From my reading, __scsi_execute() is clearing sshdr by way of >> >> __scsi_execute() >> -> scsi_normalize_sense() >> -> memset(sshdr) > __scsi_execute > > req = blk_get_request(sdev->request_queue, > data_direction == DMA_TO_DEVICE ? > REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); > if (IS_ERR(req)) > return ret; -->just return > rq = scsi_req(req); > > if (bufflen && blk_rq_map_kern(sdev->request_queue, req, > buffer, bufflen, GFP_NOIO)) > goto out; -->just goto out may be we should init sshdr in __scsi_execute? which is the simplest way, and do not lose anyone. If we init sshdr in the callers, maybe we will lose some function. + /* +* Zero-initialize sshdr for those callers that check the *sshdr +* contents even if no sense data is available. +*/ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + > >> Cheers, >> >> Hannes
Re: [PATCH 0/2] qla2xxx: Fixes for the driver
Himanshu, > This series has couple bug fixes for the driver. > > First patch addresses initialization error with the newer adapter on a > blade systems. > > Second patch adds protection for accidental flash corruption using SysFS > path. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [RFC PATCH V4 2/2] scsi: core: don't limit per-LUN queue depth for SSD
On Fri, Oct 18, 2019 at 12:00:07AM +0530, Kashyap Desai wrote: > > On 10/9/19 2:32 AM, Ming Lei wrote: > > > @@ -354,7 +354,8 @@ void scsi_device_unbusy(struct scsi_device *sdev, > > struct scsi_cmnd *cmd) > > > if (starget->can_queue > 0) > > > atomic_dec(&starget->target_busy); > > > > > > - atomic_dec(&sdev->device_busy); > > > + if (!blk_queue_nonrot(sdev->request_queue)) > > > + atomic_dec(&sdev->device_busy); > > > } > > > > > > > Hi Ming, > > > > Does this patch impact the meaning of the queue_depth sysfs attribute (see > > also sdev_store_queue_depth()) and also the queue depth ramp up/down > > mechanism (see also scsi_handle_queue_ramp_up())? Have you considered to > > enable/disable busy tracking per LUN depending on whether or not sdev- > > >queue_depth < shost->can_queue? > > > > The megaraid and mpt3sas drivers read sdev->device_busy directly. Is the > > current version of this patch compatible with these drivers? > > We need to know per scsi device outstanding in mpt3sas and megaraid_sas > driver. Is the READ done in fast path or slow path? If it is on slow path, it should be easy to do via blk_mq_in_flight_rw(). > Can we get supporting API from block layer (through SML) ? something > similar to "atomic_read(&hctx->nr_active)" which can be derived from > sdev->request_queue->hctx ? > At least for those driver which is nr_hw_queue = 1, it will be useful and we > can avoid sdev->device_busy dependency. If you mean to add new atomic counter, we just move the .device_busy into blk-mq, that can become new bottleneck. thanks, Ming
Re: [PATCH 18/24] st: return error code in st_scsi_execute()
On 2019-10-21 23:28, Hannes Reinecke wrote:
> On 10/21/19 6:41 PM, Bart Van Assche wrote:
>> On 10/21/19 2:53 AM, Hannes Reinecke wrote:
>>> We should return the actual error code in st_scsi_execute(),
>>> avoiding the need to use DRIVER_ERROR.
>>>
>>> Signed-off-by: Hannes Reinecke
>>> ---
>>> drivers/scsi/st.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
>>> index e3266a64a477..5f38369cc62f 100644
>>> --- a/drivers/scsi/st.c
>>> +++ b/drivers/scsi/st.c
>>> @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request
>>> *SRpnt, const unsigned char *cmd,
>>> data_direction == DMA_TO_DEVICE ?
>>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0);
>>> if (IS_ERR(req))
>>> - return DRIVER_ERROR << 24;
>>> + return PTR_ERR(req);
>>> rq = scsi_req(req);
>>> req->rq_flags |= RQF_QUIET;
>>> @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request
>>> *SRpnt, const unsigned char *cmd,
>>> GFP_KERNEL);
>>> if (err) {
>>> blk_put_request(req);
>>> - return DRIVER_ERROR << 24;
>>> + return err;
>>> }
>>> }
>>
>> The patch description looks confusing to me. Is it perhaps because the
>> caller compares the st_scsi_execute() return value with zero and doesn't
>> use the return value in any other way that it is fine to return an
>> integer error code instead of a SCSI status?
>>
> Yes. The caller does:
>
> ret = st_scsi_execute(SRpnt, cmd, direction, NULL, bytes, timeout,
> retries);
> if (ret) {
> /* could not allocate the buffer or request was too large */
> (STp->buffer)->syscall_result = (-EBUSY);
> (STp->buffer)->last_SRpnt = NULL;
>
> So it's immaterial _what_ we return here as long as it's non-zero.
Please make this clear in the patch description. I think that will make
this patch easier to review.
Thanks,
Bart.
Re: [PATCH 06/24] scsi: change status_byte() to return the standard SCSI status
On 10/21/19 11:53 AM, Hannes Reinecke wrote: Instead of returning the linux-special status (which is shifted by 1 to the right) change the status_byte() macro to return the correct SCSI standard status. And audit all callers to handle this change. Signed-off-by: Hannes Reinecke --- drivers/scsi/53c700.c| 6 +++--- drivers/scsi/NCR5380.c | 2 +- drivers/scsi/arm/acornscsi.c | 10 - drivers/scsi/arm/fas216.c| 10 - drivers/scsi/dc395x.c| 8 +++- drivers/scsi/scsi.c | 2 +- drivers/scsi/scsi_error.c| 48 ++-- drivers/scsi/scsi_lib.c | 2 +- drivers/scsi/sg.c| 4 ++-- include/scsi/scsi.h | 2 +- 10 files changed, 46 insertions(+), 48 deletions(-) diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 5339baadc082..de52632c6022 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -207,7 +207,7 @@ static inline int scsi_is_wlun(u64 lun) * host_byte = set by low-level driver to indicate status. * driver_byte = set by mid-level. */ -#define status_byte(result) (((result) >> 1) & 0x7f) +#define status_byte(result) (((result)) & 0xff) drop the now unnecessary additional parentheses pair around (result)?: +#define status_byte(result) ((result) & 0xff) #define msg_byte(result)(((result) >> 8) & 0xff) #define host_byte(result) (((result) >> 16) & 0xff) #define driver_byte(result) (((result) >> 24) & 0xff) -- Mit freundlichen Gruessen / Kind regards Steffen Maier Linux on IBM Z Development https://www.ibm.com/privacy/us/en/ IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Matthias Hartmann Geschaeftsfuehrung: Dirk Wittkopp Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 12/24] scsi: introduce scsi_build_sense()
On 10/21/19 11:53 AM, Hannes Reinecke wrote:
Introduce scsi_build_sense() as a wrapper around
scsi_build_sense_buffer() to format the buffer and set
the correct SCSI status.
Signed-off-by: Hannes Reinecke
---
drivers/s390/scsi/zfcp_scsi.c | 5 +--
16 files changed, 60 insertions(+), 128 deletions(-)
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index e9ded2befa0d..da52d7649f4d 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -834,10 +834,7 @@ void zfcp_scsi_set_prot(struct zfcp_adapter *adapter)
*/
void zfcp_scsi_dif_sense_error(struct scsi_cmnd *scmd, int ascq)
{
- scsi_build_sense_buffer(1, scmd->sense_buffer,
- ILLEGAL_REQUEST, 0x10, ascq);
- set_driver_byte(scmd, DRIVER_SENSE);
- scmd->result |= SAM_STAT_CHECK_CONDITION;
+ scsi_build_sense(scmd, 1, ILLEGAL_REQUEST, 0x10, ascq);
set_host_byte(scmd, DID_SOFT_ERROR);
}
looks like a non-functional change for zfcp, so for this part:
Acked-by: Steffen Maier # for zfcp
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a0db8d8766a8..2babf6df8066 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3117,3 +3117,21 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int
*rel_id)
return group_id;
}
EXPORT_SYMBOL(scsi_vpd_tpg_id);
+
+/**
+ * scsi_build_sense - build sense data for a command
minor: I suppose kdoc&sphnix parse and render it correctly? Because
Documentation/doc-guide/kernel-doc.rst says the format for function kdoc has
"()" as function name suffix:
+ * scsi_build_sense() - build sense data for a command
+ * @scmd: scsi command for which the sense should be formatted
+ * @desc: Sense format (non-zero == descriptor format,
+ * 0 == fixed format)
Looks like this has already been like that. Not sure if this patch set touches
every user of scsi_build_sense{_buffer}(). It would be nice to have meaningful
identifiers for values passed to @desc, e.g. something like the following
instead of "magic" zero and non-zero:
enum scsi_sense_format {
SCSI_SENSE_FIXED = 0,
SCSI_SENSE_DESCRIPTOR
};
+ * @key: Sense key
+ * @asc: Additional sense code
+ * @ascq: Additional sense code qualifier
+ *
+ **/
minor:
+ */
[no double star at kdoc end?]
+void scsi_build_sense(struct scsi_cmnd *scmd, int desc, u8 key, u8 asc, u8
ascq)
+{
+ scsi_build_sense_buffer(desc, scmd->sense_buffer, key, asc, ascq);
+ scmd->result = (DRIVER_SENSE << 24) | (DID_OK << 16) |
+ SAM_STAT_CHECK_CONDITION;
While this is scsi_lib and thus "internal" helper code, I wonder if this should
nonetheless use the helper functions to access and build scmd->result in order
to have the error-prone bit shifts in only one central place?:
scmd->result = SAM_STAT_CHECK_CONDITION;
set_driver_byte(scmd, DRIVER_SENSE);
set_host_byte(scmd, DID_OK);
+}
+EXPORT_SYMBOL_GPL(scsi_build_sense);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6932d91472d5..9b9ca629097d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -338,4 +338,7 @@ static inline unsigned scsi_transfer_length(struct
scsi_cmnd *scmd)
return xfer_len;
}
+extern void scsi_build_sense(struct scsi_cmnd *scmd, int desc,
+u8 key, u8 asc, u8 ascq);
+
#endif /* _SCSI_SCSI_CMND_H */
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 19/24] scsi_ioctl: return error code when blk_map_user() fails
On 10/21/19 6:44 PM, Bart Van Assche wrote:
> On 10/21/19 2:53 AM, Hannes Reinecke wrote:
>> When failing to map the user buffer we should return the actual
>> error code, avoiding the usage of DRIVER_ERROR.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> block/scsi_ioctl.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
>> index f5e0ad65e86a..1ab1b8d9641c 100644
>> --- a/block/scsi_ioctl.c
>> +++ b/block/scsi_ioctl.c
>> @@ -485,9 +485,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct
>> gendisk *disk, fmode_t mode,
>> break;
>> }
>> - if (bytes && blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO)) {
>> - err = DRIVER_ERROR << 24;
>> - goto error;
>> + if (bytes) {
>> + err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO);
>> + if (err)
>> + goto error;
>> }
>> blk_execute_rq(q, disk, rq, 0);
>
> Since sg_scsi_ioctl() is used to implement SCSI_IOCTL_SEND_COMMAND, does
> this patch change the ABI between user space and kernel in a
> backwards-incompatible way?
>
Not really. We do change the return code, but sg_scsi_ioctl() already
returns negative POSIX error numbers on failure. And, in fact, this
position is the _only_ possible case where we return something else than
either an errno or a SCSI status. Which arguably is an error even in the
current code.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 18/24] st: return error code in st_scsi_execute()
On 10/21/19 6:41 PM, Bart Van Assche wrote:
> On 10/21/19 2:53 AM, Hannes Reinecke wrote:
>> We should return the actual error code in st_scsi_execute(),
>> avoiding the need to use DRIVER_ERROR.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/scsi/st.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
>> index e3266a64a477..5f38369cc62f 100644
>> --- a/drivers/scsi/st.c
>> +++ b/drivers/scsi/st.c
>> @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request
>> *SRpnt, const unsigned char *cmd,
>> data_direction == DMA_TO_DEVICE ?
>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0);
>> if (IS_ERR(req))
>> - return DRIVER_ERROR << 24;
>> + return PTR_ERR(req);
>> rq = scsi_req(req);
>> req->rq_flags |= RQF_QUIET;
>> @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request
>> *SRpnt, const unsigned char *cmd,
>> GFP_KERNEL);
>> if (err) {
>> blk_put_request(req);
>> - return DRIVER_ERROR << 24;
>> + return err;
>> }
>> }
>
> The patch description looks confusing to me. Is it perhaps because the
> caller compares the st_scsi_execute() return value with zero and doesn't
> use the return value in any other way that it is fine to return an
> integer error code instead of a SCSI status?
>
Yes. The caller does:
ret = st_scsi_execute(SRpnt, cmd, direction, NULL, bytes, timeout,
retries);
if (ret) {
/* could not allocate the buffer or request was too large */
(STp->buffer)->syscall_result = (-EBUSY);
(STp->buffer)->last_SRpnt = NULL;
So it's immaterial _what_ we return here as long as it's non-zero.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 11/24] advansys: kill driver_defined status byte accessors
On 10/21/19 6:37 PM, Bart Van Assche wrote:
> On 10/21/19 2:53 AM, Hannes Reinecke wrote:
>> @@ -6021,43 +6015,28 @@ static void adv_isr_callback(ADV_DVC_VAR
>> *adv_dvc_varp, ADV_SCSI_REQ_Q *scsiqp)
>> ASC_DBG(2, "SAM_STAT_CHECK_CONDITION\n");
>> ASC_DBG_PRT_SENSE(2, scp->sense_buffer,
>> SCSI_SENSE_BUFFERSIZE);
>> - /*
>> - * Note: The 'status_byte()' macro used by
>> - * target drivers defined in scsi.h shifts the
>> - * status byte returned by host drivers right
>> - * by 1 bit. This is why target drivers also
>> - * use right shifted status byte definitions.
>> - * For instance target drivers use
>> - * CHECK_CONDITION, defined to 0x1, instead of
>> - * the SCSI defined check condition value of
>> - * 0x2. Host drivers are supposed to return
>> - * the status byte as it is defined by SCSI.
>> - */
>> - scp->result = DRIVER_BYTE(DRIVER_SENSE) |
>> - STATUS_BYTE(scsiqp->scsi_status);
>> - } else {
>> - scp->result = STATUS_BYTE(scsiqp->scsi_status);
>> }
>> + scp->result = status_byte(scsiqp->scsi_status);
>> break;
>
> Did you really want to delete the code that sets DRIVER_SENSE?
>
Yes. SAM_STAT_CHECK_CONDITION is already set, and the whole point of
this patchset was to drop the DRIVER_SENSE usage.
>> @@ -6789,47 +6768,30 @@ static void asc_isr_callback(ASC_DVC_VAR
>> *asc_dvc_varp, ASC_QDONE_INFO *qdonep)
>> ASC_DBG(2, "SAM_STAT_CHECK_CONDITION\n");
>> ASC_DBG_PRT_SENSE(2, scp->sense_buffer,
>> SCSI_SENSE_BUFFERSIZE);
>> - /*
>> - * Note: The 'status_byte()' macro used by
>> - * target drivers defined in scsi.h shifts the
>> - * status byte returned by host drivers right
>> - * by 1 bit. This is why target drivers also
>> - * use right shifted status byte definitions.
>> - * For instance target drivers use
>> - * CHECK_CONDITION, defined to 0x1, instead of
>> - * the SCSI defined check condition value of
>> - * 0x2. Host drivers are supposed to return
>> - * the status byte as it is defined by SCSI.
>> - */
>> - scp->result = DRIVER_BYTE(DRIVER_SENSE) |
>> - STATUS_BYTE(qdonep->d3.scsi_stat);
>> - } else {
>> - scp->result = STATUS_BYTE(qdonep->d3.scsi_stat);
>> }
>> + scp->result = status_byte(qdonep->d3.scsi_stat);
>> break;
>
> Same comment here: did you really want to delete the code that sets
> DRIVER_SENSE?
>
See above: yes.
That was kinda the point of this patchset.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH RFC 00/24] scsi: Revamp result values
On 10/21/19 8:32 PM, Douglas Gilbert wrote:
> On 2019-10-21 5:52 a.m., Hannes Reinecke wrote:
>> Hi all,
>>
>> the 'result' field in the SCSI command is defined as having
>> 4 fields. The top byte is declared as a 'driver_byte', where the
>> driver can signal some internal status back to the midlayer.
>> However, there is quite a bit of overlap between the driver_byte
>> and the host_byte, resulting in the driver_byte being used in
>> very few places, and mostly in legacy drivers.
>> Additionally, we have _two_ sets of definitions for the
>> last byte (status byte), which can specified either in SAM terms
>> or in the linux-specific terms, which are shifted right by one
>> from the SAM ones.
>> Needless to say, the linux-specific ones are declared obsolete
>> for years now.
>> And to make the confusion complete, both the status byte and
>> the driver byte have a byte for a valid sense code, resulting
>> in quite some confusion which of those bits to check.
>>
>> This patchset does several things:
>> - remove the linux-specific status byte definitions, and use
>> the SAM values throughout
>> - replace the driver-byte values with either SAM ones (for sense
>> code checking) or host-byte definitions
>> - remove the driver-byte definitions
>
> This is a brave change proposal. The masked_status has been tricked
> up so it won't break user code. However the driver byte is exposed
> by the sg v2, v3 and v4 interfaces which means via bsg device nodes,
> sg devices nodes and many other block storage device nodes (e.g.
> /dev/sdc and /dev/st1) via:
> ioctl(, SG_IO, ptr_to_v3_interface) .
>
> So if there is any user space code out there that checks the
> driver byte (e.g. 'sg_io_hdr::driver_status & DRIVER_SENSE') do we
> have a problem?
>
> If so, we could hack the DRIVER_SENSE case *** by putting it back
> for the user space to see when the driver (e.g. sg) knows there
> is sense data. What about the other values?
>
>> As usual, comments and reviews are welcome.
>
> It is hard to make an omelette without breaking some eggs.
>
> Doug Gilbert
>
>> Please note, commit 66cf50e65b18 ("scsi: qla2xxx: fixup incorrect
>> usage of host_byte") from 5.4/scsi-fixes is a prerequisite for
>> this patch series.
>
>
>
> *** Here is a snippet from sg3_utils library code:
>
> if ((SAM_STAT_CHECK_CONDITION == scsi_status) ||
> (SAM_STAT_COMMAND_TERMINATED == scsi_status) ||
> (SG_LIB_DRIVER_SENSE == masked_driver_status))
> return sg_err_category_sense(sense_buffer, sb_len);
>
> Due to the logical OR, as long as SAM_STAT_CHECK_CONDITION is set
> whenever there is sense, then we don't care about DRIVER_SENSE.
>
> I believe this code comes from the days before auto-sense when say a
> READ(6) would fail, send back a CHECK_CONDITION and the host would then
> need to issue a REQUEST SENSE command to get the sense data. However
> REQUEST SENSE could itself yield a CHECK_CONDITION. Hence DRIVER_SENSE
> set, SAM_STAT_CHECK_CONDITION clear could be interpreted as the
> initial command failing and the follow-up REQUEST SENSE succeeded; if
> they are both set, then both commands failed (e.g. the disk has gone
> away).
Well, the easier explanation is that not every driver sets DRIVER_SENSE;
some do, some don't, relying on CHECK_CONDITION here.
Which also means that any code relying on DRIVER_SENSE alone would break
even today.
So really I don't think this should break anything; but if the consensus
is that we need to fake DRIVER_SENSE for userland ABI stability so be it.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 13/24] scsi: Kill DRIVER_SENSE
On 10/22/19 1:44 AM, Finn Thain wrote: > >> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c >> b/drivers/scsi/megaraid/megaraid_sas_base.c >> index c40fbea06cc5..649f9610ca72 100644 >> --- a/drivers/scsi/megaraid/megaraid_sas_base.c >> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c >> @@ -1,3 +1,4 @@ >> + >> // SPDX-License-Identifier: GPL-2.0-or-later >> /* >> * Linux MegaRAID driver for SAS based RAID controllers > > Typo? > Indeed. Will fix it up. >> index 59443e0184fd..d6ecb773c512 100644 >> --- a/drivers/scsi/scsi.c >> +++ b/drivers/scsi/scsi.c >> @@ -203,8 +203,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) >> * If we have valid sense information, then some kind of recovery >> * must have taken place. Make a note of this. >> */ >> -if (SCSI_SENSE_VALID(cmd)) >> -cmd->result |= (DRIVER_SENSE << 24); >> +if (SCSI_SENSE_VALID(cmd) && status_byte(cmd->result) == SAM_STAT_GOOD) >> +set_status_byte(cmd, SAM_STAT_CHECK_CONDITION); > > This means that a REQUEST SENSE command can never result in SAM_STAT_GOOD, > right? Are there implications for higher layers? > Hmm. Blasted REQUEST SENSE. Indeed a REQUEST SENSE command should never return with CHECK_CONDITION. But then a REQUEST SENSE command returns with the sense code in the payload, which equally is not something which is expected. I'll be having a look here. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 12/24] scsi: introduce scsi_build_sense()
On 10/22/19 1:31 AM, Finn Thain wrote:
>
> On Mon, 21 Oct 2019, Hannes Reinecke wrote:
>
>> Introduce scsi_build_sense() as a wrapper around
>> scsi_build_sense_buffer() to format the buffer and set
>> the correct SCSI status.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/ata/libata-scsi.c | 7 ++--
>> drivers/s390/scsi/zfcp_scsi.c | 5 +--
>> drivers/scsi/3w-.c| 3 +-
>> drivers/scsi/libiscsi.c | 5 +--
>> drivers/scsi/lpfc/lpfc_scsi.c | 30 -
>> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +--
>> drivers/scsi/mvumi.c | 5 +--
>> drivers/scsi/myrb.c | 61
>> ---
>> drivers/scsi/myrs.c | 9 ++
>> drivers/scsi/ps3rom.c | 3 +-
>> drivers/scsi/qla2xxx/qla_isr.c| 15 ++---
>> drivers/scsi/scsi_debug.c | 11 +++
>> drivers/scsi/scsi_lib.c | 18 +++
>> drivers/scsi/smartpqi/smartpqi_init.c | 3 +-
>> drivers/scsi/stex.c | 5 +--
>> include/scsi/scsi_cmnd.h | 3 ++
>> 16 files changed, 60 insertions(+), 128 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index b197d2fbe3f8..0fd3cb8e4e49 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -342,9 +342,7 @@ void ata_scsi_set_sense(struct ata_device *dev, struct
>> scsi_cmnd *cmd,
>> if (!cmd)
>> return;
>>
>> -cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
>> -
>> -scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
>> +scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>> }
>>
>> void ata_scsi_set_sense_information(struct ata_device *dev,
>> @@ -1092,8 +1090,7 @@ static void ata_gen_passthru_sense(struct
>> ata_queued_cmd *qc)
>> * ATA PASS-THROUGH INFORMATION AVAILABLE
>> * Always in descriptor format sense.
>> */
>> -scsi_build_sense_buffer(1, cmd->sense_buffer,
>> -RECOVERED_ERROR, 0, 0x1D);
>> +scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
>> }
>>
>> if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
>> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
>> index e9ded2befa0d..da52d7649f4d 100644
>> --- a/drivers/s390/scsi/zfcp_scsi.c
>> +++ b/drivers/s390/scsi/zfcp_scsi.c
>> @@ -834,10 +834,7 @@ void zfcp_scsi_set_prot(struct zfcp_adapter *adapter)
>> */
>> void zfcp_scsi_dif_sense_error(struct scsi_cmnd *scmd, int ascq)
>> {
>> -scsi_build_sense_buffer(1, scmd->sense_buffer,
>> -ILLEGAL_REQUEST, 0x10, ascq);
>> -set_driver_byte(scmd, DRIVER_SENSE);
>> -scmd->result |= SAM_STAT_CHECK_CONDITION;
>> +scsi_build_sense(scmd, 1, ILLEGAL_REQUEST, 0x10, ascq);
>> set_host_byte(scmd, DID_SOFT_ERROR);
>> }
>>
>> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
>> index 79eca8f1fd05..381723634c13 100644
>> --- a/drivers/scsi/3w-.c
>> +++ b/drivers/scsi/3w-.c
>> @@ -1981,8 +1981,7 @@ static int tw_scsi_queue_lck(struct scsi_cmnd *SCpnt,
>> void (*done)(struct scsi_c
>> printk(KERN_NOTICE "3w-: scsi%d: Unknown scsi
>> opcode: 0x%x\n", tw_dev->host->host_no, *command);
>> tw_dev->state[request_id] = TW_S_COMPLETED;
>> tw_state_request_finish(tw_dev, request_id);
>> -SCpnt->result = (DRIVER_SENSE << 24) |
>> SAM_STAT_CHECK_CONDITION;
>> -scsi_build_sense_buffer(1, SCpnt->sense_buffer,
>> ILLEGAL_REQUEST, 0x20, 0);
>> +scsi_build_sense(SCpnt, 1, ILLEGAL_REQUEST, 0x20, 0);
>> done(SCpnt);
>> retval = 0;
>> }
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index ebd47c0cf9e9..9c85d7902faa 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -813,10 +813,7 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn,
>> struct iscsi_hdr *hdr,
>>
>> ascq = session->tt->check_protection(task, §or);
>> if (ascq) {
>> -sc->result = DRIVER_SENSE << 24 |
>> - SAM_STAT_CHECK_CONDITION;
>> -scsi_build_sense_buffer(1, sc->sense_buffer,
>> -ILLEGAL_REQUEST, 0x10, ascq);
>> +scsi_build_sense(sc, 1, ILLEGAL_REQUEST, 0x10, ascq);
>> scsi_set_sense_information(sc->sense_buffer,
>> SCSI_SENSE_BUFFERSIZE,
>> sector);
>> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
>> index f06f63e58596..aa8431fe9c1f 100644
>> --- a/drivers/scsi/lpfc/lpfc_scsi.
Re: [PATCH 05/24] scsi: use standard SAM status codes
On 10/22/19 1:17 AM, Finn Thain wrote: > On Mon, 21 Oct 2019, Hannes Reinecke wrote: > >> Use standard SAM status codes and omit the explicit shift to convert >> to linus-specific ones. > > "Linux-specific". > Indeed, it was Linus who did that :-) Will be fixing it up. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 03/24] wd33c93: use SCSI status
On 10/22/19 1:16 AM, Finn Thain wrote:
> On Mon, 21 Oct 2019, Hannes Reinecke wrote:
>
>> Use standard SCSI status and drop usage of the linux-specific ones.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/scsi/wd33c93.c | 21 -
>> 1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
>> index f81046f0e68a..98e04a7b9d63 100644
>> --- a/drivers/scsi/wd33c93.c
>> +++ b/drivers/scsi/wd33c93.c
>> @@ -1176,10 +1176,8 @@ wd33c93_intr(struct Scsi_Host *instance)
>> if (cmd->SCp.Status == ILLEGAL_STATUS_BYTE)
>> cmd->SCp.Status = lun;
>> if (cmd->cmnd[0] == REQUEST_SENSE
>> -&& cmd->SCp.Status != GOOD)
>> -cmd->result =
>> -(cmd->
>> - result & 0x00) | (DID_ERROR << 16);
>> +&& cmd->SCp.Status != SAM_STAT_GOOD)
>> +set_host_byte(cmd, DID_ERROR);
>
> This isn't obviously equivalent. Perhaps the set_host_byte() changes
> should be done in a separate patch to the SAM_STAT_FOO changes (?)
>
Yes, indeed.
Will be fixing it up in the next round.
>> else
>> cmd->result =
>> cmd->SCp.Status | (cmd->SCp.Message << 8);
>> @@ -1262,9 +1260,8 @@ wd33c93_intr(struct Scsi_Host *instance)
>> hostdata->connected = NULL;
>> hostdata->busy[cmd->device->id] &= ~(1 << (cmd->device->lun &
>> 0xff));
>> hostdata->state = S_UNCONNECTED;
>> -if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status != GOOD)
>> -cmd->result =
>> -(cmd->result & 0x00) | (DID_ERROR << 16);
>> +if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status !=
>> SAM_STAT_GOOD)
>> +set_host_byte(cmd, DID_ERROR);
>
> Same.
>
>> else
>> cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
>> cmd->scsi_done(cmd);
>> @@ -1294,12 +1291,10 @@ wd33c93_intr(struct Scsi_Host *instance)
>> hostdata->connected = NULL;
>> hostdata->busy[cmd->device->id] &= ~(1 <<
>> (cmd->device->lun & 0xff));
>> hostdata->state = S_UNCONNECTED;
>> -DB(DB_INTR, printk(":%d", cmd->SCp.Status))
>> -if (cmd->cmnd[0] == REQUEST_SENSE
>> -&& cmd->SCp.Status != GOOD)
>> -cmd->result =
>> -(cmd->
>> - result & 0x00) | (DID_ERROR << 16);
>> +DB(DB_INTR, printk(":%d", cmd->SCp.Status));
>> +if (cmd->cmnd[0] == REQUEST_SENSE
>> +&& cmd->SCp.Status != SAM_STAT_GOOD)
>> +set_host_byte(cmd->result, DID_ERROR);
>
> Same.
>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 10/24] scsi: introduce set_status_byte()
On 10/22/19 12:12 AM, Finn Thain wrote:
> On Mon, 21 Oct 2019, Hannes Reinecke wrote:
>
>> To be in-line with the other set_XX_byte() functions.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> include/scsi/scsi_cmnd.h | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index 91bd749a02f7..6932d91472d5 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -307,6 +307,11 @@ static inline struct scsi_data_buffer *scsi_prot(struct
>> scsi_cmnd *cmd)
>> #define scsi_for_each_prot_sg(cmd, sg, nseg, __i) \
>> for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
>>
>> +static inline void set_status_byte(struct scsi_cmnd *cmd, char status)
>> +{
>> +cmd->result = (cmd->result & 0xff00) | status;
>
> Is sign-extension desirable here? Do callers need it?
>
It'll be a theoretical issue, as a status value with the top bit set
would be invalid anyway.
But for consistencies sake I'll make it an unsigned char in the next
iteration.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/21 21:06, Hannes Reinecke wrote: > On 10/21/19 3:49 AM, zhengbin (A) wrote: >> On 2019/10/18 21:43, Martin K. Petersen wrote: >>> Hannes, >>> The one thing which I patently don't like is the ambivalence between DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ of them is set? IE what would be the correct way of action if DRIVER_SENSE is not set, but we have a valid sense code? Or the other way around? >>> I agree, it's a mess. >>> >>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >>> and most arcane code in SCSI) >>> But more important, from a quick glance not all drivers set the DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is never evaluated after this patchset. >>> And yet we appear to have several code paths where sense evaluation is >>> contingent on DRIVER_SENSE. So no matter what, behavior might >>> change if we enforce consistent semantics. *sigh* >> So what should we do to prevent unit-value access of sshdr? >> > Where do you see it? > >From my reading, __scsi_execute() is clearing sshdr by way of > > __scsi_execute() > -> scsi_normalize_sense() > -> memset(sshdr) __scsi_execute req = blk_get_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); if (IS_ERR(req)) return ret; -->just return rq = scsi_req(req); if (bufflen && blk_rq_map_kern(sdev->request_queue, req, buffer, bufflen, GFP_NOIO)) goto out; -->just goto out > > Cheers, > > Hannes
Re: [PATCH 13/24] scsi: Kill DRIVER_SENSE
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c > b/drivers/scsi/megaraid/megaraid_sas_base.c > index c40fbea06cc5..649f9610ca72 100644 > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > @@ -1,3 +1,4 @@ > + > // SPDX-License-Identifier: GPL-2.0-or-later > /* > * Linux MegaRAID driver for SAS based RAID controllers Typo? > index 59443e0184fd..d6ecb773c512 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -203,8 +203,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) >* If we have valid sense information, then some kind of recovery >* must have taken place. Make a note of this. >*/ > - if (SCSI_SENSE_VALID(cmd)) > - cmd->result |= (DRIVER_SENSE << 24); > + if (SCSI_SENSE_VALID(cmd) && status_byte(cmd->result) == SAM_STAT_GOOD) > + set_status_byte(cmd, SAM_STAT_CHECK_CONDITION); This means that a REQUEST SENSE command can never result in SAM_STAT_GOOD, right? Are there implications for higher layers? --
Re: [PATCH 12/24] scsi: introduce scsi_build_sense()
On Mon, 21 Oct 2019, Hannes Reinecke wrote:
> Introduce scsi_build_sense() as a wrapper around
> scsi_build_sense_buffer() to format the buffer and set
> the correct SCSI status.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/ata/libata-scsi.c | 7 ++--
> drivers/s390/scsi/zfcp_scsi.c | 5 +--
> drivers/scsi/3w-.c| 3 +-
> drivers/scsi/libiscsi.c | 5 +--
> drivers/scsi/lpfc/lpfc_scsi.c | 30 -
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +--
> drivers/scsi/mvumi.c | 5 +--
> drivers/scsi/myrb.c | 61
> ---
> drivers/scsi/myrs.c | 9 ++
> drivers/scsi/ps3rom.c | 3 +-
> drivers/scsi/qla2xxx/qla_isr.c| 15 ++---
> drivers/scsi/scsi_debug.c | 11 +++
> drivers/scsi/scsi_lib.c | 18 +++
> drivers/scsi/smartpqi/smartpqi_init.c | 3 +-
> drivers/scsi/stex.c | 5 +--
> include/scsi/scsi_cmnd.h | 3 ++
> 16 files changed, 60 insertions(+), 128 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b197d2fbe3f8..0fd3cb8e4e49 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -342,9 +342,7 @@ void ata_scsi_set_sense(struct ata_device *dev, struct
> scsi_cmnd *cmd,
> if (!cmd)
> return;
>
> - cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> -
> - scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
> + scsi_build_sense(cmd, d_sense, sk, asc, ascq);
> }
>
> void ata_scsi_set_sense_information(struct ata_device *dev,
> @@ -1092,8 +1090,7 @@ static void ata_gen_passthru_sense(struct
> ata_queued_cmd *qc)
>* ATA PASS-THROUGH INFORMATION AVAILABLE
>* Always in descriptor format sense.
>*/
> - scsi_build_sense_buffer(1, cmd->sense_buffer,
> - RECOVERED_ERROR, 0, 0x1D);
> + scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> }
>
> if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index e9ded2befa0d..da52d7649f4d 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -834,10 +834,7 @@ void zfcp_scsi_set_prot(struct zfcp_adapter *adapter)
> */
> void zfcp_scsi_dif_sense_error(struct scsi_cmnd *scmd, int ascq)
> {
> - scsi_build_sense_buffer(1, scmd->sense_buffer,
> - ILLEGAL_REQUEST, 0x10, ascq);
> - set_driver_byte(scmd, DRIVER_SENSE);
> - scmd->result |= SAM_STAT_CHECK_CONDITION;
> + scsi_build_sense(scmd, 1, ILLEGAL_REQUEST, 0x10, ascq);
> set_host_byte(scmd, DID_SOFT_ERROR);
> }
>
> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
> index 79eca8f1fd05..381723634c13 100644
> --- a/drivers/scsi/3w-.c
> +++ b/drivers/scsi/3w-.c
> @@ -1981,8 +1981,7 @@ static int tw_scsi_queue_lck(struct scsi_cmnd *SCpnt,
> void (*done)(struct scsi_c
> printk(KERN_NOTICE "3w-: scsi%d: Unknown scsi
> opcode: 0x%x\n", tw_dev->host->host_no, *command);
> tw_dev->state[request_id] = TW_S_COMPLETED;
> tw_state_request_finish(tw_dev, request_id);
> - SCpnt->result = (DRIVER_SENSE << 24) |
> SAM_STAT_CHECK_CONDITION;
> - scsi_build_sense_buffer(1, SCpnt->sense_buffer,
> ILLEGAL_REQUEST, 0x20, 0);
> + scsi_build_sense(SCpnt, 1, ILLEGAL_REQUEST, 0x20, 0);
> done(SCpnt);
> retval = 0;
> }
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ebd47c0cf9e9..9c85d7902faa 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -813,10 +813,7 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn,
> struct iscsi_hdr *hdr,
>
> ascq = session->tt->check_protection(task, §or);
> if (ascq) {
> - sc->result = DRIVER_SENSE << 24 |
> - SAM_STAT_CHECK_CONDITION;
> - scsi_build_sense_buffer(1, sc->sense_buffer,
> - ILLEGAL_REQUEST, 0x10, ascq);
> + scsi_build_sense(sc, 1, ILLEGAL_REQUEST, 0x10, ascq);
> scsi_set_sense_information(sc->sense_buffer,
> SCSI_SENSE_BUFFERSIZE,
> sector);
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index f06f63e58596..aa8431fe9c1f 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -2843,10 +2843,7 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struc
Re: [PATCH RFC 00/24] scsi: Revamp result values
On Mon, 21 Oct 2019, Douglas Gilbert wrote: > > > As usual, comments and reviews are welcome. > > It is hard to make an omelette without breaking some eggs. > Coccinelle can minimize the breakage; particularly the straight-forward conversion of (FOO << 1) to SAM_STAT_BAR. -- > Doug Gilbert >
Re: [PATCH 05/24] scsi: use standard SAM status codes
On Mon, 21 Oct 2019, Hannes Reinecke wrote:
> Use standard SAM status codes and omit the explicit shift to convert
> to linus-specific ones.
"Linux-specific".
--
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/ata/libata-scsi.c | 2 +-
> drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
> drivers/scsi/3w-9xxx.c| 5 +++--
> drivers/scsi/3w-sas.c | 3 ++-
> drivers/scsi/3w-.c| 4 ++--
> drivers/scsi/arcmsr/arcmsr_hba.c | 4 ++--
> drivers/scsi/bfa/bfad_im.c| 2 +-
> drivers/scsi/dc395x.c | 18 +-
> drivers/scsi/dpt_i2o.c| 2 +-
> drivers/scsi/gdth.c | 12 ++--
> drivers/scsi/megaraid.c | 10 +-
> drivers/scsi/megaraid/megaraid_mbox.c | 12 ++--
> 12 files changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 76d0f9de767b..b197d2fbe3f8 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -856,7 +856,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct
> ata_device *dev,
> if (cmd->request->rq_flags & RQF_QUIET)
> qc->flags |= ATA_QCFLAG_QUIET;
> } else {
> - cmd->result = (DID_OK << 16) | (QUEUE_FULL << 1);
> + cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
> cmd->scsi_done(cmd);
> }
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index b5960351bec0..4570e3c79ea5 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -2404,7 +2404,7 @@ static int srp_queuecommand(struct Scsi_Host *shost,
> struct scsi_cmnd *scmnd)
>* to reduce queue depth temporarily.
>*/
> scmnd->result = len == -ENOMEM ?
> - DID_OK << 16 | QUEUE_FULL << 1 : DID_ERROR << 16;
> + DID_OK << 16 | SAM_STAT_TASK_SET_FULL : DID_ERROR << 16;
> goto err_iu;
> }
>
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 3337b1e80412..ada77c136f8b 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -1018,7 +1018,8 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev,
> int request_id, int copy_
>
> if (copy_sense) {
> memcpy(tw_dev->srb[request_id]->sense_buffer,
> full_command_packet->header.sense_data, TW_SENSE_DATA_LENGTH);
> - tw_dev->srb[request_id]->result =
> (full_command_packet->command.newcommand.status << 1);
> + tw_dev->srb[request_id]->result =
> + full_command_packet->command.newcommand.status;
> retval = TW_ISR_DONT_RESULT;
> goto out;
> }
> @@ -1342,7 +1343,7 @@ static irqreturn_t twa_interrupt(int irq, void
> *dev_instance)
> /* If error, command failed */
> if (error == 1) {
> /* Ask for a host reset */
> - cmd->result = (DID_OK << 16) |
> (CHECK_CONDITION << 1);
> + cmd->result = (DID_OK << 16) |
> SAM_STAT_CHECK_CONDITION;
> }
>
> /* Report residual bytes for single sgl */
> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index dda6fa857709..d11f62c60877 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c
> @@ -891,7 +891,8 @@ static int twl_fill_sense(TW_Device_Extension *tw_dev,
> int i, int request_id, in
>
> if (copy_sense) {
> memcpy(tw_dev->srb[request_id]->sense_buffer,
> header->sense_data, TW_SENSE_DATA_LENGTH);
> - tw_dev->srb[request_id]->result =
> (full_command_packet->command.newcommand.status << 1);
> + tw_dev->srb[request_id]->result =
> + full_command_packet->command.newcommand.status;
> goto out;
> }
> out:
> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
> index 2b1e0d503020..79eca8f1fd05 100644
> --- a/drivers/scsi/3w-.c
> +++ b/drivers/scsi/3w-.c
> @@ -429,7 +429,7 @@ static int tw_decode_sense(TW_Device_Extension *tw_dev,
> int request_id, int fill
> /* Additional sense code qualifier */
>
> tw_dev->srb[request_id]->sense_buffer[13] = tw_sense_table[i][3];
>
> - tw_dev->srb[request_id]->result =
> (DID_OK << 16) | (CHECK_CONDITION << 1);
> + tw_dev->srb[request_id]->result =
> (DID_OK << 16) | SAM_STAT_CHECK_CONDITION;
> return TW_ISR_DONT_RESULT; /* Special
> case for isr to not over-write result */
>
Re: [PATCH 03/24] wd33c93: use SCSI status
On Mon, 21 Oct 2019, Hannes Reinecke wrote:
> Use standard SCSI status and drop usage of the linux-specific ones.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/wd33c93.c | 21 -
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> index f81046f0e68a..98e04a7b9d63 100644
> --- a/drivers/scsi/wd33c93.c
> +++ b/drivers/scsi/wd33c93.c
> @@ -1176,10 +1176,8 @@ wd33c93_intr(struct Scsi_Host *instance)
> if (cmd->SCp.Status == ILLEGAL_STATUS_BYTE)
> cmd->SCp.Status = lun;
> if (cmd->cmnd[0] == REQUEST_SENSE
> - && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->
> - result & 0x00) | (DID_ERROR << 16);
> + && cmd->SCp.Status != SAM_STAT_GOOD)
> + set_host_byte(cmd, DID_ERROR);
This isn't obviously equivalent. Perhaps the set_host_byte() changes
should be done in a separate patch to the SAM_STAT_FOO changes (?)
> else
> cmd->result =
> cmd->SCp.Status | (cmd->SCp.Message << 8);
> @@ -1262,9 +1260,8 @@ wd33c93_intr(struct Scsi_Host *instance)
> hostdata->connected = NULL;
> hostdata->busy[cmd->device->id] &= ~(1 << (cmd->device->lun &
> 0xff));
> hostdata->state = S_UNCONNECTED;
> - if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->result & 0x00) | (DID_ERROR << 16);
> + if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status !=
> SAM_STAT_GOOD)
> + set_host_byte(cmd, DID_ERROR);
Same.
> else
> cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
> cmd->scsi_done(cmd);
> @@ -1294,12 +1291,10 @@ wd33c93_intr(struct Scsi_Host *instance)
> hostdata->connected = NULL;
> hostdata->busy[cmd->device->id] &= ~(1 <<
> (cmd->device->lun & 0xff));
> hostdata->state = S_UNCONNECTED;
> - DB(DB_INTR, printk(":%d", cmd->SCp.Status))
> - if (cmd->cmnd[0] == REQUEST_SENSE
> - && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->
> - result & 0x00) | (DID_ERROR << 16);
> + DB(DB_INTR, printk(":%d", cmd->SCp.Status));
> + if (cmd->cmnd[0] == REQUEST_SENSE
> + && cmd->SCp.Status != SAM_STAT_GOOD)
> + set_host_byte(cmd->result, DID_ERROR);
Same.
--
> else
> cmd->result =
> cmd->SCp.Status | (cmd->SCp.Message << 8);
>
Re: [PATCH 10/24] scsi: introduce set_status_byte()
On Mon, 21 Oct 2019, Hannes Reinecke wrote:
> To be in-line with the other set_XX_byte() functions.
>
> Signed-off-by: Hannes Reinecke
> ---
> include/scsi/scsi_cmnd.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 91bd749a02f7..6932d91472d5 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -307,6 +307,11 @@ static inline struct scsi_data_buffer *scsi_prot(struct
> scsi_cmnd *cmd)
> #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)\
> for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
>
> +static inline void set_status_byte(struct scsi_cmnd *cmd, char status)
> +{
> + cmd->result = (cmd->result & 0xff00) | status;
Is sign-extension desirable here? Do callers need it?
--
> +}
> +
> static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
> {
> cmd->result = (cmd->result & 0x00ff) | (status << 8);
>
Re: [PATCH 09/24] scsi: Kill obsolete linux-specific status codes
Hi Hannes,
I love your patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191021]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-Revamp-result-values/20191022-004918
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/scsi/ibmvscsi/ibmvscsi.c: In function 'handle_cmd_rsp':
>> drivers/scsi/ibmvscsi/ibmvscsi.c:989:39: error: 'CHECK_CONDITION' undeclared
>> (first use in this function); did you mean 'H_MR_CONDITION'?
if (((cmnd->result >> 1) & 0x1f) == CHECK_CONDITION)
^~~
H_MR_CONDITION
drivers/scsi/ibmvscsi/ibmvscsi.c:989:39: note: each undeclared identifier is
reported only once for each function it appears in
vim +989 drivers/scsi/ibmvscsi/ibmvscsi.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 968
^1da177e4c3f41 Linus Torvalds 2005-04-16 969 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 970 * handle_cmd_rsp: - Handle
responses from commands
^1da177e4c3f41 Linus Torvalds 2005-04-16 971 * @evt_struct:
srp_event_struct to be handled
^1da177e4c3f41 Linus Torvalds 2005-04-16 972 *
^1da177e4c3f41 Linus Torvalds 2005-04-16 973 * Used as a callback by when
sending scsi cmds.
^1da177e4c3f41 Linus Torvalds 2005-04-16 974 * Gets called by
ibmvscsi_handle_crq()
^1da177e4c3f41 Linus Torvalds 2005-04-16 975 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 976 static void
handle_cmd_rsp(struct srp_event_struct *evt_struct)
^1da177e4c3f41 Linus Torvalds 2005-04-16 977 {
^1da177e4c3f41 Linus Torvalds 2005-04-16 978 struct srp_rsp *rsp =
&evt_struct->xfer_iu->srp.rsp;
^1da177e4c3f41 Linus Torvalds 2005-04-16 979 struct scsi_cmnd *cmnd
= evt_struct->cmnd;
^1da177e4c3f41 Linus Torvalds 2005-04-16 980
ef265673434680 FUJITA Tomonori 2006-03-26 981 if
(unlikely(rsp->opcode != SRP_RSP)) {
^1da177e4c3f41 Linus Torvalds 2005-04-16 982 if
(printk_ratelimit())
6c0a60ec52042e Brian King 2007-06-13 983
dev_warn(evt_struct->hostdata->dev,
15c9274699e8b6 Tyrel Datwyler 2016-12-07 984
"bad SRP RSP type %#02x\n", rsp->opcode);
^1da177e4c3f41 Linus Torvalds 2005-04-16 985 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 986
^1da177e4c3f41 Linus Torvalds 2005-04-16 987 if (cmnd) {
c3a3b55ae80a0d Brian King 2008-04-25 988 cmnd->result |=
rsp->status;
^1da177e4c3f41 Linus Torvalds 2005-04-16 @989 if
(((cmnd->result >> 1) & 0x1f) == CHECK_CONDITION)
^1da177e4c3f41 Linus Torvalds 2005-04-16 990
memcpy(cmnd->sense_buffer,
ef265673434680 FUJITA Tomonori 2006-03-26 991
rsp->data,
72264eb6dbb909 Anton Blanchard 2013-09-03 992
be32_to_cpu(rsp->sense_data_len));
^1da177e4c3f41 Linus Torvalds 2005-04-16 993
unmap_cmd_data(&evt_struct->iu.srp.cmd,
4dddbc26c3895e James Bottomley 2005-09-06 994
evt_struct,
^1da177e4c3f41 Linus Torvalds 2005-04-16 995
evt_struct->hostdata->dev);
^1da177e4c3f41 Linus Torvalds 2005-04-16 996
ef265673434680 FUJITA Tomonori 2006-03-26 997 if (rsp->flags
& SRP_RSP_FLAG_DOOVER)
72264eb6dbb909 Anton Blanchard 2013-09-03 998
scsi_set_resid(cmnd,
72264eb6dbb909 Anton Blanchard 2013-09-03 999
be32_to_cpu(rsp->data_out_res_cnt));
ef265673434680 FUJITA Tomonori 2006-03-26 1000 else if
(rsp->flags & SRP_RSP_FLAG_DIOVER)
72264eb6dbb909 Anton Blanchard 2013-09-03 1001
scsi_set_resid(cmnd, be32_to_cpu(rsp->data_in_res_cnt));
^1da177e4c3f41 Linus Torvalds 2005-04-16 1002 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1003
^1da177e4c3f41 Linus Torvalds 2005-04-16 1004 if
(evt_struct->cmnd_done)
^1da177e4c3f41 Linus Torvalds 2005-04-16 1005
evt_struct->cmnd_done(cmnd);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1006
Re: [PATCH 03/24] wd33c93: use SCSI status
Hi Hannes,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on v5.4-rc4 next-20191021]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-Revamp-result-values/20191022-004918
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: m68k-multi_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
drivers/scsi/wd33c93.c: In function 'wd33c93_intr':
>> drivers/scsi/wd33c93.c:1297:19: warning: passing argument 1 of
>> 'set_host_byte' makes pointer from integer without a cast [-Wint-conversion]
set_host_byte(cmd->result, DID_ERROR);
^~~
In file included from drivers/scsi/wd33c93.c:79:0:
include/scsi/scsi_cmnd.h:315:20: note: expected 'struct scsi_cmnd *' but
argument is of type 'int'
static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
^
vim +/set_host_byte +1297 drivers/scsi/wd33c93.c
1200
1201 case CSR_SDP:
1202 DB(DB_INTR, printk("SDP"))
1203 hostdata->state = S_RUNNING_LEVEL2;
1204 write_wd33c93(regs, WD_COMMAND_PHASE, 0x41);
1205 write_wd33c93_cmd(regs, WD_CMD_SEL_ATN_XFER);
1206 spin_unlock_irqrestore(&hostdata->lock, flags);
1207 break;
1208
1209 case CSR_XFER_DONE | PHS_MESS_OUT:
1210 case CSR_UNEXP | PHS_MESS_OUT:
1211 case CSR_SRV_REQ | PHS_MESS_OUT:
1212 DB(DB_INTR, printk("MSG_OUT="))
1213
1214 /* To get here, we've probably requested MESSAGE_OUT and have
1215 * already put the correct bytes in outgoing_msg[] and filled
1216 * in outgoing_len. We simply send them out to the SCSI bus.
1217 * Sometimes we get MESSAGE_OUT phase when we're not expecting
1218 * it - like when our SDTR message is rejected by a target. Some
1219 * targets send the REJECT before receiving all of the extended
1220 * message, and then seem to go back to MESSAGE_OUT for a byte
1221 * or two. Not sure why, or if I'm doing something wrong to
1222 * cause this to happen. Regardless, it seems that sending
1223 * NOP messages in these situations results in no harm and
1224 * makes everyone happy.
1225 */
1226 if (hostdata->outgoing_len == 0) {
1227 hostdata->outgoing_len = 1;
1228 hostdata->outgoing_msg[0] = NOP;
1229 }
1230 transfer_pio(regs, hostdata->outgoing_msg,
1231 hostdata->outgoing_len, DATA_OUT_DIR,
hostdata);
1232 DB(DB_INTR, printk("%02x", hostdata->outgoing_msg[0]))
1233 hostdata->outgoing_len = 0;
1234 hostdata->state = S_CONNECTED;
1235 spin_unlock_irqrestore(&hostdata->lock, flags);
1236 break;
1237
1238 case CSR_UNEXP_DISC:
1239
1240 /* I think I've seen this after a request-sense that was in response
1241 * to an error condition, but not sure. We certainly need to do
1242 * something when we get this interrupt - the question is 'what?'.
1243 * Let's think positively, and assume some command has finished
1244 * in a legal manner (like a command that provokes a request-sense),
1245 * so we treat it as a normal command-complete-disconnect.
1246 */
1247
1248 /* Make sure that reselection is enabled at this point - it may
1249 * have been turned off for the command that just completed.
1250 */
1251
1252 write_wd33c93(regs, WD_SOURCE_ID, SRCID_ER);
1253 if (cmd == NULL) {
1254 printk(" - Already disconnected! ");
1255 hostdata->state = S_UNCONNECTED;
1256 spin_unlock_irqrestore(&hostdata->lock, flags);
1257 return;
1258 }
1259 DB(DB_INTR, printk("UNEXP_DISC"))
1260 hostdata->connected = NULL;
1261 hostdata->busy[cmd->device->id] &= ~(1 <<
(cmd->device->lun & 0xff));
1262 hostdata->state = S_UNCONNECT
Re: [PATCH RFC 00/24] scsi: Revamp result values
On 2019-10-21 5:52 a.m., Hannes Reinecke wrote:
Hi all,
the 'result' field in the SCSI command is defined as having
4 fields. The top byte is declared as a 'driver_byte', where the
driver can signal some internal status back to the midlayer.
However, there is quite a bit of overlap between the driver_byte
and the host_byte, resulting in the driver_byte being used in
very few places, and mostly in legacy drivers.
Additionally, we have _two_ sets of definitions for the
last byte (status byte), which can specified either in SAM terms
or in the linux-specific terms, which are shifted right by one
from the SAM ones.
Needless to say, the linux-specific ones are declared obsolete
for years now.
And to make the confusion complete, both the status byte and
the driver byte have a byte for a valid sense code, resulting
in quite some confusion which of those bits to check.
This patchset does several things:
- remove the linux-specific status byte definitions, and use
the SAM values throughout
- replace the driver-byte values with either SAM ones (for sense
code checking) or host-byte definitions
- remove the driver-byte definitions
This is a brave change proposal. The masked_status has been tricked
up so it won't break user code. However the driver byte is exposed
by the sg v2, v3 and v4 interfaces which means via bsg device nodes,
sg devices nodes and many other block storage device nodes (e.g.
/dev/sdc and /dev/st1) via:
ioctl(, SG_IO, ptr_to_v3_interface) .
So if there is any user space code out there that checks the
driver byte (e.g. 'sg_io_hdr::driver_status & DRIVER_SENSE') do we
have a problem?
If so, we could hack the DRIVER_SENSE case *** by putting it back
for the user space to see when the driver (e.g. sg) knows there
is sense data. What about the other values?
As usual, comments and reviews are welcome.
It is hard to make an omelette without breaking some eggs.
Doug Gilbert
Please note, commit 66cf50e65b18 ("scsi: qla2xxx: fixup incorrect
usage of host_byte") from 5.4/scsi-fixes is a prerequisite for
this patch series.
*** Here is a snippet from sg3_utils library code:
if ((SAM_STAT_CHECK_CONDITION == scsi_status) ||
(SAM_STAT_COMMAND_TERMINATED == scsi_status) ||
(SG_LIB_DRIVER_SENSE == masked_driver_status))
return sg_err_category_sense(sense_buffer, sb_len);
Due to the logical OR, as long as SAM_STAT_CHECK_CONDITION is set
whenever there is sense, then we don't care about DRIVER_SENSE.
I believe this code comes from the days before auto-sense when say a
READ(6) would fail, send back a CHECK_CONDITION and the host would then
need to issue a REQUEST SENSE command to get the sense data. However
REQUEST SENSE could itself yield a CHECK_CONDITION. Hence DRIVER_SENSE
set, SAM_STAT_CHECK_CONDITION clear could be interpreted as the
initial command failing and the follow-up REQUEST SENSE succeeded; if
they are both set, then both commands failed (e.g. the disk has gone
away).
Re: [PATCH 09/24] scsi: Kill obsolete linux-specific status codes
Hi Hannes,
I love your patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191021]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-Revamp-result-values/20191022-004918
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/scsi/pcmcia/nsp_cs.c: In function 'nsp_queuecommand_lck':
>> drivers/scsi/pcmcia/nsp_cs.c:226:22: error: 'CHECK_CONDITION' undeclared
>> (first use in this function); did you mean 'SEC_CONVERSION'?
SCpnt->SCp.Status = CHECK_CONDITION;
^~~
SEC_CONVERSION
drivers/scsi/pcmcia/nsp_cs.c:226:22: note: each undeclared identifier is
reported only once for each function it appears in
vim +226 drivers/scsi/pcmcia/nsp_cs.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 221
^1da177e4c3f41 Linus Torvalds 2005-04-16 222 show_command(SCpnt);
^1da177e4c3f41 Linus Torvalds 2005-04-16 223
^1da177e4c3f41 Linus Torvalds 2005-04-16 224 data->CurrentSC = SCpnt;
^1da177e4c3f41 Linus Torvalds 2005-04-16 225
^1da177e4c3f41 Linus Torvalds 2005-04-16 @226 SCpnt->SCp.Status =
CHECK_CONDITION;
^1da177e4c3f41 Linus Torvalds 2005-04-16 227 SCpnt->SCp.Message = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 228 SCpnt->SCp.have_data_in =
IO_UNKNOWN;
^1da177e4c3f41 Linus Torvalds 2005-04-16 229 SCpnt->SCp.sent_command = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 230 SCpnt->SCp.phase=
PH_UNDETERMINED;
040cd23242413a Boaz Harrosh 2007-08-16 231 scsi_set_resid(SCpnt,
scsi_bufflen(SCpnt));
^1da177e4c3f41 Linus Torvalds 2005-04-16 232
^1da177e4c3f41 Linus Torvalds 2005-04-16 233 /* setup scratch area
^1da177e4c3f41 Linus Torvalds 2005-04-16 234 SCp.ptr :
buffer pointer
^1da177e4c3f41 Linus Torvalds 2005-04-16 235 SCp.this_residual:
buffer length
^1da177e4c3f41 Linus Torvalds 2005-04-16 236 SCp.buffer : next
buffer
^1da177e4c3f41 Linus Torvalds 2005-04-16 237 SCp.buffers_residual : left
buffers in list
^1da177e4c3f41 Linus Torvalds 2005-04-16 238 SCp.phase:
current state of the command */
040cd23242413a Boaz Harrosh 2007-08-16 239 if (scsi_bufflen(SCpnt)) {
040cd23242413a Boaz Harrosh 2007-08-16 240 SCpnt->SCp.buffer
= scsi_sglist(SCpnt);
^1da177e4c3f41 Linus Torvalds 2005-04-16 241 SCpnt->SCp.ptr
= BUFFER_ADDR;
^1da177e4c3f41 Linus Torvalds 2005-04-16 242
SCpnt->SCp.this_residual= SCpnt->SCp.buffer->length;
040cd23242413a Boaz Harrosh 2007-08-16 243
SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
^1da177e4c3f41 Linus Torvalds 2005-04-16 244 } else {
040cd23242413a Boaz Harrosh 2007-08-16 245 SCpnt->SCp.ptr
= NULL;
040cd23242413a Boaz Harrosh 2007-08-16 246
SCpnt->SCp.this_residual= 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 247 SCpnt->SCp.buffer
= NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 248
SCpnt->SCp.buffers_residual = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 249 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 250
^1da177e4c3f41 Linus Torvalds 2005-04-16 251 if
(nsphw_start_selection(SCpnt) == FALSE) {
^1da177e4c3f41 Linus Torvalds 2005-04-16 252
nsp_dbg(NSP_DEBUG_QUEUECOMMAND, "selection fail");
^1da177e4c3f41 Linus Torvalds 2005-04-16 253 SCpnt->result =
DID_BUS_BUSY << 16;
^1da177e4c3f41 Linus Torvalds 2005-04-16 254 nsp_scsi_done(SCpnt);
^1da177e4c3f41 Linus Torvalds 2005-04-16 255 return 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 256 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 257
^1da177e4c3f41 Linus Torvalds 2005-04-16 258
^1da177e4c3f41 Linus Torvalds 2005-04-16 259
//nsp_dbg(NSP_DEBUG_QUEUECOMMAND, "out");
^1da177e4c3f41 Linus Torvalds 2005-04-16 260 #ifdef NSP_DEBUG
^1da177e4c3f41 Linus Torvalds 2005-04-16 261 data->CmdId++;
^1da177e4c3f41 Linus Torvalds 2005-04-16 262 #endif
^1da177e4c3f41 Linus Torvalds 2005-04-16 263 return 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 264 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 265
:: The code at line 226
Re: [PATCH 19/24] scsi_ioctl: return error code when blk_map_user() fails
On 10/21/19 2:53 AM, Hannes Reinecke wrote:
When failing to map the user buffer we should return the actual
error code, avoiding the usage of DRIVER_ERROR.
Signed-off-by: Hannes Reinecke
---
block/scsi_ioctl.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index f5e0ad65e86a..1ab1b8d9641c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -485,9 +485,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk
*disk, fmode_t mode,
break;
}
- if (bytes && blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO)) {
- err = DRIVER_ERROR << 24;
- goto error;
+ if (bytes) {
+ err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO);
+ if (err)
+ goto error;
}
blk_execute_rq(q, disk, rq, 0);
Since sg_scsi_ioctl() is used to implement SCSI_IOCTL_SEND_COMMAND, does
this patch change the ABI between user space and kernel in a
backwards-incompatible way?
Thanks,
Bart.
Re: [PATCH 18/24] st: return error code in st_scsi_execute()
On 10/21/19 2:53 AM, Hannes Reinecke wrote:
We should return the actual error code in st_scsi_execute(),
avoiding the need to use DRIVER_ERROR.
Signed-off-by: Hannes Reinecke
---
drivers/scsi/st.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e3266a64a477..5f38369cc62f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const
unsigned char *cmd,
data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0);
if (IS_ERR(req))
- return DRIVER_ERROR << 24;
+ return PTR_ERR(req);
rq = scsi_req(req);
req->rq_flags |= RQF_QUIET;
@@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
GFP_KERNEL);
if (err) {
blk_put_request(req);
- return DRIVER_ERROR << 24;
+ return err;
}
}
The patch description looks confusing to me. Is it perhaps because the
caller compares the st_scsi_execute() return value with zero and doesn't
use the return value in any other way that it is fine to return an
integer error code instead of a SCSI status?
Thanks,
Bart.
Re: [PATCH 11/24] advansys: kill driver_defined status byte accessors
On 10/21/19 2:53 AM, Hannes Reinecke wrote:
@@ -6021,43 +6015,28 @@ static void adv_isr_callback(ADV_DVC_VAR *adv_dvc_varp,
ADV_SCSI_REQ_Q *scsiqp)
ASC_DBG(2, "SAM_STAT_CHECK_CONDITION\n");
ASC_DBG_PRT_SENSE(2, scp->sense_buffer,
SCSI_SENSE_BUFFERSIZE);
- /*
-* Note: The 'status_byte()' macro used by
-* target drivers defined in scsi.h shifts the
-* status byte returned by host drivers right
-* by 1 bit. This is why target drivers also
-* use right shifted status byte definitions.
-* For instance target drivers use
-* CHECK_CONDITION, defined to 0x1, instead of
-* the SCSI defined check condition value of
-* 0x2. Host drivers are supposed to return
-* the status byte as it is defined by SCSI.
-*/
- scp->result = DRIVER_BYTE(DRIVER_SENSE) |
- STATUS_BYTE(scsiqp->scsi_status);
- } else {
- scp->result = STATUS_BYTE(scsiqp->scsi_status);
}
+ scp->result = status_byte(scsiqp->scsi_status);
break;
Did you really want to delete the code that sets DRIVER_SENSE?
@@ -6789,47 +6768,30 @@ static void asc_isr_callback(ASC_DVC_VAR *asc_dvc_varp,
ASC_QDONE_INFO *qdonep)
ASC_DBG(2, "SAM_STAT_CHECK_CONDITION\n");
ASC_DBG_PRT_SENSE(2, scp->sense_buffer,
SCSI_SENSE_BUFFERSIZE);
- /*
-* Note: The 'status_byte()' macro used by
-* target drivers defined in scsi.h shifts the
-* status byte returned by host drivers right
-* by 1 bit. This is why target drivers also
-* use right shifted status byte definitions.
-* For instance target drivers use
-* CHECK_CONDITION, defined to 0x1, instead of
-* the SCSI defined check condition value of
-* 0x2. Host drivers are supposed to return
-* the status byte as it is defined by SCSI.
-*/
- scp->result = DRIVER_BYTE(DRIVER_SENSE) |
- STATUS_BYTE(qdonep->d3.scsi_stat);
- } else {
- scp->result = STATUS_BYTE(qdonep->d3.scsi_stat);
}
+ scp->result = status_byte(qdonep->d3.scsi_stat);
break;
Same comment here: did you really want to delete the code that sets
DRIVER_SENSE?
Thanks,
Bart.
Re: [PATCH v5 05/23] sg: bitops in sg_device
On 10/21/19 3:22 PM, Douglas Gilbert wrote: > On 2019-10-18 6:05 a.m., Hannes Reinecke wrote: [ .. ] >> One thing to keep in mind here is that 'set_bit()' is not atomic; it >> needs to be followed by a memory barrier or being replaced by >> test_and_set_bit() if possible. >> Please audit the code if that poses a problem. > > Hi, > set_bit() and friends are documented in Documentation/atomic_bitops.txt > so it would be surprising if they were not _atomic_ . There are variants > documented in that file that _maybe_ non-atomic, they are the one that > start with __ such as __set_bit(). > > So what I believe Documentation/atomic_bitops.txt is trying to say (in > a sloppy kind of way) is that set_bit() and clear_bit() have _relaxed_ > memory order. C/C++ standards (from 2011 onwards) have 6 memory orders: > - relaxed [no memory order guarantees] > - consume [C++17 says "please don't use"!] > - acquire > - release > - acquire-release > - sequentially consistent ["cst"] > > That list is from weakest to strongest. C/C++ default everything they > can to "cst" as it requires the least thinking and is therefore the > safest, but has the highest runtime overhead (depending somewhat on > the platform). > > Linux adds a few wrinkles to the above as C/C++ are only considering > threads while Linux additionally has ISRs, DMA, memory-mapped IO and > the like to consider. > > From what I have read in the Documentation directories, most > descriptions are written by gifted and un-gifted amateurs, apart from > one amazing exception: Documentation/memory-barriers.txt . Now they > are professions and the authors put their names, in full, at the > top which IMO is always a good sign. > > > Back to the review at hand, if set_bit() has relaxed memory order > and needs to be seen by another thread (or ISR/bottom half) then it > is relying on a following atomic operation that has stronger memory > ordering guarantees (than relaxed). In my code I tend to use > __set_bit() and __clear_bit() when a file descriptor or request object > is being created, as no other thread should know of its existence at > that time. > > Doug Gilbert > > Reference: C++ Concurrency in action, Second edition, Anthony Williams > [As the C11 and C++11 concurrency subcommittees were "manned" by the > same folks, they tried to make their memory models as compatible as > possible.] > > That's all I wanted to know. Thanks for the confirmation. Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 05/23] sg: bitops in sg_device
On 2019-10-18 6:05 a.m., Hannes Reinecke wrote:
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
Introduce bitops in sg_device to replace an atomic, a bool and a
char. That char (sgdebug) had been reduced to only two states.
Add some associated macros to make the code a little clearer.
Signed-off-by: Douglas Gilbert
---
drivers/scsi/sg.c | 104 +++---
1 file changed, 53 insertions(+), 51 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 9aa1b1030033..97ce84f0c51b 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -74,6 +74,11 @@ static char *sg_version_date = "20190606";
#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+/* Bit positions (flags) for sg_device::fdev_bm bitmask follow */
+#define SG_FDEV_EXCLUDE0 /* have fd open with O_EXCL */
+#define SG_FDEV_DETACHING 1 /* may be unexpected device removal */
+#define SG_FDEV_LOG_SENSE 2 /* set by ioctl(SG_SET_DEBUG) */
+
int sg_big_buff = SG_DEF_RESERVED_SIZE;
/* N.B. This variable is readable and writeable via
/proc/scsi/sg/def_reserved_size . Each time sg_open() is called a buffer
@@ -155,14 +160,12 @@ struct sg_device { /* holds the state of each scsi
generic device */
struct scsi_device *device;
wait_queue_head_t open_wait;/* queue open() when O_EXCL present */
struct mutex open_rel_lock; /* held when in open() or release() */
- int sg_tablesize; /* adapter's max scatter-gather table size */
- u32 index; /* device index number */
struct list_head sfds;
rwlock_t sfd_lock; /* protect access to sfd list */
- atomic_t detaching; /* 0->device usable, 1->device detaching */
- bool exclude; /* 1->open(O_EXCL) succeeded and is active */
+ int sg_tablesize; /* adapter's max scatter-gather table size */
+ u32 index; /* device index number */
int open_cnt; /* count of opens (perhaps < num(sfds) ) */
- char sgdebug; /* 0->off, 1->sense, 9->dump dev, 10-> all devs
*/
+ unsigned long fdev_bm[1]; /* see SG_FDEV_* defines above */
struct gendisk *disk;
struct cdev * cdev; /* char_dev [sysfs: /sys/cdev/major/sg] */
struct kref d_ref;
@@ -200,6 +203,9 @@ static void sg_device_destroy(struct kref *kref);
#define SZ_SG_IO_HDR ((int)sizeof(struct sg_io_hdr)) /* v3 header */
#define SZ_SG_REQ_INFO ((int)sizeof(struct sg_req_info))
+#define SG_IS_DETACHING(sdp) test_bit(SG_FDEV_DETACHING, (sdp)->fdev_bm)
+#define SG_HAVE_EXCLUDE(sdp) test_bit(SG_FDEV_EXCLUDE, (sdp)->fdev_bm)
+
/*
* Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
* 'depth' is a number between 1 (most severe) and 7 (most noisy, most
@@ -273,26 +279,26 @@ sg_wait_open_event(struct sg_device *sdp, bool o_excl)
while (sdp->open_cnt > 0) {
mutex_unlock(&sdp->open_rel_lock);
retval = wait_event_interruptible(sdp->open_wait,
- (atomic_read(&sdp->detaching) ||
+ (SG_IS_DETACHING(sdp) ||
!sdp->open_cnt));
mutex_lock(&sdp->open_rel_lock);
if (retval) /* -ERESTARTSYS */
return retval;
- if (atomic_read(&sdp->detaching))
+ if (SG_IS_DETACHING(sdp))
return -ENODEV;
}
} else {
- while (sdp->exclude) {
+ while (SG_HAVE_EXCLUDE(sdp)) {
mutex_unlock(&sdp->open_rel_lock);
retval = wait_event_interruptible(sdp->open_wait,
- (atomic_read(&sdp->detaching) ||
-!sdp->exclude));
+ (SG_IS_DETACHING(sdp) ||
+!SG_HAVE_EXCLUDE(sdp)));
mutex_lock(&sdp->open_rel_lock);
if (retval) /* -ERESTARTSYS */
return retval;
- if (atomic_read(&sdp->detaching))
+ if (SG_IS_DETACHING(sdp))
return -ENODEV;
}
}
@@ -354,7 +360,7 @@ sg_open(struct inode *inode, struct file *filp)
goto error_mutex_locked;
}
} else {
- if (sdp->exclude) {
+ if (SG_HAVE_EXCLUDE(sdp)) {
retval = -EBUSY;
goto error_mutex_locked;
}
@@ -367,10 +373,10 @@ sg_open(struct inode *inode, struct file *filp)
/* N.B. at this point we are holding
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 10/21/19 3:49 AM, zhengbin (A) wrote: > > On 2019/10/18 21:43, Martin K. Petersen wrote: >> Hannes, >> >>> The one thing which I patently don't like is the ambivalence between >>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >>> of them is set? IE what would be the correct way of action if >>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >>> way around? >> I agree, it's a mess. >> >> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >> and most arcane code in SCSI) >> >>> But more important, from a quick glance not all drivers set the >>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >>> never evaluated after this patchset. >> And yet we appear to have several code paths where sense evaluation is >> contingent on DRIVER_SENSE. So no matter what, behavior might >> change if we enforce consistent semantics. *sigh* > > So what should we do to prevent unit-value access of sshdr? > Where do you see it? >From my reading, __scsi_execute() is clearing sshdr by way of __scsi_execute() -> scsi_normalize_sense() -> memset(sshdr) Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop
Hi zhengbin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191018]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/zhengbin/scsi-core-fix-uninit-value-access-of-variable-sshdr/20191021-160007
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
In file included from drivers/scsi/device_handler/scsi_dh_hp_sw.c:13:0:
drivers/scsi/device_handler/scsi_dh_hp_sw.c: In function 'hp_sw_start_stop':
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: error: 'result'
>> undeclared (first use in this function); did you mean 'res'?
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~
drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: note: each undeclared
identifier is reported only once for each function it appears in
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~
vim +132 drivers/scsi/device_handler/scsi_dh_hp_sw.c
> 13 #include
14 #include
15 #include
16 #include
17
18 #define HP_SW_NAME "hp_sw"
19
20 #define HP_SW_TIMEOUT (60 * HZ)
21 #define HP_SW_RETRIES 3
22
23 #define HP_SW_PATH_UNINITIALIZED-1
24 #define HP_SW_PATH_ACTIVE 0
25 #define HP_SW_PATH_PASSIVE 1
26
27 struct hp_sw_dh_data {
28 int path_state;
29 int retries;
30 int retry_cnt;
31 struct scsi_device *sdev;
32 };
33
34 static int hp_sw_start_stop(struct hp_sw_dh_data *);
35
36 /*
37 * tur_done - Handle TEST UNIT READY return status
38 * @sdev: sdev the command has been sent to
39 * @errors: blk error code
40 *
41 * Returns SCSI_DH_DEV_OFFLINED if the sdev is on the passive path
42 */
43 static int tur_done(struct scsi_device *sdev, struct hp_sw_dh_data *h,
44 struct scsi_sense_hdr *sshdr)
45 {
46 int ret = SCSI_DH_IO;
47
48 switch (sshdr->sense_key) {
49 case UNIT_ATTENTION:
50 ret = SCSI_DH_IMM_RETRY;
51 break;
52 case NOT_READY:
53 if (sshdr->asc == 0x04 && sshdr->ascq == 2) {
54 /*
55 * LUN not ready - Initialization command
required
56 *
57 * This is the passive path
58 */
59 h->path_state = HP_SW_PATH_PASSIVE;
60 ret = SCSI_DH_OK;
61 break;
62 }
63 /* Fallthrough */
64 default:
65 sdev_printk(KERN_WARNING, sdev,
66 "%s: sending tur failed, sense %x/%x/%x\n",
67 HP_SW_NAME, sshdr->sense_key, sshdr->asc,
68 sshdr->ascq);
69 break;
70 }
71 return ret;
72 }
73
74 /*
75 * hp_sw_tur - Send TEST UNIT READY
76 * @sdev: sdev command should be sent to
77 *
78 * Use the TEST UNIT READY command to determine
79 * the path state.
80 */
81 static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
82 {
83 unsigned char cmd[6] = { TEST_UNIT_READY };
84 struct scsi_sense_hdr sshdr;
85 int ret = SCSI_DH_OK, res;
86 u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
87 REQ_FAILFAST_DRIVER;
88
89 retry:
90 res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
91 HP_SW_TIMEOU
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 18/10/2019 15:43, Martin K. Petersen wrote: [...] > Johannes: Whatever happened to your efforts at cleaning all this up? Do > you have a patch series or a working tree we could use as starting > point? I'll have to look. I think it's still floating around in some git tree. Let me search for it. Johannes -- Johannes ThumshirnSUSE Labs Filesystems jthumsh...@suse.de+49 911 74053 689 SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] scsi: sd: fix uninit access of sshdr
On 2019-10-18 03:01, zhengbin wrote:
> @@ -1648,16 +1651,20 @@ static int sd_sync_cache(struct scsi_disk *sdkp,
> struct scsi_sense_hdr *sshdr)
> if (res) {
> sd_print_result(sdkp, "Synchronize Cache(10) failed", res);
>
> - if (driver_byte(res) == DRIVER_SENSE)
> + /* we need to evaluate the error return */
> + if (driver_byte(res) == DRIVER_SENSE &&
> + scsi_sense_valid(sshdr)) {
> sd_print_sense_hdr(sdkp, sshdr);
>
Does Hannes' comment about DRIVER_SENSE also apply to this patch?
Thanks,
Bart.
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/18 21:43, Martin K. Petersen wrote: > Hannes, > >> The one thing which I patently don't like is the ambivalence between >> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >> of them is set? IE what would be the correct way of action if >> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >> way around? > I agree, it's a mess. > > (Sorry, zhengbin, you opened a can of worms. This is some of our oldest > and most arcane code in SCSI) > >> But more important, from a quick glance not all drivers set the >> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >> never evaluated after this patchset. > And yet we appear to have several code paths where sense evaluation is > contingent on DRIVER_SENSE. So no matter what, behavior might > change if we enforce consistent semantics. *sigh* So what should we do to prevent unit-value access of sshdr? 1. still init sshdr in __scsi_execute? @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, struct scsi_request *rq; int ret = DRIVER_ERROR << 24; + /* +* Zero-initialize sshdr for those callers that check the *sshdr +* contents even if no sense data is available. +*/ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + req = blk_get_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); 2. init sshdr in callers of scsi_execute, instead of check the result of scsi_execute and check whether sshdr is valid? for example: @@ -506,6 +506,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer, put_unaligned_be32(len, &cmd[6]); memset(buffer, 0, len); + memset(&sshdr, 0 ,sizeof(sshdr)); result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len, &sshdr, 30 * HZ, 3, NULL) Besides, should sd_pr_command init sshdr?? cause sd_pr_command have checked the result of scsi_execute and check whether sshdr is valid. 3. get rid of DRIVER_* completely? even this, we should init sshdr first. sshdr is just 8 bytes, init it does not affect performance My advice is 2. > >> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on >> scsi_sense_valid() only. > I would really like to get rid of DRIVER_* completely. Except for > DRIVER_SENSE, few are actually in use: > > DRIVER_OK:0 > DRIVER_BUSY: 0 > DRIVER_SOFT: 0 > DRIVER_MEDIA: 0 > DRIVER_ERROR: 6 > DRIVER_INVALID: 4 > DRIVER_TIMEOUT: 1 > DRIVER_SENSE: 58 > > Johannes: Whatever happened to your efforts at cleaning all this up? Do > you have a patch series or a working tree we could use as starting > point? >
Re: [RFC PATCH 0/3] lpfc: nodelist pointer cleanup
On 10/18/19 11:45 PM, James Smart wrote: On 10/18/2019 12:50 AM, Hannes Reinecke wrote: Hi James, trying to figure this annoying lpfc_set_rrq_active() bug I've found the nodelist pointer handling in the lpfc io buffers a bit strange; there's a 'ndlp' pointer, but for scsi the nodelist is primarily accessed via the 'rdata' pointer (although not everywhere). For NVMe it's primarily the 'ndlp' pointer, apparently, but the usage is quite confusing. So here's a patchset to straighten things up; it primarily moves the anonymous protocol-specific structure in the io buffer to a named one, and always accesses the nodelist through the protocol-specific rport data structure. It also has the nice side-effect that the protocol-specific areas are aligned now, so clearing the 'rdata' pointer on the scsi side will be equivalent to clearing the 'rport' pointer on the nvme side. And it reduces the size of the io buffer. Let me know what you think. Hannes Reinecke (3): lpfc: use named structure for combined I/O buffer lpfc: access nodelist through scsi-specific rdata pointer lpfc: access nvme nodelist through nvme rport structure drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 56 ++-- drivers/scsi/lpfc/lpfc_scsi.c | 196 +- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 6 +- 5 files changed, 143 insertions(+), 143 deletions(-) Well, the problem I think you are trying to solve is ultimately the root issue that is solved by this patch in the just-posted 12.6.0.0 patch set: [PATCH 05/16] lpfc: Fix bad ndlp ptr in xri aborted handling As such, I'd like to see if the 12.6.0.0 patch resolves the issue before going through all the shifting in your patches. Note: the failing routine can change as it's totally dependent on where the bogus pointer value takes you. The key is the lpfc_sli4_sp_handle_abort_xri_wcqe() routine on the stack. Fair enough. I'll be giving your patchset a spin once submitted. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)
Re: [PATCH] scsi_dh_alua: Do not run STPG for implicit ALUA
On 10/18/19 11:44 PM, Martin K. Petersen wrote:
Hannes,
If a target only supports implicit ALUA sending a SET TARGET PORT
GROUPS command is not only pointless, but might actually cause issues.
We already have a conditional in alua_stpg():
if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
/* Only implicit ALUA supported, retry */
return SCSI_DH_RETRY;
}
@@ -832,6 +832,10 @@ static void alua_rtpg_work(struct work_struct *work)
if (err != SCSI_DH_OK)
pg->flags &= ~ALUA_PG_RUN_STPG;
}
+ /* Do not run STPG if only implicit ALUA is supported */
+ if (scsi_device_tpgs(sdev) == TPGS_MODE_IMPLICIT)
+ pg->flags &= ~ALUA_PG_RUN_STPG;
+
if (pg->flags & ALUA_PG_RUN_STPG) {
pg->flags &= ~ALUA_PG_RUN_STPG;
spin_unlock_irqrestore(&pg->lock, flags);
Instead of checking for EXPLICIT one place and checking for !IMPLICIT
another, can we consolidate the two and maybe do:
if (pg->flags & ALUA_PG_RUN_STPG &&
scsi_device_tpgs(sdev) == TPGS_MODE_EXPLICIT) {
[...]
and then remove the redundant check in alua_stpg()?
Good point.
Will be resending the patch.
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Re: [PATCH 0/7] scsi: aacraid updates
Balsundar, > Balsundar P (7): > scsi: aacraid: Fix illegal data read or write beyond last LBA > scsi: aacraid: Fixed failure to check IO response and report IO error > scsi: aacraid: Fixed basecode assert when IOP reset is sent by driver > scsi: aacraid: setting different timeout for src and thor > scsi: aacraid: check adapter health before processing IOCTL > scsi: aacraid: Issued AIF request command after IOP RESET > scsi: aacraid: Update driver version to 50983 Applied to 5.5/scsi-queue. Zeroday had some additional warnings that were not introduced by this series. Please review and address those. Thanks! -- Martin K. Petersen Oracle Linux Engineering
[RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On Fri, 18 Oct 2019, Martin K. Petersen wrote:
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is
set clobbers the most significant bytes in cmd->result.
Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.
This is a theoretical bug, found by inspection, so I'm sending an untested
RFC patch.
I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@ static inline struct scsi_data_buffer *scsi_prot(struct
scsi_cmnd *cmd)
#define scsi_for_each_prot_sg(cmd, sg, nseg, __i) \
for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0x00ff) | (status << 8);
}
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0xff00) | (status << 16);
}
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0x00ff) | (status << 24);
}
Re: [PATCH] scsi: ufs-bsg: Wake the device before sending raw upiu commands
Avri, > The scsi async probe process is calling blk_pm_runtime_init for each > lun, and then those request queues are monitored by the block layer pm > engine (blk-pm.c). This is however, not the case for scsi-passthrough > queues, created by bsg_setup_queue(). > > So the ufs-bsg driver might send various commands, disregarding the pm > status of the device. This is wrong, regardless if its request queue > is pm-aware or not. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi_dh_alua: Do not run STPG for implicit ALUA
Hannes,
> If a target only supports implicit ALUA sending a SET TARGET PORT
> GROUPS command is not only pointless, but might actually cause issues.
We already have a conditional in alua_stpg():
if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
/* Only implicit ALUA supported, retry */
return SCSI_DH_RETRY;
}
> @@ -832,6 +832,10 @@ static void alua_rtpg_work(struct work_struct *work)
> if (err != SCSI_DH_OK)
> pg->flags &= ~ALUA_PG_RUN_STPG;
> }
> + /* Do not run STPG if only implicit ALUA is supported */
> + if (scsi_device_tpgs(sdev) == TPGS_MODE_IMPLICIT)
> + pg->flags &= ~ALUA_PG_RUN_STPG;
> +
> if (pg->flags & ALUA_PG_RUN_STPG) {
> pg->flags &= ~ALUA_PG_RUN_STPG;
> spin_unlock_irqrestore(&pg->lock, flags);
Instead of checking for EXPLICIT one place and checking for !IMPLICIT
another, can we consolidate the two and maybe do:
if (pg->flags & ALUA_PG_RUN_STPG &&
scsi_device_tpgs(sdev) == TPGS_MODE_EXPLICIT) {
[...]
and then remove the redundant check in alua_stpg()?
--
Martin K. Petersen Oracle Linux Engineering
Re: [RFC PATCH 0/3] lpfc: nodelist pointer cleanup
On 10/18/2019 12:50 AM, Hannes Reinecke wrote: Hi James, trying to figure this annoying lpfc_set_rrq_active() bug I've found the nodelist pointer handling in the lpfc io buffers a bit strange; there's a 'ndlp' pointer, but for scsi the nodelist is primarily accessed via the 'rdata' pointer (although not everywhere). For NVMe it's primarily the 'ndlp' pointer, apparently, but the usage is quite confusing. So here's a patchset to straighten things up; it primarily moves the anonymous protocol-specific structure in the io buffer to a named one, and always accesses the nodelist through the protocol-specific rport data structure. It also has the nice side-effect that the protocol-specific areas are aligned now, so clearing the 'rdata' pointer on the scsi side will be equivalent to clearing the 'rport' pointer on the nvme side. And it reduces the size of the io buffer. Let me know what you think. Hannes Reinecke (3): lpfc: use named structure for combined I/O buffer lpfc: access nodelist through scsi-specific rdata pointer lpfc: access nvme nodelist through nvme rport structure drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 56 ++-- drivers/scsi/lpfc/lpfc_scsi.c | 196 +- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 6 +- 5 files changed, 143 insertions(+), 143 deletions(-) Well, the problem I think you are trying to solve is ultimately the root issue that is solved by this patch in the just-posted 12.6.0.0 patch set: [PATCH 05/16] lpfc: Fix bad ndlp ptr in xri aborted handling As such, I'd like to see if the 12.6.0.0 patch resolves the issue before going through all the shifting in your patches. Note: the failing routine can change as it's totally dependent on where the bogus pointer value takes you. The key is the lpfc_sli4_sp_handle_abort_xri_wcqe() routine on the stack. -- james
Re: [PATCH] qla2xxx: fixup incorrect usage of host_byte
Hannes, > DRIVER_ERROR is a a driver byte setting, not a host byte. The qla2xxx > driver should rather return DID_ERROR here to be in line with the > other drivers. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] lpfc: remove left-over BUILD_NVME defines
James, > I assume that 5.4/scsi-fixes will get merged into 5.4 pre-release, Yes. > and that the stable tree will rebase to pick it up ? stable/master is tracking Linus until final release. If you want the stats issue fixed in 5.3, it's best to wait for Hannes' commit to be merged by Linus. You can then request a stable backport. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] lpfc: remove left-over BUILD_NVME defines
On 10/17/2019 7:01 PM, Martin K. Petersen wrote: Hannes, The BUILD_NVME define never got defined anywhere, causing NVMe commands to be treated as SCSI commands when freeing the buffers. This was causing a stuck discovery and a horrible crash in lpfc_set_rrq_active() later on. Applied to 5.4/scsi-fixes, thanks! The offending patches that introduced the define are: From 12.2.0.0: scsi: lpfc: Move SCSI and NVME Stats to hardware queue structures commit 4c47efc140fa926f00aa59c248458d95bd7b5eab From 12.4.0.0: scsi: lpfc: Merge per-protocol WQ/CQ pairs into single per-cpu pair commit c00f62e6c5468ed0673c583f1ff284274e817410 The 12.2 patch just misses some stats - no big deal. But the 12.4 patch introduces a logic error, and is in the head of the stable tree. I assume that 5.4/scsi-fixes will get merged into 5.4 pre-release, and that the stable tree will rebase to pick it up ? -- james
Re: [PATCH] scsi: fixup scsi_device_from_queue()
Hannes, this is already in
https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=5.4/scsi-postmerge
from
https://lore.kernel.org/linux-block/20190807144948.28265-1-ma...@linux.ibm.com/T/
and Martin just sent a pull request to Linus
https://lore.kernel.org/linux-scsi/yq1pniui429@oracle.com/T/#m91f5b9098c369dc0d9bfef84aa53ee35533a31be
On 10/18/19 4:03 PM, Hannes Reinecke wrote:
After commit 8930a6c20791 ("scsi: core: add support for request batching")
scsi_device_from_queue() will not work for devices implementing the
new scsi_mq_ops_no_commit template.
Hence multipath is not able to detect the underlying scsi devices
and multipath startup will fail.
Fixes: 8930a6c20791 ("scsi: core: add support for request batching")
Signed-off-by: Hannes Reinecke
---
drivers/scsi/scsi_lib.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c1c2998297b2..cd3e21a0098c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1924,7 +1924,8 @@ struct scsi_device *scsi_device_from_queue(struct
request_queue *q)
{
struct scsi_device *sdev = NULL;
- if (q->mq_ops == &scsi_mq_ops)
+ if (q->mq_ops == &scsi_mq_ops ||
+ q->mq_ops == &scsi_mq_ops_no_commit)
sdev = q->queuedata;
if (!sdev || !get_device(&sdev->sdev_gendev))
sdev = NULL;
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 10/18/19 3:43 PM, Martin K. Petersen wrote: > > Hannes, > >> The one thing which I patently don't like is the ambivalence between >> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >> of them is set? IE what would be the correct way of action if >> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >> way around? > > I agree, it's a mess. > > (Sorry, zhengbin, you opened a can of worms. This is some of our oldest > and most arcane code in SCSI) > >> But more important, from a quick glance not all drivers set the >> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >> never evaluated after this patchset. > > And yet we appear to have several code paths where sense evaluation is > contingent on DRIVER_SENSE. So no matter what, behavior might > change if we enforce consistent semantics. *sigh* > >> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on >> scsi_sense_valid() only. > > I would really like to get rid of DRIVER_* completely. Except for > DRIVER_SENSE, few are actually in use: > > DRIVER_OK:0 > DRIVER_BUSY: 0 > DRIVER_SOFT: 0 > DRIVER_MEDIA: 0 > DRIVER_ERROR: 6 Three less now :-) Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
Hannes, > The one thing which I patently don't like is the ambivalence between > DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ > of them is set? IE what would be the correct way of action if > DRIVER_SENSE is not set, but we have a valid sense code? Or the other > way around? I agree, it's a mess. (Sorry, zhengbin, you opened a can of worms. This is some of our oldest and most arcane code in SCSI) > But more important, from a quick glance not all drivers set the > DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is > never evaluated after this patchset. And yet we appear to have several code paths where sense evaluation is contingent on DRIVER_SENSE. So no matter what, behavior might change if we enforce consistent semantics. *sigh* > I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on > scsi_sense_valid() only. I would really like to get rid of DRIVER_* completely. Except for DRIVER_SENSE, few are actually in use: DRIVER_OK: 0 DRIVER_BUSY:0 DRIVER_SOFT:0 DRIVER_MEDIA: 0 DRIVER_ERROR: 6 DRIVER_INVALID: 4 DRIVER_TIMEOUT: 1 DRIVER_SENSE: 58 Johannes: Whatever happened to your efforts at cleaning all this up? Do you have a patch series or a working tree we could use as starting point? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/18 17:17, zhengbin wrote:
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
> v4->v5: fix uninit-value access bug in callers, not in __scsi_execute
An explanation text of the series would be great...
What about defining a little helper function for checking
driver_byte(err) == DRIVER_SENSE && scsi_sense_valid(&sshdr)
instead of having that hard-coded everywhere ? That would make the code
a lot cleaner and more readable.
Something like:
static inline bool scsi_driver_sense_valid(int result,
struct scsi_sense_hdr *sshdr)
{
return driver_byte(result) == DRIVER_SENSE &&
scsi_sense_valid(sshdr);
}
>
> zhengbin (13):
> scsi: core: need to check the result of scsi_execute in
> scsi_report_opcode
> scsi: core: need to check the result of scsi_execute in
> scsi_test_unit_ready
> scsi: core: need to check the result of scsi_execute in
> scsi_report_lun_scan
> scsi: sr: need to check the result of scsi_execute in sr_get_events
> scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
> scsi: scsi_dh_emc: need to check the result of scsi_execute in
> send_trespass_cmd
> scsi: scsi_dh_rdac: need to check the result of scsi_execute in
> send_mode_select
> scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
> hp_sw_tur,hp_sw_start_stop
> scsi: scsi_dh_alua: need to check the result of scsi_execute in
> alua_rtpg,alua_stpg
> scsi: scsi_transport_spi: need to check whether sshdr is valid in
> spi_execute
> scsi: cxlflash: need to check whether sshdr is valid in read_cap16
> scsi: ufs: need to check whether sshdr is valid in
> ufshcd_set_dev_pwr_mode
> scsi: ch: need to check whether sshdr is valid in ch_do_scsi
>
> drivers/scsi/ch.c | 6 --
> drivers/scsi/cxlflash/superpipe.c | 2 +-
> drivers/scsi/device_handler/scsi_dh_alua.c | 6 --
> drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
> drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 --
> drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +---
> drivers/scsi/scsi.c | 2 +-
> drivers/scsi/scsi_lib.c | 3 +++
> drivers/scsi/scsi_scan.c| 3 ++-
> drivers/scsi/scsi_transport_spi.c | 1 +
> drivers/scsi/sr.c | 3 ++-
> drivers/scsi/sr_ioctl.c | 6 ++
> drivers/scsi/ufs/ufshcd.c | 3 ++-
> 13 files changed, 37 insertions(+), 15 deletions(-)
>
> --
> 2.7.4
>
>
--
Damien Le Moal
Western Digital Research
Re: [PATCH v5 22/23] sg: sg_fill_request_element
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > Replace sg_fill_request_table() with sg_fill_request_element(). > Reduce the size of the sg_rq_end_io() function by breaking out > some sense buffer checks into sg_check_sense(). Reduce the > size of the sg_start_req() function with sg_set_map_data() > helper. All code refactoring, no logical change. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 211 ++ > 1 file changed, 118 insertions(+), 93 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 21/23] sg: sg_find_srp_by_id
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
> Replace sg_get_rq_mark() with sg_find_srp_by_id() and
> sg_get_ready_srp(). Add sg_chk_mmap() to check flags and
> reserve buffer available for mmap() based requests. Add
> sg_copy_sense() and sg_rec_v3_state() which is just
> refactoring. Add sg_calc_rq_dur() and sg_get_dur() in
> preparation for optional nanosecond duration timing.
>
> Signed-off-by: Douglas Gilbert
> ---
> drivers/scsi/sg.c | 293 +++---
> 1 file changed, 200 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 4e6f6fb2a54e..7f3a4b937a5a 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -158,16 +158,20 @@ struct sg_fd;
>
> struct sg_request { /* SG_MAX_QUEUE requests outstanding per file */
> struct list_head entry; /* list entry */
> - struct sg_fd *parentfp; /* NULL -> not in use */
> struct sg_scatter_hold data;/* hold buffer, perhaps scatter list */
> struct sg_io_hdr header; /* scsi command+info, see */
> u8 sense_b[SCSI_SENSE_BUFFERSIZE];
> + u32 duration; /* cmd duration in milliseconds */
> char res_used; /* 1 -> using reserve buffer, 0 -> not ... */
> char orphan;/* 1 -> drop on sight, 0 -> normal */
> char sg_io_owned; /* 1 -> packet belongs to SG_IO */
> /* done protected by rq_list_lock */
> char done; /* 0->before bh, 1->before read, 2->read */
> atomic_t rq_st; /* request state, holds a enum sg_rq_state */
> + u8 cmd_opcode; /* first byte of SCSI cdb */
> + u64 start_ns; /* starting point of command duration calc */
> + unsigned long frq_bm[1];/* see SG_FRQ_* defines above */
> + struct sg_fd *parentfp; /* pointer to owning fd, even when on fl */
> struct request *rq; /* released in sg_rq_end_io(), bio kept */
> struct bio *bio;/* kept until this req -->SG_RS_INACTIVE */
> struct execute_work ew_orph;/* harvest orphan request */
cmd_opcode is used where?
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 20/23] sg: introduce request state machine
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
> The introduced request state machine is not wired in so that
> the size of one of the following patches is reduced. Bit
> operation defines for the request and file descriptor level
> are also introduced. Minor rework og sg_rd_append() function.
>
> Signed-off-by: Douglas Gilbert
> ---
> drivers/scsi/sg.c | 237 ++
> 1 file changed, 175 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 5b560f720993..4e6f6fb2a54e 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -75,7 +75,43 @@ static char *sg_version_date = "20190606";
> #define uptr64(val) ((void __user *)(uintptr_t)(val))
> #define cuptr64(val) ((const void __user *)(uintptr_t)(val))
>
> +/* Following enum contains the states of sg_request::rq_st */
> +enum sg_rq_state {
> + SG_RS_INACTIVE = 0, /* request not in use (e.g. on fl) */
> + SG_RS_INFLIGHT, /* active: cmd/req issued, no response yet */
> + SG_RS_AWAIT_RD, /* response received, awaiting read */
> + SG_RS_DONE_RD, /* read is ongoing or done */
> + SG_RS_BUSY, /* temporary state should rarely be seen */
> +};
> +
> +#define SG_TIME_UNIT_MS 0/* milliseconds */
> +#define SG_DEF_TIME_UNIT SG_TIME_UNIT_MS
> #define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
> +#define SG_FD_Q_AT_HEAD 0
> +#define SG_DEFAULT_Q_AT SG_FD_Q_AT_HEAD /* for backward compatibility */
> +#define SG_FL_MMAP_DIRECT (SG_FLAG_MMAP_IO | SG_FLAG_DIRECT_IO)
> +
> +/* Only take lower 4 bits of driver byte, all host byte and sense byte */
> +#define SG_ML_RESULT_MSK 0x0fff00ff /* mid-level's 32 bit result value */
> +
> +#define SG_PACK_ID_WILDCARD (-1)
> +
> +#define SG_ADD_RQ_MAX_RETRIES 40 /* to stop infinite _trylock(s) */
> +
> +/* Bit positions (flags) for sg_request::frq_bm bitmask follow */
> +#define SG_FRQ_IS_ORPHAN 1 /* owner of request gone */
> +#define SG_FRQ_SYNC_INVOC2 /* synchronous (blocking) invocation */
> +#define SG_FRQ_DIO_IN_USE3 /* false->indirect_IO,mmap; 1->dio */
> +#define SG_FRQ_NO_US_XFER4 /* no user space transfer of data */
> +#define SG_FRQ_DEACT_ORPHAN 7 /* not keeping orphan so de-activate */
> +#define SG_FRQ_BLK_PUT_REQ 9 /* set when blk_put_request() called */
> +
> +/* Bit positions (flags) for sg_fd::ffd_bm bitmask follow */
> +#define SG_FFD_FORCE_PACKID 0 /* receive only given pack_id/tag */
> +#define SG_FFD_CMD_Q 1 /* clear: only 1 active req per fd */
> +#define SG_FFD_KEEP_ORPHAN 2 /* policy for this fd */
> +#define SG_FFD_MMAP_CALLED 3 /* mmap(2) system call made on fd */
> +#define SG_FFD_Q_AT_TAIL 5 /* set: queue reqs at tail of blk q */
>
> /* Bit positions (flags) for sg_device::fdev_bm bitmask follow */
> #define SG_FDEV_EXCLUDE 0 /* have fd open with O_EXCL */
> @@ -83,12 +119,11 @@ static char *sg_version_date = "20190606";
> #define SG_FDEV_LOG_SENSE2 /* set by ioctl(SG_SET_DEBUG) */
>
> int sg_big_buff = SG_DEF_RESERVED_SIZE;
> -/* N.B. This variable is readable and writeable via
> - /proc/scsi/sg/def_reserved_size . Each time sg_open() is called a buffer
> - of this size (or less if there is not enough memory) will be reserved
> - for use by this file descriptor. [Deprecated usage: this variable is also
> - readable via /proc/sys/kernel/sg-big-buff if the sg driver is built into
> - the kernel (i.e. it is not a module).] */
> +/*
> + * This variable is accessible via /proc/scsi/sg/def_reserved_size . Each
> + * time sg_open() is called a sg_request of this size (or less if there is
> + * not enough memory) will be reserved for use by this file descriptor.
> + */
> static int def_reserved_size = -1; /* picks up init parameter */
> static int sg_allow_dio = SG_ALLOW_DIO_DEF;
>
> @@ -132,6 +167,7 @@ struct sg_request { /* SG_MAX_QUEUE requests
> outstanding per file */
> char sg_io_owned; /* 1 -> packet belongs to SG_IO */
> /* done protected by rq_list_lock */
> char done; /* 0->before bh, 1->before read, 2->read */
> + atomic_t rq_st; /* request state, holds a enum sg_rq_state */
> struct request *rq; /* released in sg_rq_end_io(), bio kept */
> struct bio *bio;/* kept until this req -->SG_RS_INACTIVE */
> struct execute_work ew_orph;/* harvest orphan request */
> @@ -208,10 +244,15 @@ static void sg_unlink_reserve(struct sg_fd *sfp, struct
> sg_request *srp);
> static struct sg_fd *sg_add_sfp(struct sg_device *sdp);
> static void sg_remove_sfp(struct kref *);
> static struct sg_request *sg_add_request(struct sg_fd *sfp);
> -static int sg_deact_request(struct sg_fd *sfp, struct sg_request *srp);
> +static void sg_deact_request(struct sg_fd *sfp, struct sg_request *srp);
> static struct
Re: [PATCH v5 19/23] sg: rework scatter gather handling
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > Rename sg_build_indirect() to sg_mk_sgat() and sg_remove_scat() > to sg_remove_sgat(). Re-implement those functions. Add > sg_calc_sgat_param() to calculate various scatter gather > list parameters. Some other minor clean-ups. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 275 +- > 1 file changed, 152 insertions(+), 123 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 18/23] sg: replace sg_allow_access
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > Replace the sg_allow_access() function with sg_fetch_cmnd() > which does a little more. Change sg_finish_scsi_blk_rq() from an > int to a void returning function. Rename sg_remove_request() > to sg_deact_request(). Other changes, mainly cosmetic. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 149 +- > 1 file changed, 80 insertions(+), 69 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 17/23] sg: rework sg_mmap
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
> Simple rework of the sg_mmap() function.
>
> Signed-off-by: Douglas Gilbert
> ---
> drivers/scsi/sg.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index befcbfbcece1..2ad86aaaf74d 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1449,14 +1449,15 @@ static const struct vm_operations_struct
> sg_mmap_vm_ops = {
> .fault = sg_vma_fault,
> };
>
> +/* Entry point for mmap(2) system call */
> static int
> sg_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> - struct sg_fd *sfp;
> - unsigned long req_sz, len, sa;
> - struct sg_scatter_hold *rsv_schp;
> int k, length;
> int ret = 0;
> + unsigned long req_sz, len, sa;
> + struct sg_scatter_hold *rsv_schp;
> + struct sg_fd *sfp;
>
> if (!filp || !vma)
> return -ENXIO;
> @@ -1469,19 +1470,23 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
> SG_LOG(3, sfp, "%s: vm_start=%p, len=%d\n", __func__,
> (void *)vma->vm_start, (int)req_sz);
> if (vma->vm_pgoff)
> - return -EINVAL; /* want no offset */
> - rsv_schp = &sfp->reserve;
> + return -EINVAL; /* only an offset of 0 accepted */
> + /* Check reserve request is inactive and has large enough buffer */
> mutex_lock(&sfp->f_mutex);
> - if (req_sz > rsv_schp->buflen) {
> - ret = -ENOMEM; /* cannot map more than reserved buffer */
> + if (sfp->res_in_use) {
> + ret = -EBUSY;
> + goto out;
> + }
> + rsv_schp = &sfp->reserve;
> + if (req_sz > (unsigned long)rsv_schp->buflen) {
> + ret = -ENOMEM;
> goto out;
> }
> -
> sa = vma->vm_start;
> length = 1 << (PAGE_SHIFT + rsv_schp->page_order);
> - for (k = 0; k < rsv_schp->num_sgat && sa < vma->vm_end; k++) {
> + for (k = 0; k < rsv_schp->num_sgat && sa < vma->vm_end; ++k) {
> len = vma->vm_end - sa;
> - len = (len < length) ? len : length;
> + len = min_t(unsigned long, len, (unsigned long)length);
> sa += len;
> }
>
>
Some comment regarding kernel-doc applies here, too.
Anc the change in the 'for' condition above appears to be rather cosmetic...
Otherwise:
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 16/23] sg: rework sg_vma_fault
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
> Simple refactoring of the sg_vma_fault() function.
>
> Signed-off-by: Douglas Gilbert
> ---
> drivers/scsi/sg.c | 33 +++--
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 903faafaeff9..befcbfbcece1 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1389,14 +1389,16 @@ sg_fasync(int fd, struct file *filp, int mode)
> return fasync_helper(fd, filp, mode, &sfp->async_qp);
> }
>
> +/* Note: the error return: VM_FAULT_SIGBUS causes a "bus error" */
> static vm_fault_t
> sg_vma_fault(struct vm_fault *vmf)
> {
> - struct vm_area_struct *vma = vmf->vma;
> - struct sg_fd *sfp;
> + int k, length;
> unsigned long offset, len, sa;
> + struct vm_area_struct *vma = vmf->vma;
> struct sg_scatter_hold *rsv_schp;
> - int k, length;
> + struct sg_device *sdp;
> + struct sg_fd *sfp;
> const char *nbp = "==NULL, bad";
>
> if (!vma) {
Of course, one would prefer normal kernel-doc style for the comment ...
Otherwise:
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 15/23] sg: sg_common_write add structure for arguments
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > As the number of arguments to sg_common_write() starts to grow > (more in later patches) add a structure to hold most of these > arguments. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 47 --- > 1 file changed, 32 insertions(+), 15 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 14/23] sg: split sg_read
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > As sg_read() is getting quite long, split out the v1 and v2 > processing into sg_rd_v1v2(). Rename sg_new_read() to > sg_v3_receive() as the v3 interface is now older than the v4 > interface which is being added in a later patch. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 273 +++--- > 1 file changed, 135 insertions(+), 138 deletions(-) > Please use consistent naming. Either 'sg_v3_receive', 'sg_v1v2_read' etc, or 'sg_receive_v3', 'sg_read_v1v2' etc. And decide whether to use abbreviated or full names. IE either sg_read_v1v2 or sg_rd_v1v2, but if the latter than I would also prefer to have 'sg_rcv_v3'. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 13/23] sg: ioctl handling
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > Shorten sg_ioctl() by adding some helper functions. sg_ioctl() > is the main entry point for ioctls used on this driver's > devices. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 325 -- > 1 file changed, 200 insertions(+), 125 deletions(-) > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 2796fef42837..90753f7759c7 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -72,6 +72,9 @@ static char *sg_version_date = "20190606"; > */ > #define SG_MAX_CDB_SIZE 252 > > +#define uptr64(val) ((void __user *)(uintptr_t)(val)) > +#define cuptr64(val) ((const void __user *)(uintptr_t)(val)) > + > #define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ) > > /* Bit positions (flags) for sg_device::fdev_bm bitmask follow */ These defines are used only once; I'd rather drop them and do the conversion in-place. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 12/23] sg: change rwlock to spinlock
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > A reviewer suggested that the extra overhead associated with a > rw lock compared to a spinlock was not worth it for short, > oft-used critcal sections. > > So the rwlock on the request list/array is changed to a spinlock. > The head of that list is in the owning sf file descriptor object. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 52 +++ > 1 file changed, 26 insertions(+), 26 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 05/23] sg: bitops in sg_device
On 10/8/19 9:50 AM, Douglas Gilbert wrote:
> Introduce bitops in sg_device to replace an atomic, a bool and a
> char. That char (sgdebug) had been reduced to only two states.
> Add some associated macros to make the code a little clearer.
>
> Signed-off-by: Douglas Gilbert
> ---
> drivers/scsi/sg.c | 104 +++---
> 1 file changed, 53 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 9aa1b1030033..97ce84f0c51b 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -74,6 +74,11 @@ static char *sg_version_date = "20190606";
>
> #define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
>
> +/* Bit positions (flags) for sg_device::fdev_bm bitmask follow */
> +#define SG_FDEV_EXCLUDE 0 /* have fd open with O_EXCL */
> +#define SG_FDEV_DETACHING1 /* may be unexpected device removal */
> +#define SG_FDEV_LOG_SENSE2 /* set by ioctl(SG_SET_DEBUG) */
> +
> int sg_big_buff = SG_DEF_RESERVED_SIZE;
> /* N.B. This variable is readable and writeable via
> /proc/scsi/sg/def_reserved_size . Each time sg_open() is called a buffer
> @@ -155,14 +160,12 @@ struct sg_device { /* holds the state of each scsi
> generic device */
> struct scsi_device *device;
> wait_queue_head_t open_wait;/* queue open() when O_EXCL present */
> struct mutex open_rel_lock; /* held when in open() or release() */
> - int sg_tablesize; /* adapter's max scatter-gather table size */
> - u32 index; /* device index number */
> struct list_head sfds;
> rwlock_t sfd_lock; /* protect access to sfd list */
> - atomic_t detaching; /* 0->device usable, 1->device detaching */
> - bool exclude; /* 1->open(O_EXCL) succeeded and is active */
> + int sg_tablesize; /* adapter's max scatter-gather table size */
> + u32 index; /* device index number */
> int open_cnt; /* count of opens (perhaps < num(sfds) ) */
> - char sgdebug; /* 0->off, 1->sense, 9->dump dev, 10-> all devs
> */
> + unsigned long fdev_bm[1]; /* see SG_FDEV_* defines above */
> struct gendisk *disk;
> struct cdev * cdev; /* char_dev [sysfs: /sys/cdev/major/sg] */
> struct kref d_ref;
> @@ -200,6 +203,9 @@ static void sg_device_destroy(struct kref *kref);
> #define SZ_SG_IO_HDR ((int)sizeof(struct sg_io_hdr)) /* v3 header */
> #define SZ_SG_REQ_INFO ((int)sizeof(struct sg_req_info))
>
> +#define SG_IS_DETACHING(sdp) test_bit(SG_FDEV_DETACHING, (sdp)->fdev_bm)
> +#define SG_HAVE_EXCLUDE(sdp) test_bit(SG_FDEV_EXCLUDE, (sdp)->fdev_bm)
> +
> /*
> * Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
> * 'depth' is a number between 1 (most severe) and 7 (most noisy, most
> @@ -273,26 +279,26 @@ sg_wait_open_event(struct sg_device *sdp, bool o_excl)
> while (sdp->open_cnt > 0) {
> mutex_unlock(&sdp->open_rel_lock);
> retval = wait_event_interruptible(sdp->open_wait,
> - (atomic_read(&sdp->detaching) ||
> + (SG_IS_DETACHING(sdp) ||
>!sdp->open_cnt));
> mutex_lock(&sdp->open_rel_lock);
>
> if (retval) /* -ERESTARTSYS */
> return retval;
> - if (atomic_read(&sdp->detaching))
> + if (SG_IS_DETACHING(sdp))
> return -ENODEV;
> }
> } else {
> - while (sdp->exclude) {
> + while (SG_HAVE_EXCLUDE(sdp)) {
> mutex_unlock(&sdp->open_rel_lock);
> retval = wait_event_interruptible(sdp->open_wait,
> - (atomic_read(&sdp->detaching) ||
> - !sdp->exclude));
> + (SG_IS_DETACHING(sdp) ||
> + !SG_HAVE_EXCLUDE(sdp)));
> mutex_lock(&sdp->open_rel_lock);
>
> if (retval) /* -ERESTARTSYS */
> return retval;
> - if (atomic_read(&sdp->detaching))
> + if (SG_IS_DETACHING(sdp))
> return -ENODEV;
> }
> }
> @@ -354,7 +360,7 @@ sg_open(struct inode *inode, struct file *filp)
> goto error_mutex_locked;
> }
> } else {
> - if (sdp->exclude) {
> + if (SG_HAVE_EXCLUDE(sdp)) {
> retval = -EBUSY;
> goto error_mutex_locked;
> }
> @@ -367,10 +373,10 @@ sg_open(struct inode *inode, struct file *fil
Re: [PATCH v5 11/23] sg: improve naming
On 10/8/19 9:50 AM, Douglas Gilbert wrote: > Remove use of typedef sg_io_hdr_t and replace with struct > sg_io_hdr. Change some names on driver wide structure fields > and comment them. > > Signed-off-by: Douglas Gilbert > --- > drivers/scsi/sg.c | 234 +- > 1 file changed, 125 insertions(+), 109 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v4 1/2] sr: need to check whether sshdr is valid in sr_do_ioctl
zhengbin, > Like other callers of scsi_execute(send_trespass_cmd, hp_sw_tur...), > we need to check whether sshdr is valid. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On 10/18/19 10:24 AM, zhengbin wrote: > v1->v2: modify the comments suggested by Bart > v2->v3: fix bug in sr_do_ioctl > v3->v4: let "fix bug in sr_do_ioctl" be a separate patch > v4->v5: fix uninit-value access bug in callers, not in __scsi_execute > > zhengbin (13): > scsi: core: need to check the result of scsi_execute in > scsi_report_opcode > scsi: core: need to check the result of scsi_execute in > scsi_test_unit_ready > scsi: core: need to check the result of scsi_execute in > scsi_report_lun_scan > scsi: sr: need to check the result of scsi_execute in sr_get_events > scsi: sr: need to check the result of scsi_execute in sr_do_ioctl > scsi: scsi_dh_emc: need to check the result of scsi_execute in > send_trespass_cmd > scsi: scsi_dh_rdac: need to check the result of scsi_execute in > send_mode_select > scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in > hp_sw_tur,hp_sw_start_stop > scsi: scsi_dh_alua: need to check the result of scsi_execute in > alua_rtpg,alua_stpg > scsi: scsi_transport_spi: need to check whether sshdr is valid in > spi_execute > scsi: cxlflash: need to check whether sshdr is valid in read_cap16 > scsi: ufs: need to check whether sshdr is valid in > ufshcd_set_dev_pwr_mode > scsi: ch: need to check whether sshdr is valid in ch_do_scsi > > drivers/scsi/ch.c | 6 -- > drivers/scsi/cxlflash/superpipe.c | 2 +- > drivers/scsi/device_handler/scsi_dh_alua.c | 6 -- > drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++- > drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 -- > drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +--- > drivers/scsi/scsi.c | 2 +- > drivers/scsi/scsi_lib.c | 3 +++ > drivers/scsi/scsi_scan.c| 3 ++- > drivers/scsi/scsi_transport_spi.c | 1 + > drivers/scsi/sr.c | 3 ++- > drivers/scsi/sr_ioctl.c | 6 ++ > drivers/scsi/ufs/ufshcd.c | 3 ++- > 13 files changed, 37 insertions(+), 15 deletions(-) > > -- > 2.7.4 > Nope. The one thing which I patently don't like is the ambivalence between DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ of them is set? IE what would be the correct way of action if DRIVER_SENSE is not set, but we have a valid sense code? Or the other way around? But more important, from a quick glance not all drivers set the DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is never evaluated after this patchset. I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on scsi_sense_valid() only. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v4 2/2] scsi: core: fix uninit-value access of variable sshdr
zhengbin, > We can init sshdr in sr_get_events, but there have many callers of > scsi_execute, scsi_execute_req, we have to troubleshoot all callers, > the simpler way is init sshdr in __scsi_execute. There aren't that many callers. I'd prefer to make sure that everybody is handling DRIVER_SENSE and scsi_sense_valid() correctly. Looks like we're generally OK, but please verify. Thanks! -- Martin K. Petersen Oracle Linux Engineering
RE: [RFC PATCH] scsi: aacraid: aac_schedule_bus_scan() can be static
Acked-by: Balsundar P < balsunda...@microchip.com>
-Original Message-
From: kbuild test robot
Sent: Wednesday, October 16, 2019 00:28
To: balsunda...@microsemi.com
Cc: kbuild-...@lists.01.org; linux-scsi@vger.kernel.org;
j...@linux.vnet.ibm.com; aacr...@microsemi.com
Subject: [RFC PATCH] scsi: aacraid: aac_schedule_bus_scan() can be static
External E-Mail
Fixes: ffcdda7d81b4 ("scsi: aacraid: send AIF request post IOP RESET")
Signed-off-by: kbuild test robot
---
commsup.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 1c3beea2b3c57..5a8a999606ea3 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1464,7 +1464,7 @@ static void aac_handle_aif(struct aac_dev * dev, struct
fib * fibptr)
}
}
-void aac_schedule_bus_scan(struct aac_dev *aac)
+static void aac_schedule_bus_scan(struct aac_dev *aac)
{
if (aac->sa_firmware)
aac_schedule_safw_scan_worker(aac);
Re: [PATCH v4 2/2] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/18 10:40, Martin K. Petersen wrote: > zhengbin, > >> We can init sshdr in sr_get_events, but there have many callers of >> scsi_execute, scsi_execute_req, we have to troubleshoot all callers, >> the simpler way is init sshdr in __scsi_execute. > There aren't that many callers. I'd prefer to make sure that everybody > is handling DRIVER_SENSE and scsi_sense_valid() correctly. Looks like > we're generally OK, but please verify. OK, I have troubleshoot callers, there are similar bug(scsi_report_opcode, cache_type_store, scsi_test_unit_ready, scsi_report_lun_scan, sd_spinup_disk, read_capacity_16, read_capacity_10, sr_get_events, alua_rtpg, alua_stpg, send_trespass_cmd, hp_sw_tur, hp_sw_start_stop, send_mode_select, sd_sync_cache, sd_start_stop_device, sr_do_ioctl). I modify these in a patch? or every .c a patch, use a patchset? > > Thanks! >
Re: [PATCH v4 1/2] sr: need to check whether sshdr is valid in sr_do_ioctl
zhengbin, >> Like other callers of scsi_execute(send_trespass_cmd, hp_sw_tur...), >> we need to check whether sshdr is valid. > > Applied to 5.4/scsi-fixes, thanks! Actually, after looking at this again, the function is making the assumption that if driver_byte(result) != 0, then the sense is present. It really should check both driver_byte(result) == DRIVER_SENSE and scsi_sense_valid(sshdr) before poking at the sense data. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v4 2/2] scsi: core: fix uninit-value access of variable sshdr
zhengbin, > I modify these in a patch? or every .c a patch, use a patchset? A patchset consisting of one patch per file, please. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] lpfc: remove left-over BUILD_NVME defines
Hannes, > The BUILD_NVME define never got defined anywhere, causing NVMe > commands to be treated as SCSI commands when freeing the buffers. > This was causing a stuck discovery and a horrible crash in > lpfc_set_rrq_active() later on. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: core: try to get module before removing devcie
>> We have a test case like block/001 in blktests, which will create a >> scsi device by loading scsi_debug module and then try to delete the >> device by sysfs interface. At the same time, it may remove the >> scsi_debug module. Applied to 5.4/scsi-fixes, thanks! > Please consider to contribute the test case to the blktests project. Yes, please do! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] hpsa: add missing hunks in reset-patch
Don, > - correct returning from reset before outstanding > commands are completed for the device. Applied to 5.4/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
RE: [RFC PATCH V4 2/2] scsi: core: don't limit per-LUN queue depth for SSD
> On 10/9/19 2:32 AM, Ming Lei wrote: > > @@ -354,7 +354,8 @@ void scsi_device_unbusy(struct scsi_device *sdev, > struct scsi_cmnd *cmd) > > if (starget->can_queue > 0) > > atomic_dec(&starget->target_busy); > > > > - atomic_dec(&sdev->device_busy); > > + if (!blk_queue_nonrot(sdev->request_queue)) > > + atomic_dec(&sdev->device_busy); > > } > > > > Hi Ming, > > Does this patch impact the meaning of the queue_depth sysfs attribute (see > also sdev_store_queue_depth()) and also the queue depth ramp up/down > mechanism (see also scsi_handle_queue_ramp_up())? Have you considered to > enable/disable busy tracking per LUN depending on whether or not sdev- > >queue_depth < shost->can_queue? > > The megaraid and mpt3sas drivers read sdev->device_busy directly. Is the > current version of this patch compatible with these drivers? We need to know per scsi device outstanding in mpt3sas and megaraid_sas driver. Can we get supporting API from block layer (through SML) ? something similar to "atomic_read(&hctx->nr_active)" which can be derived from sdev->request_queue->hctx ? At least for those driver which is nr_hw_queue = 1, it will be useful and we can avoid sdev->device_busy dependency. Kashyap > > Thanks, > > Bart.
Re: [PATCH] lpfc: remove left-over BUILD_NVME defines
On 10/17/2019 8:00 AM, Hannes Reinecke wrote:
The BUILD_NVME define never got defined anywhere, causing
NVMe commands to be treated as SCSI commands when freeing
the buffers.
This was causing a stuck discovery and a horrible crash
in lpfc_set_rrq_active() later on.
Fixes: c00f62e6c546 ("scsi: lpfc: Merge per-protocol WQ/CQ pairs into single per-cpu
pair")
Signed-off-by: Hannes Reinecke
---
drivers/scsi/lpfc/lpfc_init.c | 2 --
drivers/scsi/lpfc/lpfc_scsi.c | 2 --
2 files changed, 4 deletions(-)
Yep. Thanks.
Reviewed-by: James Smart
-- james
Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/17 10:45, Bart Van Assche wrote:
> On 2019-10-11 20:25, zhengbin wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 5447738..d5e29c5 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const
>> unsigned char *cmd,
>> struct scsi_request *rq;
>> int ret = DRIVER_ERROR << 24;
>>
>> +/*
>> + * Zero-initialize sshdr for those callers that check the *sshdr
>> + * contents even if no sense data is available.
>> + */
>> +if (sshdr)
>> +memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
>> +
>> req = blk_get_request(sdev->request_queue,
>> data_direction == DMA_TO_DEVICE ?
>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> Although I don't have a strong opinion about this, I'm still wondering
> whether 'sshdr' should be initialized in __scsi_execute() or by its caller.
@jejb, @martin, any suggestion?
>
>> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
>> index ffcf902..335cfdd 100644
>> --- a/drivers/scsi/sr_ioctl.c
>> +++ b/drivers/scsi/sr_ioctl.c
>> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>>
>> /* Minimal error checking. Ignore cases we know about, and report the
>> rest. */
>> if (driver_byte(result) != 0) {
>> +if (!scsi_sense_valid(sshdr)) {
>> +err = -EIO;
>> +goto out;
>> +}
>> +
>> switch (sshdr->sense_key) {
>> case UNIT_ATTENTION:
>> SDev->changed = 1;
> Shouldn't this be a separate patch?
OK, will send a separate patch
>
> Thanks,
>
> Bart.
>
> .
>
Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
On 2019-10-11 20:25, zhengbin wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5447738..d5e29c5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
> struct scsi_request *rq;
> int ret = DRIVER_ERROR << 24;
>
> + /*
> + * Zero-initialize sshdr for those callers that check the *sshdr
> + * contents even if no sense data is available.
> + */
> + if (sshdr)
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
> req = blk_get_request(sdev->request_queue,
> data_direction == DMA_TO_DEVICE ?
> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
Although I don't have a strong opinion about this, I'm still wondering
whether 'sshdr' should be initialized in __scsi_execute() or by its caller.
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index ffcf902..335cfdd 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>
> /* Minimal error checking. Ignore cases we know about, and report the
> rest. */
> if (driver_byte(result) != 0) {
> + if (!scsi_sense_valid(sshdr)) {
> + err = -EIO;
> + goto out;
> + }
> +
> switch (sshdr->sense_key) {
> case UNIT_ATTENTION:
> SDev->changed = 1;
Shouldn't this be a separate patch?
Thanks,
Bart.
Re: [PATCH] scsi: core: try to get module before removing devcie
On 2019-10-15 06:05, Yufen Yu wrote: > We have a test case like block/001 in blktests, which will > create a scsi device by loading scsi_debug module and then > try to delete the device by sysfs interface. At the same time, > it may remove the scsi_debug module. Please consider to contribute the test case to the blktests project. Anyway: Reviewed-by: Bart Van Assche
Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/17 8:05, Finn Thain wrote: > On Sat, 12 Oct 2019, zhengbin wrote: > >> kmsan report a warning in 5.1-rc4: >> >> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline] >> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 >> drivers/scsi/sr.c:243 >> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: GB >> 5.1.0-rc4+ #8 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> Ubuntu-1.8.2-1ubuntu1 04/01/2014 >> Call Trace: >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x173/0x1d0 lib/dump_stack.c:113 >> kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619 >> __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310 >> sr_get_events drivers/scsi/sr.c:207 [inline] >> sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243 >> >> The reason is as follows: >> sr_get_events >> struct scsi_sense_hdr sshdr; -->uninit >> scsi_execute_req -->If fail, will not set sshdr >> scsi_sense_valid(&sshdr) -->access sshdr >> >> We can init sshdr in sr_get_events, but there have many callers of >> scsi_execute, scsi_execute_req, we have to troubleshoot all callers, >> the simpler way is init sshdr in __scsi_execute. >> >> BTW: sr_do_ioctl should check whether sshdr is valid, fix this >> >> Signed-off-by: zhengbin >> --- >> v1->v2: modify the comments suggested by Bart >> v2->v3: fix bug in sr_do_ioctl >> drivers/scsi/scsi_lib.c | 7 +++ >> drivers/scsi/sr_ioctl.c | 5 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 5447738..d5e29c5 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const >> unsigned char *cmd, >> struct scsi_request *rq; >> int ret = DRIVER_ERROR << 24; >> >> +/* >> + * Zero-initialize sshdr for those callers that check the *sshdr >> + * contents even if no sense data is available. >> + */ >> +if (sshdr) >> +memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); >> + >> req = blk_get_request(sdev->request_queue, >> data_direction == DMA_TO_DEVICE ? >> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); > Does this have any effect? It appears __scsi_execute() calls > scsi_normalize_sense(), which already does > memset(sshdr, 0, sizeof(struct scsi_sense_hdr)). __scsi_execute if (IS_ERR(req)) -->not init return ret; if (bufflen && ...) -->not init goto out; scsi_normalize_sense >
Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
On Sat, 12 Oct 2019, zhengbin wrote:
> kmsan report a warning in 5.1-rc4:
>
> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline]
> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: GB 5.1.0-rc4+
> #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x173/0x1d0 lib/dump_stack.c:113
> kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619
> __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310
> sr_get_events drivers/scsi/sr.c:207 [inline]
> sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
>
> The reason is as follows:
> sr_get_events
> struct scsi_sense_hdr sshdr; -->uninit
> scsi_execute_req -->If fail, will not set sshdr
> scsi_sense_valid(&sshdr) -->access sshdr
>
> We can init sshdr in sr_get_events, but there have many callers of
> scsi_execute, scsi_execute_req, we have to troubleshoot all callers,
> the simpler way is init sshdr in __scsi_execute.
>
> BTW: sr_do_ioctl should check whether sshdr is valid, fix this
>
> Signed-off-by: zhengbin
> ---
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> drivers/scsi/scsi_lib.c | 7 +++
> drivers/scsi/sr_ioctl.c | 5 +
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5447738..d5e29c5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
> struct scsi_request *rq;
> int ret = DRIVER_ERROR << 24;
>
> + /*
> + * Zero-initialize sshdr for those callers that check the *sshdr
> + * contents even if no sense data is available.
> + */
> + if (sshdr)
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
> req = blk_get_request(sdev->request_queue,
> data_direction == DMA_TO_DEVICE ?
> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
Does this have any effect? It appears __scsi_execute() calls
scsi_normalize_sense(), which already does
memset(sshdr, 0, sizeof(struct scsi_sense_hdr)).
--
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index ffcf902..335cfdd 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>
> /* Minimal error checking. Ignore cases we know about, and report the
> rest. */
> if (driver_byte(result) != 0) {
> + if (!scsi_sense_valid(sshdr)) {
> + err = -EIO;
> + goto out;
> + }
> +
> switch (sshdr->sense_key) {
> case UNIT_ATTENTION:
> SDev->changed = 1;
> --
> 2.7.4
>
>
Re: [PATCH 6/7] scsi: aacraid: send AIF request post IOP RESET
Hi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on scsi/for-next] [cannot apply to v5.4-rc3 next-20191014] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/balsundar-p-microsemi-com/scsi-aacraid-updates/20191015-142326 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next reproduce: # apt-get install sparse # sparse version: v0.6.1-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot sparse warnings: (new ones prefixed by >>) >> drivers/scsi/aacraid/commsup.c:1467:6: sparse: sparse: symbol >> 'aac_schedule_bus_scan' was not declared. Should it be static? drivers/scsi/aacraid/commsup.c:2385:5: sparse: sparse: symbol 'aac_send_safw_hostttime' was not declared. Should it be static? drivers/scsi/aacraid/commsup.c:2414:5: sparse: sparse: symbol 'aac_send_hosttime' was not declared. Should it be static? drivers/scsi/aacraid/commsup.c:599:17: sparse: sparse: context imbalance in 'aac_fib_send' - different lock contexts for basic block drivers/scsi/aacraid/commsup.c:754:17: sparse: sparse: context imbalance in 'aac_hba_send' - different lock contexts for basic block drivers/scsi/aacraid/commsup.c:1502:32: sparse: sparse: context imbalance in '_aac_reset_adapter' - unexpected unlock Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v2] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/12 10:06, James Bottomley wrote: > On Sat, 2019-10-12 at 10:03 +0800, zhengbin (A) wrote: >> On 2019/10/12 9:58, James Bottomley wrote: >>> On Sat, 2019-10-12 at 09:26 +0800, zhengbin wrote: BTW: we can't just init sshdr->response_code, sr_do_ioctl use sshdr->sense_key >>> That's an actual bug, isn't it? >> If we init sshdr in __scsi_execute, this will be ok > No I mean it's a bug because sr_do_ioctl shouldn't be acting on sense > that isn't valid. So all uses of sshdr should be gated on a validity > check. Yes you are right, the right way is use scsi_sense_valid(&sshdr), I have troubleshoot callers, this is the only wrong use(Maybe I miss some...). > > James > > > . >
Re: [PATCH v2] scsi: core: fix uninit-value access of variable sshdr
On Sat, 2019-10-12 at 10:03 +0800, zhengbin (A) wrote: > On 2019/10/12 9:58, James Bottomley wrote: > > On Sat, 2019-10-12 at 09:26 +0800, zhengbin wrote: > > > BTW: we can't just init sshdr->response_code, sr_do_ioctl use > > > sshdr->sense_key > > > > That's an actual bug, isn't it? > > If we init sshdr in __scsi_execute, this will be ok No I mean it's a bug because sr_do_ioctl shouldn't be acting on sense that isn't valid. So all uses of sshdr should be gated on a validity check. James
Re: [PATCH v2] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/12 9:58, James Bottomley wrote: > On Sat, 2019-10-12 at 09:26 +0800, zhengbin wrote: >> BTW: we can't just init sshdr->response_code, sr_do_ioctl use >> sshdr->sense_key > That's an actual bug, isn't it? If we init sshdr in __scsi_execute, this will be ok > > James > > > . >
Re: [PATCH v2] scsi: core: fix uninit-value access of variable sshdr
On Sat, 2019-10-12 at 09:26 +0800, zhengbin wrote: > BTW: we can't just init sshdr->response_code, sr_do_ioctl use > sshdr->sense_key That's an actual bug, isn't it? James
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 5:05 AM, zhengbin wrote: + /* +* need to initial sshdr to avoid uninit-value access +*/ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + I think the above comment is slightly confusing because it is correct for some callers but not for all callers of scsi_execute(). How about changing the comment into something like the following: "Zero-initialize sshdr for those callers that check the *sshdr contents even if no sense data is available"? Thanks, Bart.
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 8:07 PM, zhengbin (A) wrote: Besides, scsi_sense_hdr is just 8 bytes, memset it to 0 will not affect performance That's true ... Bart.
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/11 1:03, Bart Van Assche wrote: > On 10/10/19 5:05 AM, zhengbin wrote: >> kmsan report a warning in 5.1-rc4: >> >> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline] >> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 >> drivers/scsi/sr.c:243 >> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: G B >> 5.1.0-rc4+ #8 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> Ubuntu-1.8.2-1ubuntu1 04/01/2014 >> Call Trace: >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x173/0x1d0 lib/dump_stack.c:113 >> kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619 >> __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310 >> sr_get_events drivers/scsi/sr.c:207 [inline] >> sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243 >> >> The reason is as follows: >> sr_get_events >> struct scsi_sense_hdr sshdr; -->uninit >> scsi_execute_req -->If fail, will not set sshdr >> scsi_sense_valid(&sshdr) -->access sshdr >> >> We can init sshdr in sr_get_events, but there have many callers of >> scsi_execute, scsi_execute_req, we have to troubleshoot all callers, >> the simpler way is init sshdr in __scsi_execute. >> >> BTW: we can't just init sshdr->response_code, sr_do_ioctl use >> sshdr->sense_key(Need to troubleshoot all callers) >> >> Signed-off-by: zhengbin >> --- >> drivers/scsi/scsi_lib.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 5447738..037fb2a 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -255,6 +255,12 @@ int __scsi_execute(struct scsi_device *sdev, const >> unsigned char *cmd, >> struct scsi_request *rq; >> int ret = DRIVER_ERROR << 24; >> >> + /* >> + * need to initial sshdr to avoid uninit-value access >> + */ >> + if (sshdr) >> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); >> + >> req = blk_get_request(sdev->request_queue, >> data_direction == DMA_TO_DEVICE ? >> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); > > From SAM-5: "Sense data shall be made available by the logical unit if a > command terminates with CHECK CONDITION status or other conditions (e.g., the > processing of a REQUEST SENSE command). The format, content, and conditions > under which sense data shall be prepared by the logical unit are as defined > in this standard, SPC-4, the applicable command standard, and the applicable > SCSI transport protocol standard." Even this, I suggest we should still init sshdr first(this is hard to understand) > > Apparently sr_check_events() submits a GET EVENT STATUS NOTIFICATION command > and it even evaluates the sense data if that command succeeded. I'm not aware > of other scsi_execute() callers that do this. So I'm not sure that modifying > __scsi_execute() is the best approach. I'm wondering whether the sr driver > should be fixed instead of modifying __scsi_execute(). I have troubleshoot callers, there are similar bug(scsi_report_opcode, cache_type_store, scsi_test_unit_ready, scsi_report_lun_scan, sd_spinup_disk, read_capacity_16, read_capacity_10, sr_get_events, alua_rtpg, alua_stpg, send_trespass_cmd, hp_sw_tur, hp_sw_start_stop, send_mode_select, sd_sync_cache, sd_start_stop_device, sr_do_ioctl). In __scsi_execute, if a command was executed, sshdr was set, so if failed, init sshdr should be ok too. Besides, scsi_sense_hdr is just 8 bytes, memset it to 0 will not affect performance > > Thanks, > > Bart. > > . >
Re: scsi: lpfc: Fix hardlockup in lpfc_abort_handler
On 10/10/2019 1:59 AM, Zhangguanghui wrote: Hi everyone Please refer to the latest patch. There is a race deadlock in the function lpfc_abort_handler potential deadlocks arising from lock ordering problems. It’s the correct order spin_unlock(&lpfc_cmd->buf_lock) spin_unlock_irqrestore(&phba->hbalock, flags); How to solve it ? I think that the patch is reasonable, can you help me review and commit this patch, Best regards diff --git a/src/lpfc-12.2.0.0/lpfc_scsi.c b/src/lpfc-12.2.0.0/lpfc_scsi.c index 3f1375a..19c8505 100644 --- a/src/lpfc-12.2.0.0/lpfc_scsi.c +++ b/src/lpfc-12.2.0.0/lpfc_scsi.c We confirmed the issue you stated. We are looking at what you proposed and will be adding a patch that will be posted in our next patch set after we've put it through some regression testing. -- james
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 5:05 AM, zhengbin wrote: kmsan report a warning in 5.1-rc4: BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline] BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243 CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: GB 5.1.0-rc4+ #8 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x173/0x1d0 lib/dump_stack.c:113 kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619 __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310 sr_get_events drivers/scsi/sr.c:207 [inline] sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243 The reason is as follows: sr_get_events struct scsi_sense_hdr sshdr; -->uninit scsi_execute_req -->If fail, will not set sshdr scsi_sense_valid(&sshdr) -->access sshdr We can init sshdr in sr_get_events, but there have many callers of scsi_execute, scsi_execute_req, we have to troubleshoot all callers, the simpler way is init sshdr in __scsi_execute. BTW: we can't just init sshdr->response_code, sr_do_ioctl use sshdr->sense_key(Need to troubleshoot all callers) Signed-off-by: zhengbin --- drivers/scsi/scsi_lib.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 5447738..037fb2a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -255,6 +255,12 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, struct scsi_request *rq; int ret = DRIVER_ERROR << 24; + /* +* need to initial sshdr to avoid uninit-value access +*/ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + req = blk_get_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); From SAM-5: "Sense data shall be made available by the logical unit if a command terminates with CHECK CONDITION status or other conditions (e.g., the processing of a REQUEST SENSE command). The format, content, and conditions under which sense data shall be prepared by the logical unit are as defined in this standard, SPC-4, the applicable command standard, and the applicable SCSI transport protocol standard." Apparently sr_check_events() submits a GET EVENT STATUS NOTIFICATION command and it even evaluates the sense data if that command succeeded. I'm not aware of other scsi_execute() callers that do this. So I'm not sure that modifying __scsi_execute() is the best approach. I'm wondering whether the sr driver should be fixed instead of modifying __scsi_execute(). Thanks, Bart.
Re: [PATCH] ch: Make it again possible to open a ch device two or more times
Bart, > Clearing ch->device in ch_release() is wrong because that pointer must > remain valid until ch_remove() is called. This patch fixes the > following crash the second time a ch device is opened: Applied to 5.4/scsi-fixes, thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] megaraid_sas: unique names for MSIx vectors
Chandrakanth, > Currently, MSIx vectors name appears in /proc/interrupts is "megasas" > which is same for all the vectors. This patch provides the unique > name to all megaraid_sas controllers and its associated MSIx interrupts. > > Suggested-by: Konstantin Shalygin > Signed-off-by: Sumit Saxena > Signed-off-by: Chandrakanth Patil Next time, please make sure you put a "---" separator before the patch changelog. > v2: [...] Applied to 5.5/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/10] smartpqi updates
Don, > These patches are based on Linus's tree Applied to 5.5/scsi-queue. There was some fuzz but I think I figured it out. -- Martin K. Petersen Oracle Linux Engineering
