Re: [PATCH v8] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Bart Van Assche]

On 06/28/2016 10:57 PM, Bryant G. Ly wrote:

I was hoping someone in the community can help answer this:

If I were to remove the masking off of the lun addressing method prior to 
target_submit_cmd/target_submit_tmr then I get a hang within scsi probe
on bootup. (srp->lun.scsi_lun[0] &= 0x3f; and srp_tsk->lun.scsi_lun[0] &= 0x3f;)

[0.842648] ibmvscsi 300d: sent SRP login
[0.842667] ibmvscsi 300d: SRP_LOGIN succeeded
[0.842682] scsi 0:0:2:0: CD-ROMAIX  VOPTA 
PQ: 0 ANSI: 4
[  OK  ] Started Show Plymouth Boot Screen.
[  OK  ] Reached target Paths.
[  OK  ] Reached target Basic System.
[0.886179] sr 0:0:2:0: [sr0] scsi-1 drive
[0.886199] cdrom: Uniform CD-ROM driver Revision: 3.20
[0.888582] sd 0:0:1:0: [sda] 41943040 512-byte logical blocks: (21.4 
GB/20.0 GiB)
[0.888657] sd 0:0:1:0: [sda] Write Protect is off
[0.888712] sd 0:0:1:0: [sda] Cache data unavailable
[0.888722] sd 0:0:1:0: [sda] Assuming drive cache: write through
[0.901443]  sda: sda1 sda2 sda3
[0.901838] sd 0:0:1:0: [sda] Attached SCSI disk
[  124.522778] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts
[  125.151242] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts
[  125.662808] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts

Does anyone know of the reasoning for having to mask off the addressing method 
prior
to submitting to target?


How long have you waited before giving up waiting? The SCSI initiator 
sends a REPORT LUN and other SCSI commands to LUN 0. If LUN 0 is missing 
these commands will time out but that should not cause a hang. Anyway, 
SysRq-w should help to determine the cause of the hang.


Bart.

--
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 new bug in scsi_dev_info_list string matching

[Martin K. Petersen]

> "Alan" == Alan Stern  writes:

Alan> Commit b704f70ce200 ("SCSI: fix bug in scsi_dev_info_list
Alan> matching") changed the way vendor- and model-string matching was
Alan> carried out in the routine that looks up entries in a SCSI devinfo
Alan> list.  The new matching code failed to take into account the case
Alan> of a maximum-length string; in such cases it could end up testing
Alan> for a terminating '\0' byte beyond the end of the memory allocated
Alan> to the string.  This out-of-bounds bug was detected by UBSAN.

Applied to 4.7/scsi-fixes.

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

Re: [PATCH 1/1] ipr: Clear interrupt on croc/crocodile when running with LSI

[Martin K. Petersen]

> "Brian" == Brian King  writes:

Brian> If we fall back to using LSI on the Croc or Crocodile chip we
Brian> need to clear the interrupt so we don't hang the system.

Applied to 4.7/scsi-fixes.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
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 v8] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Bryant G. Ly]

On 6/27/16, 3:17 PM, "Michael Cyr"  wrote:



