Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On 09/07/2014 06:24 PM, Christoph Hellwig wrote: Looks like you put a version into the SuSE tree with a blacklist entry just for the Fujitsu array that requires the TEST UNIT READY. Can you send that version upstream as well? I've just had reports that this is required, as it interferes with NetApp arrays. So I'll be reposting a combined series with that. Cheers, Hannes -- Dr. Hannes Reinecke zSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- 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 10/24] uas: zap_pending: data urbs should have completed at this time
Hi, On 09/13/2014 09:31 PM, Sergei Shtylyov wrote: Hello. On 9/13/2014 1:26 PM, Hans de Goede wrote: The data urbs are all killed before calling zap_pending, and their completion handler should have cleared their inflight flag. Do not 0 the data inflight flags, and add a check for try_complete succeeding, as it should always succeed when called from zap_pending. Signed-off-by: Hans de Goede hdego...@redhat.com --- drivers/usb/storage/uas.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08edb6b..85bbc1d 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result) struct uas_cmd_info *cmdinfo; struct uas_cmd_info *temp; unsigned long flags; +int err; Er, I don't see why this variable is necessary. [...] @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result) struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp); uas_log_cmd_state(cmnd, __func__); -/* all urbs are killed, clear inflight bits */ -cmdinfo-state = ~(COMMAND_INFLIGHT | -DATA_IN_URB_INFLIGHT | -DATA_OUT_URB_INFLIGHT); +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */ +cmdinfo-state = ~COMMAND_INFLIGHT; cmnd-result = result 16; -uas_try_complete(cmnd, __func__); +err = uas_try_complete(cmnd, __func__); +WARN_ON(err != 0); Why not: WARN_ON(uas_try_complete(cmnd, __func__)); This was discussed already during v1 of this patch-set, WARN_ON may not have a side-effect, as it may be defined as an empty macro. Regards, Hans -- 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] ahci_xgene: Fix the error print invalid resource for APM X-Gene SoC AHCI SATA Host Controller driver.
Hello, On Sun, Sep 14, 2014 at 11:36:51AM +0530, Suman Tripathi wrote: We can maintain same piece (IS_ERR(ctx-csr_mux)), then we can do the below instead of NULL ?? ctx-csr_mux = res ? devm_ioremap_resource(dev, res) : ERR_PTR(-EINVAL); Setting it to NULL on failure would probably make more sense. No need to carry around ERR_PTR() value around. Thanks. -- tejun -- 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 fix for 3.17 v2] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands
Hi, On 09/13/2014 10:27 PM, Sergei Shtylyov wrote: Hello. On 9/13/2014 10:21 PM, Thomas Backlund wrote: And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one seems to hang upon receiving an ATA_12 or ATA_16 command. https://bugzilla.kernel.org/show_bug.cgi?id=79511 While at it also add missing documentation for the u value for usb-storage quirks. Cc: sta...@vger.kernel.org # 3.16 Signed-off-by: Hans de Goede hdego...@redhat.com -- Changes in v2: Add documentation for new t and u usb-storage.quirks flags --- Documentation/kernel-parameters.txt | 3 +++ drivers/usb/storage/uas.c | 13 + drivers/usb/storage/unusual_uas.h | 16 ++-- drivers/usb/storage/usb.c | 6 +- include/linux/usb_usual.h | 2 ++ 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 5ae8608..7c32053 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted. bogus residue values); s = SINGLE_LUN (the device has only one Logical Unit); +t = NO_ATA_1X (don't allow ATA12 and ATA12 +commands, uas only); I guess you meant ATA12 and *ATA16* ... or even ATA(12) and ATA(16). As far as I remember SCSI, it has CDB length in parens for the command names. Right, ugh. v3 coming up ... Thanks, Hans -- 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: fix regression that accidentally disabled block-based tcq
Hi, On 09/13/2014 07:50 PM, Christoph Hellwig wrote: On Sat, Sep 13, 2014 at 12:28:41PM +0200, Hans de Goede wrote: Yes this one does the trick and fixes things. Note the git tree I used for testing also had your previous fix to split up the blk_tcq union in 2 separate struct members. Let me know if you want me to re-test without that fix. I'm pretty sure that isn't needed, so double testing it indeed doesn't would be helpful. I can confirm that things still work fine with the union left in place. I'd like to add your Tested-by: tag after that, too. Ack: Tested-by: Hans de Goede hdego...@redhat.com Regards, Hans -- 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 fix for 3.17 v3] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands
And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.
https://bugzilla.kernel.org/show_bug.cgi?id=79511
While at it also add missing documentation for the u value for usb-storage
quirks.
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
---
Documentation/kernel-parameters.txt | 3 +++
drivers/usb/storage/uas.c | 13 +
drivers/usb/storage/unusual_uas.h | 16 ++--
drivers/usb/storage/usb.c | 6 +-
include/linux/usb_usual.h | 2 ++
5 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 5ae8608..ec6b25a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be
entirely omitted.
bogus residue values);
s = SINGLE_LUN (the device has only one
Logical Unit);
+ t = NO_ATA_1X (don't allow ATA(12) and ATA(16)
+ commands, uas only);
+ u = IGNORE_UAS (do not try to use uas);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
Example: quirks=0419:aaf5:rl,0421:0433:rc
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
#include scsi/scsi_tcq.h
#include uas-detect.h
+#include scsiglue.h
/*
* The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+ unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
BUILD_BUG_ON(sizeof(struct uas_cmd_info) sizeof(struct scsi_pointer));
+ if ((devinfo-flags US_FL_NO_ATA_1X)
+ (cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
+ memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
+ sizeof(usb_stor_sense_invalidCDB));
+ cmnd-result = SAM_STAT_CHECK_CONDITION;
+ cmnd-scsi_done(cmnd);
+ return 0;
+ }
+
spin_lock_irqsave(devinfo-lock, flags);
if (devinfo-resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const
struct usb_device_id *id)
devinfo-resetting = 0;
devinfo-running_task = 0;
devinfo-shutdown = 0;
+ devinfo-flags = id-driver_info;
+ usb_stor_adjust_quirks(udev, devinfo-flags);
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h
b/drivers/usb/storage/unusual_uas.h
index 724..52f7012 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,9 @@
* and don't forget to CC: the USB development list linux-...@vger.kernel.org
*/
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV( 0xabcd, 0x1234, 0x0100, 0x0100,
- * Example,
- * Storage with broken UAS,
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+ Seagate,
+ Expansion Desk,
+ USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+ US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev,
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE |
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
- US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE);
+ US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
+ US_FL_NO_ATA_1X);
p = quirks;
while (*p) {
@@ -543,6 +544,9
Re: [PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time
On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
Hi,
On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
Hello.
On 9/13/2014 1:26 PM, Hans de Goede wrote:
The data urbs are all killed before calling zap_pending, and their
completion
handler should have cleared their inflight flag.
Do not 0 the data inflight flags, and add a check for try_complete
succeeding,
as it should always succeed when called from zap_pending.
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/storage/uas.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08edb6b..85bbc1d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
unsigned long flags;
+int err;
Er, I don't see why this variable is necessary.
[...]
@@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
uas_log_cmd_state(cmnd, __func__);
-/* all urbs are killed, clear inflight bits */
-cmdinfo-state = ~(COMMAND_INFLIGHT |
-DATA_IN_URB_INFLIGHT |
-DATA_OUT_URB_INFLIGHT);
+/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
+cmdinfo-state = ~COMMAND_INFLIGHT;
cmnd-result = result 16;
-uas_try_complete(cmnd, __func__);
+err = uas_try_complete(cmnd, __func__);
+WARN_ON(err != 0);
Why not:
WARN_ON(uas_try_complete(cmnd, __func__));
This was discussed already during v1 of this patch-set, WARN_ON may
not have a side-effect, as it may be defined as an empty macro.
Must have missed the discussion, but whoever said that loses all their
review points. We're very careful to make sure that even in the case
where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
side effects are still accounted for. This is the canonical definition
of WARN_ON in the compiled out case:
#define WARN_ON(condition) ({ \
int __ret_warn_on = !!(condition); \
unlikely(__ret_warn_on);\
})
So the compiler will eliminate the statement only if there are no side
effects.
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 v2 10/24] uas: zap_pending: data urbs should have completed at this time
Hi,
On 09/14/2014 12:29 PM, James Bottomley wrote:
On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
Hi,
On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
Hello.
On 9/13/2014 1:26 PM, Hans de Goede wrote:
The data urbs are all killed before calling zap_pending, and their
completion
handler should have cleared their inflight flag.
Do not 0 the data inflight flags, and add a check for try_complete
succeeding,
as it should always succeed when called from zap_pending.
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/storage/uas.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08edb6b..85bbc1d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
unsigned long flags;
+int err;
Er, I don't see why this variable is necessary.
[...]
@@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
uas_log_cmd_state(cmnd, __func__);
-/* all urbs are killed, clear inflight bits */
-cmdinfo-state = ~(COMMAND_INFLIGHT |
-DATA_IN_URB_INFLIGHT |
-DATA_OUT_URB_INFLIGHT);
+/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
+cmdinfo-state = ~COMMAND_INFLIGHT;
cmnd-result = result 16;
-uas_try_complete(cmnd, __func__);
+err = uas_try_complete(cmnd, __func__);
+WARN_ON(err != 0);
Why not:
WARN_ON(uas_try_complete(cmnd, __func__));
This was discussed already during v1 of this patch-set, WARN_ON may
not have a side-effect, as it may be defined as an empty macro.
Must have missed the discussion, but whoever said that loses all their
review points. We're very careful to make sure that even in the case
where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
side effects are still accounted for. This is the canonical definition
of WARN_ON in the compiled out case:
#define WARN_ON(condition) ({ \
int __ret_warn_on = !!(condition); \
unlikely(__ret_warn_on);\
})
So the compiler will eliminate the statement only if there are no side
effects.
Ah that is good to know. Still I would like to stick with the new version
(which adds the err), as I believe that that code is more readable.
AFAIK in general the kernel coding style is to favor:
err = func();
if (err)
over:
if (func())
And this is sorta the same.
Regards,
Hans
--
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 20/20] scsi_error: format abort error message
On 09/13/2014 03:07 AM, Elliott, Robert (Server Storage) wrote:
-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Christoph Hellwig
Sent: Friday, 05 September, 2014 7:33 PM
Subject: Re: [PATCH 20/20] scsi_error: format abort error message
On Wed, Sep 03, 2014 at 12:06:15PM +0200, Hannes Reinecke wrote:
...
Decode the return value if the command abort failed.
@@ -157,8 +157,8 @@ scmd_eh_abort_handler(struct work_struct *work)
} else {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_INFO, scmd,
- scmd %p abort failed, rtn %d\n,
- scmd, rtn));
+ scmd %p abort failed, rtn %s\n,
+ scmd, scsi_retval_string(rtn)));
...
I've not seen an answer to my question in reply to the previous version
of this. Why would you do pretty printing of a variable that can just
return SUCCESS or FAILURE?
Is there anything prohibiting the scsi_eh_abort_handler provided by
the LLD from returning any of the other values like NEEDS_RETRY?
#define NEEDS_RETRY 0x2001
#define SUCCESS 0x2002
#define FAILED 0x2003
#define QUEUED 0x2004
#define SOFT_ERROR 0x2005
#define ADD_TO_MLQUEUE 0x2006
#define TIMEOUT_ERROR 0x2007
#define SCSI_RETURN_NOT_HANDLED 0x2008
#define FAST_IO_FAIL0x2009
There is not, and hence I've added this patch.
Although the intention here would be that we should be
enforcing it, either by stating it in the description
of the function or programmatically.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 20/20] scsi_error: format abort error message
On Sat, Sep 13, 2014 at 01:07:11AM +, Elliott, Robert (Server Storage) wrote: I've not seen an answer to my question in reply to the previous version of this. Why would you do pretty printing of a variable that can just return SUCCESS or FAILURE? Is there anything prohibiting the scsi_eh_abort_handler provided by the LLD from returning any of the other values like NEEDS_RETRY? Right now there is nothing prohibiting them from returning any possible value, but we only handle SUCCESS specially and treat everything else as FAILED. If anyone is motivated enough to make this cleaner we should add a WARN_ON for any other value as it's a driver bug. -- 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: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
I have revamped the code so it becomes both more effective and far more clear.
Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
---
drivers/scsi/scsi_devinfo.c | 31 +++
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 49014a1..f6752cc 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -282,27 +282,18 @@ static struct scsi_dev_info_list_table
*scsi_devinfo_lookup_by_key(int key)
static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
char *from, int compatible)
{
- size_t from_length;
-
- from_length = strlen(from);
- strncpy(to, from, min(to_length, from_length));
- if (from_length to_length) {
- if (compatible) {
- /*
-* NUL terminate the string if it is short.
-*/
- to[from_length] = '\0';
- } else {
- /*
-* space pad the string if it is short.
-*/
- strncpy(to[from_length], spaces,
- to_length - from_length);
- }
- }
- if (from_length to_length)
-printk(KERN_WARNING %s: %s string '%s' is too long\n,
+ strncpy(to, from, to_length);
+ if (to[to_length - 1] != '\0') {
+ to[to_length - 1] = '\0';
+ printk(KERN_WARNING %s: %s string '%s' is too long\n,
__func__, name, from);
+ }
+ if (!compatible) {
+ /*
+* space pad the string if it is short.
+*/
+ strlcat(to, spaces, to_length);
+ }
}
/**
--
1.7.10.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] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
2014-09-14 23:34 GMT+02:00 Elliott, Robert (Server Storage) elli...@hp.com:
-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Rickard Strandqvist
...
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
...
static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
char *from, int compatible)
{
- size_t from_length;
-
- from_length = strlen(from);
- strncpy(to, from, min(to_length, from_length));
- if (from_length to_length) {
- if (compatible) {
- /*
- * NUL terminate the string if it is short.
- */
- to[from_length] = '\0';
- } else {
- /*
- * space pad the string if it is short.
- */
- strncpy(to[from_length], spaces,
- to_length - from_length);
- }
- }
- if (from_length to_length)
- printk(KERN_WARNING %s: %s string '%s' is too long\n,
+ strncpy(to, from, to_length);
+ if (to[to_length - 1] != '\0') {
+ to[to_length - 1] = '\0';
+ printk(KERN_WARNING %s: %s string '%s' is too long\n,
__func__, name, from);
+ }
The caller of this function, scsi_dev_info_list_add_keyed, created
the to destination buffer, devinfo, with kmalloc, so it's not
guaranteed to be full of zeros.
If from_length is shorter than to_length, then this code will
be inspecting an uninitialized character that strncpy didn't
touch.
---
Rob ElliottHP Server Storage
Hi Elliott
How do you mean?
strncpy zeroes throughout the remainder of the string from until the
length off to_length, or otherwise guaranteed trailing zero characters
and a warning is printed.
Is not it exactly the functionality that is desired?
Kind regards
Rickard Strandqvist
--
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: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
-Original Message- From: Rickard Strandqvist [mailto:rickard_strandqv...@spectrumdigital.se] How do you mean? strncpy zeroes throughout the remainder of the string from until the length off to_length, or otherwise guaranteed trailing zero characters and a warning is printed. Is not it exactly the functionality that is desired? Ah, I see that in man 3 strcpy: If the length of src is less than n, strncpy() pads the remainder of dest with null bytes. I agree that should work. --- Rob ElliottHP Server Storage
Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
On 09/12/2014 09:29 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
This patch is to modify previous patch 13/17 and it is relative to
http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
change in v4:
1. for readability, rename firstindex to getIndex, rename lastindex to
putIndex
For some reason, the names head+tail areusual for a circular buffer.
But let us ignore the names, I don't care.
2. define ARCMSR_API_DATA_BUFLEN as 1032
3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
It's definitely better when you post renames and other non-functional changes
in separate
patches, it's easier for the reviewer.
Signed-off-by: Ching Huang ching2...@areca.com.tw
---
diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c
b/drivers/scsi/arcmsr/arcmsr_attr.c
--- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000
+0800
@@ -50,6 +50,7 @@
#include linux/errno.h
#include linux/delay.h
#include linux/pci.h
+#include linux/circ_buf.h
#include scsi/scsi_cmnd.h
#include scsi/scsi_device.h
@@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
struct device *dev = container_of(kobj,struct device,kobj);
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
- uint8_t *pQbuffer,*ptmpQbuffer;
+ uint8_t *ptmpQbuffer;
int32_t allxfer_len = 0;
unsigned long flags;
@@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
spin_lock_irqsave(acb-rqbuffer_lock, flags);
- if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
- pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- if (acb-rqbuf_firstindex acb-rqbuf_lastindex) {
- if ((ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex) =
1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb-rqbuf_firstindex += 1032;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- allxfer_len = 1032;
- } else {
- if (((ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)
- + acb-rqbuf_lastindex) 1032) {
- memcpy(ptmpQbuffer, pQbuffer,
- ARCMSR_MAX_QBUFFER
- - acb-rqbuf_firstindex);
- ptmpQbuffer += ARCMSR_MAX_QBUFFER
- - acb-rqbuf_firstindex;
- memcpy(ptmpQbuffer, acb-rqbuffer, 1032
- - (ARCMSR_MAX_QBUFFER -
- acb-rqbuf_firstindex));
- acb-rqbuf_firstindex = 1032 -
- (ARCMSR_MAX_QBUFFER -
- acb-rqbuf_firstindex);
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer, pQbuffer,
- ARCMSR_MAX_QBUFFER -
- acb-rqbuf_firstindex);
- ptmpQbuffer += ARCMSR_MAX_QBUFFER -
- acb-rqbuf_firstindex;
- memcpy(ptmpQbuffer, acb-rqbuffer,
- acb-rqbuf_lastindex);
- allxfer_len = ARCMSR_MAX_QBUFFER -
- acb-rqbuf_firstindex +
- acb-rqbuf_lastindex;
- acb-rqbuf_firstindex =
- acb-rqbuf_lastindex;
- }
- }
- } else {
- if ((acb-rqbuf_lastindex - acb-rqbuf_firstindex)
1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb-rqbuf_firstindex += 1032;
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer, pQbuffer,
acb-rqbuf_lastindex
- - acb-rqbuf_firstindex);
- allxfer_len = acb-rqbuf_lastindex -
- acb-rqbuf_firstindex;
- acb-rqbuf_firstindex = acb-rqbuf_lastindex;
- }
+ if
Re: [PATCH v4 2/2] arcmsr: simplify of updating doneq_index and postq_index
On Fri, 2014-09-12 at 16:05 +0200, Tomas Henzl wrote:
On 09/12/2014 10:22 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
This patch is to modify previous patch 16/17 and it is relative to
http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
change in v4:
1. clean up of duplicate variable declaration in switch.
2. simplify of updating doneq_index and postq_index
3. fix spin_lock area in arcmsr_hbaD_polling_ccbdone
The intention of the doneq_lock is to protect the pmu-doneq_index and the
associated buffer,
right? Why is the spinlock not used on other places accessing the doneq_index
like arcmsr_done4abort_postqueue or arcmsr_hbaD_postqueue_isr ?
In fact, in original code arcmsr_hbaD_postqueue_isr has spinlock with a larger
area.
As to arcmsr_done4abort_postqueue, it should be protected as below.
@@ -1182,35 +1178,25 @@ static void arcmsr_done4abort_postqueue(
break;
case ACB_ADAPTER_TYPE_D: {
struct MessageUnit_D *pmu = acb-pmuD;
- uint32_t ccb_cdb_phy, outbound_write_pointer;
- uint32_t doneq_index, index_stripped, addressLow, residual;
- bool error;
- struct CommandControlBlock *pCCB;
+ uint32_t outbound_write_pointer;
+ uint32_t doneq_index, index_stripped, addressLow, residual,
toggle;
+ unsigned long flags;
- outbound_write_pointer = pmu-done_qbuffer[0].addressLow + 1;
- doneq_index = pmu-doneq_index;
residual = atomic_read(acb-ccboutstandingcount);
for (i = 0; i residual; i++) {
- while ((doneq_index 0xFFF) !=
+ spin_lock_irqsave(acb-doneq_lock, flags);
+ outbound_write_pointer =
+ pmu-done_qbuffer[0].addressLow + 1;
+ doneq_index = pmu-doneq_index;
+ if ((doneq_index 0xFFF) !=
(outbound_write_pointer 0xFFF)) {
- if (doneq_index 0x4000) {
- index_stripped = doneq_index 0xFFF;
- index_stripped += 1;
- index_stripped %=
- ARCMSR_MAX_ARC1214_DONEQUEUE;
- pmu-doneq_index = index_stripped ?
- (index_stripped | 0x4000) :
- (index_stripped + 1);
- } else {
- index_stripped = doneq_index;
- index_stripped += 1;
- index_stripped %=
- ARCMSR_MAX_ARC1214_DONEQUEUE;
- pmu-doneq_index = index_stripped ?
- index_stripped :
- ((index_stripped | 0x4000) + 1);
- }
+ toggle = doneq_index 0x4000;
+ index_stripped = (doneq_index 0xFFF) + 1;
+ index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE;
+ pmu-doneq_index = index_stripped ?
(index_stripped | toggle) :
+ ((toggle ^ 0x4000) + 1);
doneq_index = pmu-doneq_index;
+ spin_unlock_irqrestore(acb-doneq_lock, flags);
addressLow = pmu-done_qbuffer[doneq_index
0xFFF].addressLow;
ccb_cdb_phy = (addressLow 0xFFF0);
@@ -1224,11 +1210,10 @@ static void arcmsr_done4abort_postqueue(
arcmsr_drain_donequeue(acb, pCCB, error);
writel(doneq_index,
pmu-outboundlist_read_pointer);
+ } else {
+ spin_unlock_irqrestore(acb-doneq_lock, flags);
+ mdelay(10);
}
- mdelay(10);
- outbound_write_pointer =
- pmu-done_qbuffer[0].addressLow + 1;
- doneq_index = pmu-doneq_index;
}
pmu-postq_index = 0;
pmu-doneq_index = 0x40FF;
And in arcmsr_hbaD_polling_ccbdone the code looks so:
spin_unlock_irqrestore(acb-doneq_lock, flags);
doneq_index = pmu-doneq_index;
flag_ccb = pmu-done_qbuffer[doneq_index 0xFFF].addressLow;
you unlock^ and after that is the value read and used
[PATCH] xen: make pvscsi frontend dependant on xenbus frontend
The pvscsi frontend driver requires the xenbus frontend driver. Reflect this in Kconfig. Signed-off-by: Juergen Gross jgr...@suse.com --- drivers/scsi/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index 9130df1..ff62dc1 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -579,6 +579,7 @@ config VMWARE_PVSCSI config XEN_SCSI_FRONTEND tristate XEN SCSI frontend driver depends on SCSI XEN + select XEN_XENBUS_FRONTEND help The XEN SCSI frontend driver allows the kernel to access SCSI Devices within another guest OS (usually Dom0). -- 1.8.4.5 -- 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
