Re: [PATCH v2] scsi: qla4xxx: Use dma_pool_zalloc
Hi Nilesh,
On Thu, Dec 22, 2016 at 5:42 PM, Souptick Joarder wrote:
> We should use dma_pool_zalloc instead of dma_pool_alloc/memset
>
> Signed-off-by: Souptick joarder
> ---
> v2 changes:
>- Address comment from Nilesh to make same change in
> all applicable places inside qla4xxx source
>
> drivers/scsi/qla4xxx/ql4_mbx.c | 6 ++
> drivers/scsi/qla4xxx/ql4_os.c | 4 +---
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c b/drivers/scsi/qla4xxx/ql4_mbx.c
> index c291fdf..8f97839 100644
> --- a/drivers/scsi/qla4xxx/ql4_mbx.c
> +++ b/drivers/scsi/qla4xxx/ql4_mbx.c
> @@ -1587,12 +1587,11 @@ int qla4xxx_get_chap(struct scsi_qla_host *ha, char
> *username, char *password,
> struct ql4_chap_table *chap_table;
> dma_addr_t chap_dma;
>
> - chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL, &chap_dma);
> + chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
> &chap_dma);
> if (chap_table == NULL)
> return -ENOMEM;
>
> chap_size = sizeof(struct ql4_chap_table);
> - memset(chap_table, 0, chap_size);
>
> if (is_qla40XX(ha))
> offset = FLASH_CHAP_OFFSET | (idx * chap_size);
> @@ -1651,13 +1650,12 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char
> *username, char *password,
> uint32_t chap_size = 0;
> dma_addr_t chap_dma;
>
> - chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL, &chap_dma);
> + chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
> &chap_dma);
> if (chap_table == NULL) {
> ret = -ENOMEM;
> goto exit_set_chap;
> }
>
> - memset(chap_table, 0, sizeof(struct ql4_chap_table));
> if (bidi)
> chap_table->flags |= BIT_6; /* peer */
> else
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index 01c3610..0c91c89 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -825,12 +825,10 @@ static int qla4xxx_delete_chap(struct Scsi_Host *shost,
> uint16_t chap_tbl_idx)
> uint32_t chap_size;
> int ret = 0;
>
> - chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL, &chap_dma);
> + chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
> &chap_dma);
> if (chap_table == NULL)
> return -ENOMEM;
>
> - memset(chap_table, 0, sizeof(struct ql4_chap_table));
> -
> if (is_qla80XX(ha))
> max_chap_entries = (ha->hw.flt_chap_size / 2) /
>sizeof(struct ql4_chap_table);
> --
> 1.9.1
>
Any further comment on this patch ?
Regards
Souptick
--
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
[Bug 191381] LIO ignores XCOPY source and destination descriptor IDs
https://bugzilla.kernel.org/show_bug.cgi?id=191381 --- Comment #1 from David Disseldorp --- I've pushed a reproducer to: https://github.com/ddiss/libiscsi/commit/ae159e147ddf7192ff41e2375c073a10d6c90fb6 Usage is: examples/iscsi-dd --xcopy --src iscsi://192.168.155.101:3260// --dst iscsi://192.168.155.101:3260// iscsi-dd has been modified to issue XCOPY requests with the CSCD List order modified such that the destination (specified via segment descriptor ID) appears before the source. -- You are receiving this mail because: You are the assignee for the 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
Treat as urgent please.
--
Hello,
Sorry to most have border you with this message, but i believe it will
be of good benefit to you. Your name has just being mention by one of
the hoodlum we caught last week trying to claimed the sum of
{$2.20,000.00}. that belongs to one Mrs Veleries Nelton. Please
confirm your Your name, address and phone number to us so that can
verify your fund transfer and make further recommendation for the
release of your long over due fund. Please we will like to hear from
you as soon as you received this message.
Mr. Gary Cantrell.
For: Office of Criminal Investigations
--
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: mpt3sas: fix hang on ata passthru commands
On Sun, 2017-01-01 at 11:33 -0500, David Miller wrote:
> From: Bart Van Assche
> Date: Sun, 1 Jan 2017 14:22:11 +
>
> > My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
> > secure erase premature termination"). Since the mpt3sas driver uses the
> > single-queue approach and since the SCSI core unlocks the block layer
> > request queue lock before the .queuecommand callback function is called,
> > multiple threads can execute that callback function (scsih_qcmd() in this
> > case) simultaneously. This means that using scsi_internal_device_block()
> > from inside .queuecommand to serialize SCSI command execution is wrong.
>
> But this causes a regression for the thing being fixed by that
> commit.
Right, we don't do that; that's why I didn't list it as one of the
options.
> Why don't we figure out what that semantics that commit was trying to
> achieve and implement that properly?
Now that I look at the reviews, each of the reviewers said what the
correct thing to do was: return SAM_STAT_BUSY if SATL commands are
outstanding like the spec says. You all get negative brownie points
for not insisting on a rework.
Does this patch (compile tested only) fix the problems for everyone?
James
---
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 394fe13..0983a65 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -393,6 +393,7 @@ struct MPT3SAS_TARGET {
* @eedp_enable: eedp support enable bit
* @eedp_type: 0(type_1), 1(type_2), 2(type_3)
* @eedp_block_length: block size
+ * @ata_command_pending: SATL passthrough outstanding for device
*/
struct MPT3SAS_DEVICE {
struct MPT3SAS_TARGET *sas_target;
@@ -404,6 +405,17 @@ struct MPT3SAS_DEVICE {
u8 ignore_delay_remove;
/* Iopriority Command Handling */
u8 ncq_prio_enable;
+ /*
+* Bug workaround for SATL handling: the mpt2/3sas firmware
+* doesn't return BUSY or TASK_SET_FULL for subsequent
+* commands while a SATL pass through is in operation as the
+* spec requires, it simply does nothing with them until the
+* pass through completes, causing them possibly to timeout if
+* the passthrough is a long executing command (like format or
+* secure erase). This variable allows us to do the right
+* thing while a SATL command is pending.
+*/
+ u8 ata_command_pending;
};
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b5c966e..1446a0a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -3899,9 +3899,12 @@ _scsih_temp_threshold_events(struct MPT3SAS_ADAPTER *ioc,
}
}
-static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
+static void set_satl_pending(struct scsi_cmnd *scmd, bool pending)
{
- return (scmd->cmnd[0] == ATA_12 || scmd->cmnd[0] == ATA_16);
+ struct MPT3SAS_DEVICE *priv = scmd->device->hostdata;
+
+ if (scmd->cmnd[0] == ATA_12 || scmd->cmnd[0] == ATA_16)
+ priv->ata_command_pending = pending;
}
/**
@@ -3925,9 +3928,7 @@ _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
if (!scmd)
continue;
count++;
- if (ata_12_16_cmd(scmd))
- scsi_internal_device_unblock(scmd->device,
- SDEV_RUNNING);
+ set_satl_pending(scmd, false);
mpt3sas_base_free_smid(ioc, smid);
scsi_dma_unmap(scmd);
if (ioc->pci_error_recovery)
@@ -4063,13 +4064,6 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd
*scmd)
if (ioc->logging_level & MPT_DEBUG_SCSI)
scsi_print_command(scmd);
- /*
-* Lock the device for any subsequent command until command is
-* done.
-*/
- if (ata_12_16_cmd(scmd))
- scsi_internal_device_block(scmd->device);
-
sas_device_priv_data = scmd->device->hostdata;
if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
scmd->result = DID_NO_CONNECT << 16;
@@ -4083,6 +4077,16 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd
*scmd)
return 0;
}
+ /*
+* Bug work around for firmware SATL handling
+*/
+ if (sas_device_priv_data->ata_command_pending) {
+ scmd->result = SAM_STAT_BUSY;
+ scmd->scsi_done(scmd);
+ return 0;
+ }
+ set_satl_pending(scmd, true);
+
sas_target_priv_data = sas_device_priv_data->sas_target;
/* invalid device handle */
@@ -4650,8 +4654,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8
msix_index, u32 reply)
if (scmd == NULL)
return 1;
- if (ata_12_16_cmd(scmd))
-
Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands
From: Bart Van Assche
Date: Sun, 1 Jan 2017 14:22:11 +
> My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
> secure erase premature termination"). Since the mpt3sas driver uses the
> single-queue approach and since the SCSI core unlocks the block layer
> request queue lock before the .queuecommand callback function is called,
> multiple threads can execute that callback function (scsih_qcmd() in this
> case) simultaneously. This means that using scsi_internal_device_block()
> from inside .queuecommand to serialize SCSI command execution is wrong.
But this causes a regression for the thing being fixed by that commit.
Why don't we figure out what that semantics that commit was trying to
achieve and implement that properly?
--
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: mpt3sas: fix hang on ata passthru commands
On 01/01/2017 09:22 AM, Bart Van Assche wrote:
> On Sat, 2016-12-31 at 15:19 -0800, James Bottomley wrote:
>> On Thu, 2016-12-29 at 00:02 -0800, Christoph Hellwig wrote:
>>> On Wed, Dec 28, 2016 at 11:30:24PM -0500, Jason Baron wrote:
Add a new parameter to scsi_internal_device_block() to decide
whether or not to invoke scsi_wait_for_queuecommand().
>>> We'll also need to deal with the blk-mq wait path that Bart has been
>>> working on (I think it's already in the scsi tree, but I'd have to
>>> check).
>>>
>>> Also adding a bool flag for the last call in a function is style
>>> that's a little annoying.
>>>
>>> I'd prefer to add a scsi_internal_device_block_nowait that contains
>>> all the code except for the waiting, and then make
>>> scsi_internal_device_block_nowait a wrapper around it. Or drop the
>>> annoying internal for both while we're at it :)
>> OK, I know it's new year, but this is an unpatched regression in -rc1
>> that's causing serious issues. I would like this fixed by -rc3 so we
>> have 3 options
>>
>>1. revert all the queuecommand wait stuff until it proves it's actually
>> working without regressions
>>2. apply this patch and fix the style issues later
>>3. someone else supplies the correctly styled fix patch
>>
>> The conservative in me says that 1. is probably the most correct thing
>> to do because it gives us time to get the queuecommand wait stuff
>> right; that's what I'll probably do if there's no movement next week.
>> However, since we're reasonably early in the -rc cycle, so either 2 or
>> 3 are viable provided no further regressions caused by the queuecommand
>> wait stuff pop up.
> Hello James,
>
> My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
> secure erase premature termination"). Since the mpt3sas driver uses the
> single-queue approach and since the SCSI core unlocks the block layer
> request queue lock before the .queuecommand callback function is called,
> multiple threads can execute that callback function (scsih_qcmd() in this
> case) simultaneously. This means that using scsi_internal_device_block()
> from inside .queuecommand to serialize SCSI command execution is wrong.
>
> Bart.
Hi,
Yes, commit 18f6084a989b looked racy to me as well. I noted that
in a follow up to my patch, and a suggested a way of fixing it:
http://lkml.iu.edu/hypermail/linux/kernel/1612.3/01301.html
Also worth noting is that commit 18f6084a989b was backported
to at least 4.8 stable.
Thanks,
-Jason
--
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: mpt3sas: fix hang on ata passthru commands
On Sat, 2016-12-31 at 15:19 -0800, James Bottomley wrote:
> On Thu, 2016-12-29 at 00:02 -0800, Christoph Hellwig wrote:
> > On Wed, Dec 28, 2016 at 11:30:24PM -0500, Jason Baron wrote:
> > > Add a new parameter to scsi_internal_device_block() to decide
> > > whether or not to invoke scsi_wait_for_queuecommand().
> >
> > We'll also need to deal with the blk-mq wait path that Bart has been
> > working on (I think it's already in the scsi tree, but I'd have to
> > check).
> >
> > Also adding a bool flag for the last call in a function is style
> > that's a little annoying.
> >
> > I'd prefer to add a scsi_internal_device_block_nowait that contains
> > all the code except for the waiting, and then make
> > scsi_internal_device_block_nowait a wrapper around it. Or drop the
> > annoying internal for both while we're at it :)
>
> OK, I know it's new year, but this is an unpatched regression in -rc1
> that's causing serious issues. I would like this fixed by -rc3 so we
> have 3 options
>
>1. revert all the queuecommand wait stuff until it proves it's actually
> working without regressions
>2. apply this patch and fix the style issues later
>3. someone else supplies the correctly styled fix patch
>
> The conservative in me says that 1. is probably the most correct thing
> to do because it gives us time to get the queuecommand wait stuff
> right; that's what I'll probably do if there's no movement next week.
> However, since we're reasonably early in the -rc cycle, so either 2 or
> 3 are viable provided no further regressions caused by the queuecommand
> wait stuff pop up.
Hello James,
My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
secure erase premature termination"). Since the mpt3sas driver uses the
single-queue approach and since the SCSI core unlocks the block layer
request queue lock before the .queuecommand callback function is called,
multiple threads can execute that callback function (scsih_qcmd() in this
case) simultaneously. This means that using scsi_internal_device_block()
from inside .queuecommand to serialize SCSI command execution is wrong.
Bart.