+   cmd->se_cmd.tag = be64_to_cpu(srp->tag);
+
+   spin_lock_bh(>intr_lock);
+   list_add_tail(>list, >active_q);
+   spin_unlock_bh(>intr_lock);
+
+   srp->lun.scsi_lun[0] &= 0x3f;
+
+   pr_debug("calling submit_cmd, se_cmd %p, lun 0x%llx, cdb 0x%x, 
attr:%d\n",
+>se_cmd, scsilun_to_int(>lun), (int)srp->cdb[0],
+attr);
+
+   rc = target_submit_cmd(>se_cmd, nexus->se_sess, srp->cdb,
+  cmd->sense_buf, scsilun_to_int(>lun),
+  data_len, attr, dir, 0);
+   if (rc) {



+ * ibmvscsis_parse_task() - Parse SRP Task Management Request
+ *
+ * Parse the srp task management request; if it is valid then submit it to tcm.
+ * Note: The return code does not reflect the status of the task management
+ * request.
+ *
+ * EXECUTION ENVIRONMENT:
+ * Processor level
+ */
+static void ibmvscsis_parse_task(struct scsi_info *vscsi,
+struct ibmvscsis_cmd *cmd)
+{


+   spin_lock_bh(>intr_lock);
+   list_add_tail(>list, >active_q);
+   spin_unlock_bh(>intr_lock);
+
+   srp_tsk->lun.scsi_lun[0] &= 0x3f;
+
+   pr_debug("calling submit_tmr, func %d\n",
+srp_tsk->tsk_mgmt_func);
+   rc = target_submit_tmr(>se_cmd, nexus->se_sess, NULL,
+  scsilun_to_int(_tsk->lun), srp_tsk,
+  tcm_type, GFP_KERNEL, tag_to_abort, 0);
+   if (rc) {


I was hoping someone in the community can help answer this:

If I were to remove the masking off of the lun addressing method prior to 
target_submit_cmd/target_submit_tmr then I get a hang within scsi probe 
on bootup. (srp->lun.scsi_lun[0] &= 0x3f; and srp_tsk->lun.scsi_lun[0] &= 0x3f;)

[0.842648] ibmvscsi 300d: sent SRP login
[0.842667] ibmvscsi 300d: SRP_LOGIN succeeded
[0.842682] scsi 0:0:2:0: CD-ROMAIX  VOPTA 
PQ: 0 ANSI: 4
[  OK  ] Started Show Plymouth Boot Screen.
[  OK  ] Reached target Paths.
[  OK  ] Reached target Basic System.
[0.886179] sr 0:0:2:0: [sr0] scsi-1 drive
[0.886199] cdrom: Uniform CD-ROM driver Revision: 3.20
[0.888582] sd 0:0:1:0: [sda] 41943040 512-byte logical blocks: (21.4 
GB/20.0 GiB)
[0.888657] sd 0:0:1:0: [sda] Write Protect is off
[0.888712] sd 0:0:1:0: [sda] Cache data unavailable
[0.888722] sd 0:0:1:0: [sda] Assuming drive cache: write through
[0.901443]  sda: sda1 sda2 sda3
[0.901838] sd 0:0:1:0: [sda] Attached SCSI disk
[  124.522778] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts
[  125.151242] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts
[  125.662808] dracut-initqueue[353]: Warning: dracut-initqueue timeout - 
starting timeout scripts

Does anyone know of the reasoning for having to mask off the addressing method 
prior
to submitting to target?

Thanks,

Bryant


--
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] lpfc: Fix possible NULL pointer dereference

[Tyrel Datwyler]

On 06/15/2016 06:00 AM, Johannes Thumshirn wrote:
> Check for the existance of pciob->vport before accessing it.

piocb mispelled.

> 
> Signed-off-by: Johannes Thumshirn 
> ---
>  drivers/scsi/lpfc/lpfc_sli.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 70edf21..134078f 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -1329,15 +1329,10 @@ lpfc_sli_ringtxcmpl_put(struct lpfc_hba *phba, struct 
> lpfc_sli_ring *pring,
>   if ((unlikely(pring->ringno == LPFC_ELS_RING)) &&
>  (piocb->iocb.ulpCommand != CMD_ABORT_XRI_CN) &&
>  (piocb->iocb.ulpCommand != CMD_CLOSE_XRI_CN) &&
> -  (!(piocb->vport->load_flag & FC_UNLOADING))) {
> - if (!piocb->vport)
> - BUG();

Granted the previous code would crash and burn in the if statement prior
to the BUG() assertion if piocb->vport was NULL, but is the condition
!piocb->vport still a bug here? Should that case still be asserted?

-Tyrel

> - else
> - mod_timer(>vport->els_tmofunc,
> - jiffies +
> - msecs_to_jiffies(1000 * (phba->fc_ratov << 1)));
> - }
> -
> + piocb->vport && !(piocb->vport->load_flag & FC_UNLOADING))
> + mod_timer(>vport->els_tmofunc,
> +   jiffies +
> +   msecs_to_jiffies(1000 * (phba->fc_ratov << 1)));
>  
>   return 0;
>  }
> 

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

Re: [Open-FCoE] [PATCH] MAINTAINERS: Change FCoE maintainer

[Vasu Dev]

On Thu, 2016-06-23 at 10:22 +0200, Johannes Thumshirn wrote:
> Vasu is going to resign from his maintainer role and I'll take over.
> 
> Signed-off-by: Johannes Thumshirn 
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e1b090f..70af8c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4661,7 +4661,7 @@ S:  Maintained
>  F:   drivers/staging/fbtft/
>  
>  FCOE SUBSYSTEM (libfc, libfcoe, fcoe)
> -M:   Vasu Dev 
> +M:   Johannes Thumshirn 
>  L:   fcoe-de...@open-fcoe.org
>  W:   www.Open-FCoE.org
>  S:   Supported

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

Re: [PATCH V3 1/2] smartpqi: initial commit of Microsemi smartpqi driver

[Johannes Thumshirn]

On Mon, Jun 27, 2016 at 09:12:11PM +, Don Brace wrote:
[...]

> > > ---
> > 
> > [...]
> > 
> > > + */
> > > +
> > > +#if !defined(_SMARTPQI_H)
> > > +#define _SMARTPQI_H
> > > +
> > > +#pragma pack(1)
> > 
> > I know GCC does understand #pragma pack() to be compatible with MSVC
> > but please
> > use the kernel's __packed convention on structs you want to have packed.
> > 
> 
> We prefer to leave this "as is" for these reasons:
> 
> We do not use __packed on other Microsemi Linux drivers.
> 
> There's not consistency in the usage of __packed. Just a quick survey of the 
> kernel source reveals at least four styles:
> 

So your making a 5th version, c.f. https://xkcd.com/927/

Anyway, a IMHO more important question

> > > +static void pqi_complete_queued_requests(struct pqi_ctrl_info *ctrl_info,
> > > + struct pqi_scsi_dev_t *device_in_reset)
> > > +{
> > > + unsigned int i;
> > > + unsigned int path;
> > > + struct pqi_queue_group *queue_group;
> > > + unsigned long flags;
> > > + struct pqi_io_request *io_request;
> > > + struct pqi_io_request *next;
> > > + struct scsi_cmnd *scmd;
> > > + struct pqi_scsi_dev_t *device;
> > > +
> > > + for (i = 0; i < ctrl_info->num_queue_groups; i++) {
> > > + queue_group = _info->queue_groups[i];
> > > +
> > > + for (path = 0; path < 2; path++) {
> > > + spin_lock_irqsave(_group->submit_lock[path],
> > > + flags);
> > > +
> > > + list_for_each_entry_safe(io_request, next,
> > > + _group->request_list[path],
> > > + request_list_entry) {
> > > + scmd = io_request->scmd;
> > > + if (scmd) {
> > > + device = scmd->device->hostdata;
> > > + if (device == device_in_reset) {
> > > + set_host_byte(scmd, 
> > > DID_RESET);
> > > + pqi_scsi_done(scmd);
> > > + list_del(_request->
> > > + request_list_entry);
> > > + }
> > > + }
> > > + }
> > > +
> > > + 
> > > spin_unlock_irqrestore(_group->submit_lock[path],
> > > + flags);
> > > + }
> > 
> > If you'd factor the inner for loop into an own small (inline) function, it
> > could drastically improve readability here.
> > 
> > something like
> > 
> > static void pqi_complete_queue_group(struct pqi_queue_group *qg, struct
> >  pqi_device_t *device_in_reset)
> > {
> > unsigned int path;
> > unsigned long flags;
> > struct qpi_scsi_dev_t *dev;
> > struct pqi_io_request *ior, *next;
> > 
> > for (path = 0; path < 2; path++) {
> > spin_lock_irqsave(>submit_lock[path], flags);
> > 
> > list_for_each_entry_save(ior, next, >request_list[path],
> >  request_list_entry) {
> > scmd = ior->scmnd;
> > if (!scmd)
> > continue;
> > 
> > device = scmnd->device->hostdata;
> > if (dev == device_in_reset) {
> > set_host_byte(scmnd, DID_RESET);
> > pqi_scsi_done(scmnd);
> > list_del(>request_list_entry);
> > }
> > }
> > spin_unlock_irqresetore(>submit_lock[path], flags);
> > }
> > }

Does this have to be under a spin_lock and then even the irqsave
variant? From my understanding scsi-mq/blk-mq is supposed to place the
queues per-cpu. So if you're now deactivating local IRQs here you're throwing
all optimizations out of the window. Can't you have the request lists and
queues per-cpu so this code can run lockless?

Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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