Re: scsi: ufs: add support for query requests

[Dolev Raviv]

Santosh, thank you very much for your comments. Most where adopted, others
where replied inline.

Please note the comment on removing ufs_coe.c file. I will appreciated
your opinion on that.

 linux-...@vger.kernel.org is for UFS filesystem.

 The API for submitting a query request is ufs_query_request() in
ufs_core.c. This function is responsible for:

 This can be part of ufshcd.c which is actually the core file, no need to
create a new core file for the function.

You are right and I moved it for now. But, I think that a good design that
will allow easy extension and feature development, will be to separate the
hci and features from the rest.

 +
 +#define UFS_QUERY_RESERVED_SCSI_CMD_SIZE 10
 +

 Move this to ufs.h, The name seems too big, it can be changed to
UFS_QUERY_CMD_SIZE.

 + if (!hba || !query || !response) {
 +   pr_err(%s: NULL pointer hba = %p, query = %p response
= %p,
 +   __func__, hba, query, response); +
return -EINVAL;
 + }

 The function will be called withing the driver, So, no need for these
checks. The caller will/must pass the correct parameters.
I disagree. Why not play it safe?

Anyone can make this function accessible for IOCTLs or EXPORT it in
future. In case they forget (most do), let's not leave holes.

 + /*
 + * A SCSI command structure is composed from opcode at the
 + * begining and 0 at the end.
 + */
 + memset(cmd, 0, UFS_QUERY_RESERVED_SCSI_CMD_SIZE);
 + cmd[0] = UFS_QUERY_RESERVED_SCSI_CMD;

 Remove memset. Anyway only the first byte is being checked in
 ufshcd_is_query_req();

 + * ufshcd_is_valid_query_rsp - checks if controller TR response is valid
 + * @ucd_rsp_ptr: pointer to response UPIU
 + *
 + * This function checks the response UPIU for valid transaction type
in + * response field
 + * Returns 0 on success, non-zero on failure
 + */
 +static inline int
 +ufshcd_is_valid_query_rsp(struct utp_upiu_rsp *ucd_rsp_ptr)

 Combine this with ufshcd_is_valid_req_rsp(), accordingly handle it in
ufshcd_transfer_rsp_status().

 +static inline void ufshcd_copy_query_response(struct ufs_hba *hba, +
struct ufshcd_lrb *lrbp)
 +{
 + struct ufs_query_res *query_res = hba-query.response;
 + u8 *descp = (u8 *)lrbp-ucd_rsp_ptr + GENERAL_UPIU_REQUEST_SIZE;

 This can be done inside the following if condition i.e. if
 (hba-query.descriptor != NULL).
 and change the condition to if (!hba-query.descriptor).

 + /* Get the descriptor */
 + if (hba-query.descriptor != NULL  lrbp-ucd_rsp_ptr-qr.opcode ==
+ UPIU_QUERY_OPCODE_READ_DESC) {


 +static int ufshcd_compose_upiu(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
 +{
 + struct utp_upiu_req *ucd_req_ptr;

 Remove ucd_req_ptr, it is not doing anything.

 + if (!lrbp || !lrbp-cmd || !lrbp-ucd_req_ptr ||
 + !lrbp-utr_descriptor_ptr) { +   
if (!lrbp)
 +  pr_err(%s: lrbp can not be NULL, __func__); + 
 if (!lrbp-cmd)
 +  pr_err(%s: lrbp-cmd can not be NULL,
 __func__);
 +if (!lrbp-ucd_req_ptr)
 +  pr_err(%s: ucd_req_ptr can not be NULL,
__func__);
 + if (!lrbp-utr_descriptor_ptr)
 +  pr_err(%s: utr_descriptor_ptr can not be NULL,
+ __func__);
 +  ret = -EINVAL;
 +  goto exit;
 + }

I rather play it safe and not remove it.
I rewrote it to make it cleaner.

 These are redundant, remove them.

 + if (ufshcd_is_query_req(lrbp))
 + lrbp-command_type = UTP_CMD_TYPE_DEV_MANAGE;
 + else
 + lrbp-command_type = UTP_CMD_TYPE_SCSI;

  /* form UPIU before issuing the command */
 - ufshcd_compose_upiu(lrbp);
 + ufshcd_compose_upiu(hba, lrbp);
 err = ufshcd_map_sg(lrbp);
  if (err)
 goto out;

 Why call ufshcd_map_sg() and scsi_dma_unmap() in
 ufshcd_transfer_req_compl() for requests of type
 UTP_CMD_TYPE_DEV_MANAGE?

We are doing this since there is no harm calling it when the buffer is
null, as well as this flow is responsible for setting the length parameter
in the transfer request descriptor.
Bottom line there is no need to split the original flow.

 ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)

 + if (ufshcd_is_query_req(lrbp))
 + result = ufshcd_is_valid_query_rsp(lrbp-ucd_rsp_ptr);
 + else
 + result = ufshcd_is_valid_req_rsp(lrbp-ucd_rsp_ptr);
 +

 As already mentioned you can combine these.

 +/* Query response result code */
 +enum {
 + QUERY_RESULT_SUCCESS = 0x00,
 + QUERY_RESULT_NOT_READABLE = 0xF6,
 + QUERY_RESULT_NOT_WRITEABLE = 0xF7,
 + QUERY_RESULT_ALREADY_WRITTEN = 0xF8,
 + QUERY_RESULT_INVALID_LENGTH = 0xF9,
 + QUERY_RESULT_INVALID_VALUE = 0xFA,
 + QUERY_RESULT_INVALID_SELECTOR = 0xFB,
 + QUERY_RESULT_INVALID_INDEX = 0xFC,
 + QUERY_RESULT_INVALID_IDN = 0xFD,
 + QUERY_RESULT_INVALID_OPCODE = 0xFE,
 + QUERY_RESULT_GENERAL_FAILURE = 0xFF,
 +};

 Move this to ufs.h.


 --
 ~Santosh


-- 
Dolev
-- 
QUALCOMM ISRAEL, on behalf of 

[PATCH V2] scsi: ufs: add support for query requests

[Dolev Raviv]

Add support for sending UFS query requests through tagged command
queuing. This design allows queuing query requests in any open slot
along with other SCSI commands. In this way there is no need to
save a slot in the requests queue and decrease its size.

A query request is posing to a SCSI command to use native flow. But
unlike normal SCSI command flow, the data and response fields are
filled in UFS host data structure instead of passing as arguments
while queuing into SCSI mlqueue (mid-layer SCSI queue, the requests
from this queue are submitted to the device queue). As per specification
only one query request is allowed to be processed by device. Hence a
mutex lock for protecting data and response fields (hba-query.request and
hba-query.response) needs to be held while query request is in
progress.

The API for submitting a query request is ufs_query_request() in
ufshcd.c. This function is responsible for:
1. Obtaining the SCSI device from the host
2. Keeping the query mutex to prevent multiple queries
3. Storing the required data for sending a query request in ufs_hba
4. Queuing a SCSI vendor specific command to trigger a query request
   in the UFS driver.

The callers of ufs_query_request() are expected to fill the query
command data fields and are to provide an allocated response field
for the driver to fill response fields after request completion.

The request and response upiu is extended in a union to enable using the
same data structure, both for command upiu and query request upiu.

The query request flow is separated from the scsi command flow in:
1. Preparing the request
2. Validating response (error) codes
3. Copying data (only used for descriptor read/write query requests)
4. Copying response/sense

Data error can't be handled in the scsi command native flow. Hence,
we pass the code as without a change back to the submitting layer.

UPIU (UFS Protocol Information Unit) size is increased to 512 bytes
from 128. The UPIU header and the transaction specific fields (SCSI
command or Query Request OSF - Op-code Specific Fields) are 32 bytes
together, the rest is used to transfer extra request data (such as
descriptor in query requests). In order to accommodate the largest
descriptor in the UFS spec (256 bytes) we need to increase the UPIU
size.

Signed-off-by: Dolev Raviv dra...@codeaurora.org
--- 
changes for v2:
- Moved ufs_query_request to ufshcd.c and removed ufs_core.c
- Addressed Santosh's comments

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 139bc06..5d2208a 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -36,10 +36,20 @@
 #ifndef _UFS_H
 #define _UFS_H
 
+#include linux/mutex.h
+#include linux/types.h
+
 #define MAX_CDB_SIZE   16
+#define GENERAL_UPIU_REQUEST_SIZE 32
+#define UPIU_HEADER_DATA_SEGMENT_MAX_SIZE  ((ALIGNED_UPIU_SIZE) - \
+   (GENERAL_UPIU_REQUEST_SIZE))
+#define QUERY_OSF_SIZE ((GENERAL_UPIU_REQUEST_SIZE) - \
+   (sizeof(struct utp_upiu_header)))
+#define UFS_QUERY_RESERVED_SCSI_CMD 0xCC
+#define UFS_QUERY_CMD_SIZE 10
 
 #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
-   ((byte3  24) | (byte2  16) |\
+   cpu_to_be32((byte3  24) | (byte2  16) |\
 (byte1  8) | (byte0))
 
 /*
@@ -62,7 +72,7 @@ enum {
UPIU_TRANSACTION_COMMAND= 0x01,
UPIU_TRANSACTION_DATA_OUT   = 0x02,
UPIU_TRANSACTION_TASK_REQ   = 0x04,
-   UPIU_TRANSACTION_QUERY_REQ  = 0x26,
+   UPIU_TRANSACTION_QUERY_REQ  = 0x16,
 };
 
 /* UTP UPIU Transaction Codes Target to Initiator */
@@ -90,6 +100,12 @@ enum {
UPIU_TASK_ATTR_ACA  = 0x03,
 };
 
+/* UPIU Query request function */
+enum {
+   UPIU_QUERY_FUNC_STANDARD_READ_REQUEST = 0x01,
+   UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST = 0x81,
+};
+
 /* UTP QUERY Transaction Specific Fields OpCode */
 enum {
UPIU_QUERY_OPCODE_NOP   = 0x0,
@@ -103,6 +119,21 @@ enum {
UPIU_QUERY_OPCODE_TOGGLE_FLAG   = 0x8,
 };
 
+/* Query response result code */
+enum {
+   QUERY_RESULT_SUCCESS= 0x00,
+   QUERY_RESULT_NOT_READABLE   = 0xF6,
+   QUERY_RESULT_NOT_WRITEABLE  = 0xF7,
+   QUERY_RESULT_ALREADY_WRITTEN= 0xF8,
+   QUERY_RESULT_INVALID_LENGTH = 0xF9,
+   QUERY_RESULT_INVALID_VALUE  = 0xFA,
+   QUERY_RESULT_INVALID_SELECTOR   = 0xFB,
+   QUERY_RESULT_INVALID_INDEX  = 0xFC,
+   QUERY_RESULT_INVALID_IDN= 0xFD,
+   QUERY_RESULT_INVALID_OPCODE = 0xFE,
+   QUERY_RESULT_GENERAL_FAILURE= 0xFF,
+};
+
 /* UTP Transfer Request Command Type (CT) */
 enum {
UPIU_COMMAND_SET_TYPE_SCSI  = 0x0,
@@ -110,10 +141,17 @@ enum {
UPIU_COMMAND_SET_TYPE_QUERY = 0x2,
 };
 
+/* UTP Transfer Request Command 

Re: [PATCH v2 0/8] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Thu, 2013-04-11 at 16:52 -0500, Jeremy Linton wrote:
   What happened to this patch? The trail of suggested fixes for the 
 REPORT LUNS
 DATA HAS CHANGED check condition is getting pretty long. The number of devices
 (our product included) in the field that have the ability to on the fly modify
 the luns on an I_T nexus is not decreasing.

I haven't heard back about it.  If some people would ACK it I think that
would help.  I also submitted a separate patch for automatic LUN
removal.

 
   Is it because these patches are trying to fix more than one thing?
 
   What is the preferred way to fix this?
 
   Why not simply add a couple sdev_evt_send_simple()'s and an event 
 coalesce
 function to collapse this event when its received from multiple LUNs on the
 I_T? A couple extra uevents isn't going to kill udev right?

Well, the patch does that, among other things.  I think handling the
other UA codes is a good idea, because existing LUNs can be
reconfigured.  Coalescing events in the kernel is necessary because
udev couldn't handle a large number of events from a big storage
configuration.

 A really
 fancy
 patch could attempt to clear the check conditions from LUNs that share the 
 I_T.

I think the mid-layer will handle that automatically.  If check
conditions are reported the commands will have to be reissued.

-Ewan
 
   
 
 
 
 


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/8] [SCSI] Enhanced sense and Unit Attention handling

[Jeremy Linton]

On 4/15/2013 9:13 AM, Ewan Milne wrote:

 patch could attempt to clear the check conditions from LUNs that share
 the I_T.
 
 I think the mid-layer will handle that automatically.  If check conditions
 are reported the commands will have to be reissued.

But, not automatically (unless i'm missing something again). The UA is 
going to
arrive when each lun gets sent a command, which could be a long time from the
initial UA if the lun is idle. Enough time, that the attempts to coalesce the
events are going to fail.

I guess it depends on what you have udev doing when it gets the event. 
If it
triggers a rescan involving something besides inquiry/report luns then that will
trigger the remaining UA's from the luns on the target that changed. But if it
does something other than that, I don't see it by reading the
patch/scsi_scan.c code.







--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi: pcmcia: nsp_cs: remove module init/exit function prototypes

[H Hartley Sweeten]

This driver now uses the module_pcmcia_driver() macro to supply the
init/exit code. The nsp_cs_{init,exit} prototypes should be removed.

Reported-by: kbuild test robot fengguang...@intel.com
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: YOKOTA Hiroshi yok...@netlab.is.tsukuba.ac.jp
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: linux-scsi@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/scsi/pcmcia/nsp_cs.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/pcmcia/nsp_cs.h b/drivers/scsi/pcmcia/nsp_cs.h
index afd64f0..ea5122f 100644
--- a/drivers/scsi/pcmcia/nsp_cs.h
+++ b/drivers/scsi/pcmcia/nsp_cs.h
@@ -326,10 +326,6 @@ static struct Scsi_Host *nsp_detect(struct 
scsi_host_template *sht);
 /* Interrupt handler */
 //static irqreturn_t nspintr(int irq, void *dev_id);
 
-/* Module entry point*/
-static int  __init nsp_cs_init(void);
-static void __exit nsp_cs_exit(void);
-
 /* Debug */
 #ifdef NSP_DEBUG
 static void show_command (struct scsi_cmnd *SCpnt);
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/8] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Mon, 2013-04-15 at 11:20 -0500, Jeremy Linton wrote:
 On 4/15/2013 9:13 AM, Ewan Milne wrote:
 
  patch could attempt to clear the check conditions from LUNs that share
  the I_T.
  
  I think the mid-layer will handle that automatically.  If check conditions
  are reported the commands will have to be reissued.
 
   But, not automatically (unless i'm missing something again). The UA is 
 going to
 arrive when each lun gets sent a command, which could be a long time from the
 initial UA if the lun is idle. Enough time, that the attempts to coalesce the
 events are going to fail.

Yes, although we can't put off rescanning for too long, or the system
won't react in a timely manner to a storage configuration change.  I
used 2 seconds which was a compromise to avoid overloading udev.  If
the UAs are received too far apart in time, then more than one event
will be generated.

Note, if multipath is in use, multipathd will periodically (every 15
seconds, I think) send commands to all the paths, so the UAs will be
detected at that point.

 
   I guess it depends on what you have udev doing when it gets the event. 
 If it
 triggers a rescan involving something besides inquiry/report luns then that 
 will
 trigger the remaining UA's from the luns on the target that changed. But if it
 does something other than that, I don't see it by reading the
 patch/scsi_scan.c code.

What I did in my testing was have a udev rule that performed a rescan.
I believe sd_revalidate_disk() ends up being called, which will perform
several commands (MODE SENSE, READ CAPACITY, etc.), for disk devices.

-Ewan



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: pcmcia: nsp_cs: remove module init/exit function prototypes

[James Bottomley]

On Mon, 2013-04-15 at 09:42 -0700, H Hartley Sweeten wrote:
 This driver now uses the module_pcmcia_driver() macro to supply the
 init/exit code. The nsp_cs_{init,exit} prototypes should be removed.

This reasoning is bogus.

Why the driver actually has all these static prototypes in its header
file is entirely unclear to me, but singling these two out for the
reason you state above is wrong (both the reason and picking only two of
them).

Hiroshi, you're the maintainer, what do you want to do ... since the
driver is ancient, I'm happy to leave it untouched, or if you want to
tidy it up, I see no reason to have any static prototypes in the header
file.

Thanks,

James




--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Patch 1/1] cciss: bug fix, prevent cciss from loading in kdump kernel

[Mike Miller]

Patch 1/1

If hpsa is selected as the Smart Array driver cciss may try to load in the
kdump kernel. When this happens kdump fails and a core file cannot be created.
This patch prevents cciss from trying to load in this scenario. This effects
primarily older Smart Array controllers.

From: Mike Miller mike.mil...@hp.com
Signed-off-by: Mike Miller mike.mil...@hp.com
---
 drivers/block/cciss.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 1c1b8e5..a6c0973 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -4960,6 +4960,12 @@ static int cciss_init_one(struct pci_dev *pdev, const 
struct pci_device_id *ent)
ctlr_info_t *h;
unsigned long flags;
 
+   /*
+* if this is the kdump kernel and the user has set the flags to
+* use hpsa rather than cciss just bail
+*/
+   if ((reset_devices)  (cciss_allow_hpsa == 1))
+   return -ENODEV;
rc = cciss_init_reset_devices(pdev);
if (rc) {
if (rc != -ENOTSUPP)
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[wenxiong]

In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd
when sending commands and retry for a bit if the driver returns a
busy response.

Thanks,
Wendy

-- 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[wenxiong]

Fix scsi_send_eh_cmnd to check the return code of queuecommand when
sending commands and retry for a bit if the LLDD returns a busy response.
This fixes an issue seen with the ipr driver where an ipr initiated reset
immediately following an eh_host_reset caused EH initiated commands to fail,
resulting in devices being taken offline. This patch resolves the issue.

Signed-off-by: Wen Xiong wenxi...@linux.vnet.ibm.com

---
 drivers/scsi/scsi_error.c |   10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: b/drivers/scsi/scsi_error.c
===
--- a/drivers/scsi/scsi_error.c 2013-04-10 12:55:57.0 -0500
+++ b/drivers/scsi/scsi_error.c 2013-04-10 13:04:12.467858487 -0500
@@ -793,6 +793,7 @@ static int scsi_send_eh_cmnd(struct scsi
DECLARE_COMPLETION_ONSTACK(done);
unsigned long timeleft;
struct scsi_eh_save ses;
+   int attempts = 30;
int rtn;
 
scsi_eh_prep_cmnd(scmd, ses, cmnd, cmnd_size, sense_bytes);
@@ -800,7 +801,14 @@ static int scsi_send_eh_cmnd(struct scsi
 
scsi_log_send(scmd);
scmd-scsi_done = scsi_eh_done;
-   shost-hostt-queuecommand(shost, scmd);
+
+   while ((rtn = shost-hostt-queuecommand(shost, scmd))  attempts) {
+   if (rtn == SCSI_MLQUEUE_DEVICE_BUSY ||
+   rtn == SCSI_MLQUEUE_TARGET_BUSY ||
+   rtn == SCSI_MLQUEUE_HOST_BUSY)
+   attempts--;
+   ssleep(1);
+   }
 
timeleft = wait_for_completion_timeout(done, timeout);
 

-- 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

re: [SCSI] qla4xxx: Add flash node mgmt support

[Dan Carpenter]

Hi Adheer,

The patch 1e9e2be3ee03: [SCSI] qla4xxx: Add flash node mgmt support 
from Mar 22, 2013, has several endian bugs.

drivers/scsi/qla4xxx/ql4_os.c
  2217  fw_ddb_entry-tgt_portal_grp = cpu_to_le16(sess-tpgt);
  2218  fw_ddb_entry-mss = cpu_to_le16(conn-max_segment_size);
  2219  fw_ddb_entry-tcp_xmt_wsf = cpu_to_le16(conn-tcp_xmit_wsf);
  ^^^
This is u8.

  2220  fw_ddb_entry-tcp_rcv_wsf = cpu_to_le16(conn-tcp_recv_wsf);
  ^^^
This is u8.

  2221  fw_ddb_entry-ipv4_tos = conn-ipv4_tos;
    fw_ddb_entry-ipv6_flow_lbl = 
cpu_to_le16(conn-ipv6_flow_label);
  2223  fw_ddb_entry-ka_timeout = cpu_to_le16(conn-keepalive_timeout);
  2224  fw_ddb_entry-lcl_port = cpu_to_le16(conn-local_port);
  2225  fw_ddb_entry-stat_sn = cpu_to_le16(conn-statsn);
  ^^^
This is u32.

  2226  fw_ddb_entry-exp_stat_sn = cpu_to_le16(conn-exp_statsn);
  ^^^
This is u32.

  2227  fw_ddb_entry-ddb_link = 
cpu_to_le16(sess-discovery_parent_type);
  2228  fw_ddb_entry-chap_tbl_idx = cpu_to_le16(sess-chap_out_idx);
  2229  fw_ddb_entry-tsid = cpu_to_le16(sess-tsid);

Theoretically these should have been caught by Sparse:
http://lwn.net/Articles/205624/

But unfortunately, Sparse hits an error parsing the
external_hw_config_reg union because it uses bitfields as part of __le32
data.  After you hit a Sparse error then it doesn't bother to print
warnings.  This is arguably a UI problem in Sparse and it took me
forever to figure out why the warnings weren't being printed.  :/

If I changed the external_hw_config_reg to use u32 instead of __le32
then Sparse gives the max number of warnings.  I'm not sure that that's
the right thing to do.  Are those bitfields actually used?  Maybe we
should just delete it.

Anyway, I've attached the warnings below.

regards,
dan carpenter

stdin:1223:2: warning: #warning syscall finit_module not implemented [-Wcpp]
devel/drivers/scsi/qla4xxx/ql4_os.c:1370:28: warning: incorrect type in 
assignment (different base types)
devel/drivers/scsi/qla4xxx/ql4_os.c:1370:28:expected unsigned int 
[unsigned] [usertype] cookie
devel/drivers/scsi/qla4xxx/ql4_os.c:1370:28:got restricted __le32 
[usertype] noident
devel/drivers/scsi/qla4xxx/ql4_os.c:1890:21: warning: incorrect type in 
assignment (different base types)
devel/drivers/scsi/qla4xxx/ql4_os.c:1890:21:expected restricted itt_t 
[usertype] itt
devel/drivers/scsi/qla4xxx/ql4_os.c:1890:21:got unsigned int [unsigned] 
[usertype] handle
devel/drivers/scsi/qla4xxx/ql4_os.c:901:32: warning: cast to restricted __le64
devel/drivers/scsi/qla4xxx/ql4_os.c:902:32: warning: cast to restricted __le64
devel/drivers/scsi/qla4xxx/ql4_os.c:904:29: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:905:31: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:906:30: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:907:29: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:908:28: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:909:31: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:910:30: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:911:29: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:913:29: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:914:31: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:915:30: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:916:31: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:917:30: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:919:25: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:920:27: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:921:29: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:922:27: warning: cast to restricted __le32
devel/drivers/scsi/qla4xxx/ql4_os.c:502:21: warning: restricted __le16 degrades 
to integer
devel/drivers/scsi/qla4xxx/ql4_os.c:627:9: warning: cast to restricted __le16
devel/drivers/scsi/qla4xxx/ql4_os.c:630:13: warning: cast to restricted __le16
devel/drivers/scsi/qla4xxx/ql4_os.c:635:28: warning: incorrect type in 
assignment (different base types)
devel/drivers/scsi/qla4xxx/ql4_os.c:635:28:expected unsigned short 
[unsigned] [usertype] cookie
devel/drivers/scsi/qla4xxx/ql4_os.c:635:28:got restricted __le16 [usertype] 
noident
devel/drivers/scsi/qla4xxx/ql4_os.c:1126:53: warning: invalid assignment: =
devel/drivers/scsi/qla4xxx/ql4_os.c:1126:53:left side has type unsigned 
short
devel/drivers/scsi/qla4xxx/ql4_os.c:1126:53:right side has type restricted 
__le16

re: [SCSI] qla2xxx: Enhancements to support ISPFx00.

[Dan Carpenter]

Hello Giridhar Malavali,

The patch 8ae6d9c7eb10: [SCSI] qla2xxx: Enhancements to support 
ISPFx00. from Mar 28, 2013, leads to a bunch of Sparse endian warnings.

See: http://lwn.net/Articles/205624/

Most of them are just because the code hasn't been annotated but some
look like real bugs like on line 3288.

I've included the warnings below.

regards,
dan carpenter

stdin:1223:2: warning: #warning syscall finit_module not implemented [-Wcpp]
drivers/scsi/qla2xxx/qla_mr.c:55:21: warning: restricted pci_channel_state_t 
degrades to integer
drivers/scsi/qla2xxx/qla_mr.c:55:37: warning: restricted pci_channel_state_t 
degrades to integer
drivers/scsi/qla2xxx/qla_mr.c:643:35: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:643:35:expected unsigned short [unsigned] 
[usertype] request_q_outpointer
drivers/scsi/qla2xxx/qla_mr.c:643:35:got restricted __le16 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:644:35: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:644:35:expected unsigned short [unsigned] 
[usertype] response_q_inpointer
drivers/scsi/qla2xxx/qla_mr.c:644:35:got restricted __le16 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:645:31: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:645:31:expected unsigned short [unsigned] 
[usertype] request_q_length
drivers/scsi/qla2xxx/qla_mr.c:645:31:got restricted __le16 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:646:32: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:646:32:expected unsigned short [unsigned] 
[usertype] response_q_length
drivers/scsi/qla2xxx/qla_mr.c:646:32:got restricted __le16 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:647:35: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:647:35:expected unsigned int [unsigned] 
noident
drivers/scsi/qla2xxx/qla_mr.c:647:35:got restricted __le32 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:648:35: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:648:35:expected unsigned int [unsigned] 
noident
drivers/scsi/qla2xxx/qla_mr.c:648:35:got restricted __le32 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:649:36: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:649:36:expected unsigned int [unsigned] 
noident
drivers/scsi/qla2xxx/qla_mr.c:649:36:got restricted __le32 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:650:36: warning: incorrect type in assignment 
(different base types)
drivers/scsi/qla2xxx/qla_mr.c:650:36:expected unsigned int [unsigned] 
noident
drivers/scsi/qla2xxx/qla_mr.c:650:36:got restricted __le32 [usertype] 
noident
drivers/scsi/qla2xxx/qla_mr.c:883:22: warning: cast removes address space of 
expression
drivers/scsi/qla2xxx/qla_mr.c:898:22: warning: cast removes address space of 
expression
drivers/scsi/qla2xxx/qla_mr.c:1424:17: warning: incorrect type in argument 2 
(different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:1424:17:expected void volatile [noderef] 
asn:2*addr
drivers/scsi/qla2xxx/qla_mr.c:1424:17:got unsigned int *noident
drivers/scsi/qla2xxx/qla_mr.c:2200:34: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c::49: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2223:47: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2224:45: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2227:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2298:23: warning: cast to restricted __le16
drivers/scsi/qla2xxx/qla_mr.c:2299:23: warning: cast to restricted __le16
drivers/scsi/qla2xxx/qla_mr.c:2353:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2355:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2357:32: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2652:26: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2720:16: warning: incorrect type in argument 1 
(different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2720:16:expected void const volatile 
[noderef] asn:2*addr
drivers/scsi/qla2xxx/qla_mr.c:2720:16:got unsigned int *noident
drivers/scsi/qla2xxx/qla_mr.c:2723:45: warning: incorrect type in argument 2 
(different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2723:45:expected void const volatile 
[noderef] asn:2*src
drivers/scsi/qla2xxx/qla_mr.c:2723:45:got struct response_t [usertype] 
*[assigned] lptr
drivers/scsi/qla2xxx/qla_mr.c:2774:17: warning: incorrect type in argument 2 
(different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2774:17:expected void volatile [noderef] 
asn:2*addr
drivers/scsi/qla2xxx/qla_mr.c:2774:17:got unsigned int *noident
drivers/scsi/qla2xxx/qla_mr.c:3152:29: warning: incorrect 

Re: [SCSI] qla2xxx: Enhancements to support ISPFx00.

[Giridhar Malavali]

Dan,

Thanks for bringing this to our attention. We will look into this and take
care of this. 

-- Giri

On 4/15/13 1:34 PM, Dan Carpenter dan.carpen...@oracle.com wrote:

Hello Giridhar Malavali,

The patch 8ae6d9c7eb10: [SCSI] qla2xxx: Enhancements to support
ISPFx00. from Mar 28, 2013, leads to a bunch of Sparse endian warnings.

See: http://lwn.net/Articles/205624/

Most of them are just because the code hasn't been annotated but some
look like real bugs like on line 3288.

I've included the warnings below.

regards,
dan carpenter

stdin:1223:2: warning: #warning syscall finit_module not implemented
[-Wcpp]
drivers/scsi/qla2xxx/qla_mr.c:55:21: warning: restricted
pci_channel_state_t degrades to integer
drivers/scsi/qla2xxx/qla_mr.c:55:37: warning: restricted
pci_channel_state_t degrades to integer
drivers/scsi/qla2xxx/qla_mr.c:643:35: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:643:35:expected unsigned short
[unsigned] [usertype] request_q_outpointer
drivers/scsi/qla2xxx/qla_mr.c:643:35:got restricted __le16 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:644:35: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:644:35:expected unsigned short
[unsigned] [usertype] response_q_inpointer
drivers/scsi/qla2xxx/qla_mr.c:644:35:got restricted __le16 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:645:31: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:645:31:expected unsigned short
[unsigned] [usertype] request_q_length
drivers/scsi/qla2xxx/qla_mr.c:645:31:got restricted __le16 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:646:32: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:646:32:expected unsigned short
[unsigned] [usertype] response_q_length
drivers/scsi/qla2xxx/qla_mr.c:646:32:got restricted __le16 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:647:35: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:647:35:expected unsigned int [unsigned]
noident
drivers/scsi/qla2xxx/qla_mr.c:647:35:got restricted __le32 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:648:35: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:648:35:expected unsigned int [unsigned]
noident
drivers/scsi/qla2xxx/qla_mr.c:648:35:got restricted __le32 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:649:36: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:649:36:expected unsigned int [unsigned]
noident
drivers/scsi/qla2xxx/qla_mr.c:649:36:got restricted __le32 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:650:36: warning: incorrect type in
assignment (different base types)
drivers/scsi/qla2xxx/qla_mr.c:650:36:expected unsigned int [unsigned]
noident
drivers/scsi/qla2xxx/qla_mr.c:650:36:got restricted __le32 [usertype]
noident
drivers/scsi/qla2xxx/qla_mr.c:883:22: warning: cast removes address space
of expression
drivers/scsi/qla2xxx/qla_mr.c:898:22: warning: cast removes address space
of expression
drivers/scsi/qla2xxx/qla_mr.c:1424:17: warning: incorrect type in
argument 2 (different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:1424:17:expected void volatile
[noderef] asn:2*addr
drivers/scsi/qla2xxx/qla_mr.c:1424:17:got unsigned int *noident
drivers/scsi/qla2xxx/qla_mr.c:2200:34: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c::49: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2223:47: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2224:45: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2227:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2298:23: warning: cast to restricted __le16
drivers/scsi/qla2xxx/qla_mr.c:2299:23: warning: cast to restricted __le16
drivers/scsi/qla2xxx/qla_mr.c:2353:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2355:29: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2357:32: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2652:26: warning: cast to restricted __le32
drivers/scsi/qla2xxx/qla_mr.c:2720:16: warning: incorrect type in
argument 1 (different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2720:16:expected void const volatile
[noderef] asn:2*addr
drivers/scsi/qla2xxx/qla_mr.c:2720:16:got unsigned int *noident
drivers/scsi/qla2xxx/qla_mr.c:2723:45: warning: incorrect type in
argument 2 (different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2723:45:expected void const volatile
[noderef] asn:2*src
drivers/scsi/qla2xxx/qla_mr.c:2723:45:got struct response_t
[usertype] *[assigned] lptr
drivers/scsi/qla2xxx/qla_mr.c:2774:17: warning: incorrect type in
argument 2 (different address spaces)
drivers/scsi/qla2xxx/qla_mr.c:2774:17:expected void volatile
[noderef] 

Re: [PATCH 0/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[James Bottomley]

On Mon, 2013-04-15 at 13:39 -0500, wenxi...@linux.vnet.ibm.com wrote:
 In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd
 when sending commands and retry for a bit if the driver returns a
 busy response.

This is already handled by the timeout, I think.  If a driver
continuously returns MLQUEUE BUSY, then we'll fail the request after the
timeout on the command expires.

James


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[Brian King]

On 04/15/2013 03:45 PM, James Bottomley wrote:
 On Mon, 2013-04-15 at 13:39 -0500, wenxi...@linux.vnet.ibm.com wrote:
 In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd
 when sending commands and retry for a bit if the driver returns a
 busy response.
 
 This is already handled by the timeout, I think.  If a driver
 continuously returns MLQUEUE BUSY, then we'll fail the request after the
 timeout on the command expires.

If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the
abort works, we return FAILED out of scsi_send_eh_cmnd, which results in
no retry being performed, since scsi_eh_tur only retries once and
only if NEEDS_RETRY is returned. Or am I missing something?

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[James Bottomley]

On Mon, 2013-04-15 at 16:55 -0500, Brian King wrote:
 On 04/15/2013 03:45 PM, James Bottomley wrote:
  On Mon, 2013-04-15 at 13:39 -0500, wenxi...@linux.vnet.ibm.com wrote:
  In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd
  when sending commands and retry for a bit if the driver returns a
  busy response.
  
  This is already handled by the timeout, I think.  If a driver
  continuously returns MLQUEUE BUSY, then we'll fail the request after the
  timeout on the command expires.
 
 If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the
 abort works, we return FAILED out of scsi_send_eh_cmnd, which results in
 no retry being performed, since scsi_eh_tur only retries once and
 only if NEEDS_RETRY is returned. Or am I missing something?

Sorry, I'm not being clear.  It comes with being at a conference.  What
I mean is that if you do this, the criterion for success or failure
should be the amount of time left not the number of retries.  This is
what the non-eh submission path also does for retries of events that
don't count against the retry limit ... so something like this patch
(uncompiled and untested #include stddisclaimer.h)

James


diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..93ab4f4 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -793,6 +793,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, 
unsigned char *cmnd,
DECLARE_COMPLETION_ONSTACK(done);
unsigned long timeleft;
struct scsi_eh_save ses;
+   const int stall_for = min(HZ/10,1); /* 100 ms */
int rtn;
 
scsi_eh_prep_cmnd(scmd, ses, cmnd, cmnd_size, sense_bytes);
@@ -802,6 +803,8 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, 
unsigned char *cmnd,
scmd-scsi_done = scsi_eh_done;
shost-hostt-queuecommand(shost, scmd);
 
+  retry:
+
timeleft = wait_for_completion_timeout(done, timeout);
 
shost-eh_action = NULL;
@@ -831,8 +834,12 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, 
unsigned char *cmnd,
case TARGET_ERROR:
break;
case ADD_TO_MLQUEUE:
-   rtn = NEEDS_RETRY;
-   break;
+   if (timeleft  stall_for) {
+   timeout = timeleft - stall_for;
+   msleep(stall_for);
+   goto retry;
+   }
+   /* fall through */
default:
rtn = FAILED;
break;



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/1] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[wenxiong]

Quoting James Bottomley james.bottom...@hansenpartnership.com:


On Mon, 2013-04-15 at 16:55 -0500, Brian King wrote:

On 04/15/2013 03:45 PM, James Bottomley wrote:
 On Mon, 2013-04-15 at 13:39 -0500, wenxi...@linux.vnet.ibm.com wrote:
 In scsi_send_eh_cmnd(), this fix will check the return code of  
queuecomamnd

 when sending commands and retry for a bit if the driver returns a
 busy response.

 This is already handled by the timeout, I think.  If a driver
 continuously returns MLQUEUE BUSY, then we'll fail the request after the
 timeout on the command expires.

If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the
abort works, we return FAILED out of scsi_send_eh_cmnd, which results in
no retry being performed, since scsi_eh_tur only retries once and
only if NEEDS_RETRY is returned. Or am I missing something?


Sorry, I'm not being clear.  It comes with being at a conference.  What
I mean is that if you do this, the criterion for success or faiure
should be the amount of time left not the number of retries.  This is
what the non-eh submission path also does for retries of events that
don't count against the retry limit ... so something like this patch
(uncompiled and untested #include stddisclaimer.h)

James


Hi James,

The failing case for us is: Doesn't matter what timeout value we set in
wait_for_completion_timeout(), it always returns with timeleft = 0.
For example, if I set timeout = 50 secs, wait_for_completion_timeout()
always returns with timeleft =0(even adapter is already in good shape in
20 secs).  We never gets a chance to call into if (timeleft) leg.

My understanding is: if shost-host-queuecommand() failed with MLQUEUE busy
response at the first time, wait_for_completion_timeout() always wakes  
up by expired.


Here is log when I enabled scsi log:

Apr 15 18:44:35 ltcsatiocp5 kernel: scsi_send_eh_cmnd: scmd:  
c000f88bc980, timeleft: 0


I applied your patch. Because timeleft is always zero, never got a  
chance to call into

if(timeleft) { leg.

Thanks,
Wendy

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html