> > @@ -612,7 +1942,25 @@ static void mpi3mr_target_destroy(struct
> scsi_target *starget)
> > */
> > static int mpi3mr_slave_configure(struct scsi_device *sdev) {
> > + struct scsi_target *starget;
> > + struct Scsi_Host *shost;
> > + struct mpi3mr_ioc *mrioc;
> > + struct mpi3mr_tgt_dev *tgt_dev;
> > + unsigned long flags;
> > int retval = 0;
> > +
> > + starget = scsi_target(sdev);
> > + shost = dev_to_shost(&starget->dev);
> > + mrioc = shost_priv(shost);
> > +
> > + spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> > + tgt_dev = __mpi3mr_get_tgtdev_by_perst_id(mrioc, starget->id);
> > + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> > + if (!tgt_dev)
> > + return retval;
> > +
>
> Return '0' on unknown SCSI devices? Really?
Hannes - I will fix this in V4. I am planning to send V4 today. Please
review.
>
> > + mpi3mr_tgtdev_put(tgt_dev);
> > +
> > return retval;
> > }
> >
> > @@ -626,7 +1974,37 @@ static int mpi3mr_slave_configure(struct
> scsi_device *sdev)
> > */
> > static int mpi3mr_slave_alloc(struct scsi_device *sdev) {
> > + struct Scsi_Host *shost;
> > + struct mpi3mr_ioc *mrioc;
> > + struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data;
> > + struct mpi3mr_tgt_dev *tgt_dev;
> > + struct mpi3mr_sdev_priv_data *scsi_dev_priv_data;
> > + unsigned long flags;
> > + struct scsi_target *starget;
> > int retval = 0;
> > +
> > + starget = scsi_target(sdev);
> > + shost = dev_to_shost(&starget->dev);
> > + mrioc = shost_priv(shost);
> > + scsi_tgt_priv_data = starget->hostdata;
> > +
> > + scsi_dev_priv_data = kzalloc(sizeof(*scsi_dev_priv_data),
> GFP_KERNEL);
> > + if (!scsi_dev_priv_data)
> > + return -ENOMEM;
> > +
> > + scsi_dev_priv_data->lun_id = sdev->lun;
> > + scsi_dev_priv_data->tgt_priv_data = scsi_tgt_priv_data;
> > + sdev->hostdata = scsi_dev_priv_data;
> > +
> > + scsi_tgt_priv_data->num_luns++;
> > +
> > + spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> > + tgt_dev = __mpi3mr_get_tgtdev_by_perst_id(mrioc, starget->id);
> > + if (tgt_dev && (tgt_dev->starget == NULL))
> > + tgt_dev->starget = starget;
> > + if (tgt_dev)
> > + mpi3mr_tgtdev_put(tgt_dev);
> > + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> > return retval;
> > }
> >
> Same here. I would have expected -ENXIO to be returned fi the tgt_dev is
> not
> found.
> And you can fold the two 'if' clauses into one eg like:
This is fixed in in V4.
>
> if (tgt_dev) {
> if (tgt_dev->starget == NULL)
> tgt_dev = starget;
> mpi3mr_tgtdev_put(tgt_dev);
> retval = 0;
> }
>
> > @@ -640,7 +2018,33 @@ static int mpi3mr_slave_alloc(struct scsi_device
> *sdev)
> > */
> > static int mpi3mr_target_alloc(struct scsi_target *starget) {
> > + struct Scsi_Host *shost = dev_to_shost(&starget->dev);
> > + struct mpi3mr_ioc *mrioc = shost_priv(shost);
> > + struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data;
> > + struct mpi3mr_tgt_dev *tgt_dev;
> > + unsigned long flags;
> > int retval = -ENODEV;
> > +
> > + scsi_tgt_priv_data = kzalloc(sizeof(*scsi_tgt_priv_data),
> GFP_KERNEL);
> > + if (!scsi_tgt_priv_data)
> > + return -ENOMEM;
> > +
> > + starget->hostdata = scsi_tgt_priv_data;
> > + scsi_tgt_priv_data->starget = starget;
> > + scsi_tgt_priv_data->dev_handle = MPI3MR_INVALID_DEV_HANDLE;
> > +
> > + spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> > + tgt_dev = __mpi3mr_get_tgtdev_by_perst_id(mrioc, starget->id);
> > + if (tgt_dev && !tgt_dev->is_hidden) {
> > + scsi_tgt_priv_data->dev_handle = tgt_dev->dev_handle;
> > + scsi_tgt_priv_data->perst_id = tgt_dev->perst_id;
> > + scsi_tgt_priv_data->dev_type = tgt_dev->dev_type;
> > + scsi_tgt_priv_data->tgt_dev = tgt_dev;
> > + tgt_dev->starget = starget;
> > + atomic_set(&scsi_tgt_priv_data->block_io, 0);
> > + retval = 0;
> > + }
> > + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> > return retval;
> > }
> >
> Ah, here is the correct value set.
> (But wasn't it ENXIO which should've been returned for unknown targets?)
This is fixed in V4.
>
> > @@ -836,7 +2240,7 @@ mpi3mr_probe(struct pci_dev *pdev, const struct
> > pci_device_id *id) {
> > struct mpi3mr_ioc *mrioc = NULL;
> > struct Scsi_Host *shost = NULL;
> > - int retval = 0;
> > + int retval = 0, i;
> >
> > shost = scsi_host_alloc(&mpi3mr_driver_template,
> > sizeof(struct mpi3mr_ioc));
> > @@ -857,11 +2261,21 @@ mpi3mr_probe(struct pci_dev *pdev, const
> struct pci_device_id *id)
> > spin_lock_init(&mrioc->admin_req_lock);
> > spin_lock_init(&mrioc->reply_free_queue_lock);
> > spin_lock_init(&mrioc->sbq_lock);
> > + spin_lock_init(&mrioc->fwevt_lock);
> > + spin_lock_init(&mrioc->tgtdev_lock);
> > spin_lock_init(&mrioc->watchdog_lock);
> > spin_lock_init(&mrioc->chain_buf_lock);
> >
> > + INIT_LIST_HEAD(&mrioc->fwevt_list);
> > + INIT_LIST_HEAD(&mrioc->tgtdev_list);
> > + INIT_LIST_HEAD(&mrioc->delayed_rmhs_list);
> > +
> > mpi3mr_init_drv_cmd(&mrioc->init_cmds,
> MPI3MR_HOSTTAG_INITCMDS);
> >
> > + for (i = 0; i < MPI3MR_NUM_DEVRMCMD; i++)
> > + mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
> > + MPI3MR_HOSTTAG_DEVRMCMD_MIN + i);
> > +
> > if (pdev->revision)
> > mrioc->enable_segqueue = true;
> >
> > @@ -877,6 +2291,17 @@ mpi3mr_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> > shost->max_channel = 1;
> > shost->max_id = 0xFFFFFFFF;
> >
> > + snprintf(mrioc->fwevt_worker_name, sizeof(mrioc-
> >fwevt_worker_name),
> > + "%s%d_fwevt_wrkr", mrioc->driver_name, mrioc->id);
> > + mrioc->fwevt_worker_thread = alloc_ordered_workqueue(
> > + mrioc->fwevt_worker_name, WQ_MEM_RECLAIM);
> > + if (!mrioc->fwevt_worker_thread) {
> > + ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> > + __FILE__, __LINE__, __func__);
> > + retval = -ENODEV;
> > + goto out_fwevtthread_failed;
> > + }
> > +
> > mrioc->is_driver_loading = 1;
> > if (mpi3mr_init_ioc(mrioc)) {
> > ioc_err(mrioc, "failure at %s:%d/%s()!\n", @@ -903,6 +2328,8
> @@
> > mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > addhost_failed:
> > mpi3mr_cleanup_ioc(mrioc);
> > out_iocinit_failed:
> > + destroy_workqueue(mrioc->fwevt_worker_thread);
> > +out_fwevtthread_failed:
> > spin_lock(&mrioc_list_lock);
> > list_del(&mrioc->list);
> > spin_unlock(&mrioc_list_lock);
> > @@ -924,14 +2351,30 @@ static void mpi3mr_remove(struct pci_dev
> *pdev)
> > {
> > struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > struct mpi3mr_ioc *mrioc;
> > + struct workqueue_struct *wq;
> > + unsigned long flags;
> > + struct mpi3mr_tgt_dev *tgtdev, *tgtdev_next;
> >
> > mrioc = shost_priv(shost);
> > while (mrioc->reset_in_progress || mrioc->is_driver_loading)
> > ssleep(1);
> >
> > mrioc->stop_drv_processing = 1;
> > + mpi3mr_cleanup_fwevt_list(mrioc);
> > + spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> > + wq = mrioc->fwevt_worker_thread;
> > + mrioc->fwevt_worker_thread = NULL;
> > + spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
> > + if (wq)
> > + destroy_workqueue(wq);
> >
> > scsi_remove_host(shost);
> > + list_for_each_entry_safe(tgtdev, tgtdev_next, &mrioc->tgtdev_list,
> > + list) {
> > + mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev);
> > + mpi3mr_tgtdev_del_from_list(mrioc, tgtdev);
> > + mpi3mr_tgtdev_put(tgtdev);
> > + }
> >
> > mpi3mr_cleanup_ioc(mrioc);
> >
> > @@ -955,6 +2398,8 @@ static void mpi3mr_shutdown(struct pci_dev
> *pdev)
> > {
> > struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > struct mpi3mr_ioc *mrioc;
> > + struct workqueue_struct *wq;
> > + unsigned long flags;
> >
> > if (!shost)
> > return;
> > @@ -963,6 +2408,13 @@ static void mpi3mr_shutdown(struct pci_dev
> *pdev)
> > while (mrioc->reset_in_progress || mrioc->is_driver_loading)
> > ssleep(1);
> > mrioc->stop_drv_processing = 1;
> > + mpi3mr_cleanup_fwevt_list(mrioc);
> > + spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> > + wq = mrioc->fwevt_worker_thread;
> > + mrioc->fwevt_worker_thread = NULL;
> > + spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
> > + if (wq)
> > + destroy_workqueue(wq);
> >
> > mpi3mr_cleanup_ioc(mrioc);
> >
> >
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> hare@xxxxxxx +49 911 74053 688
> SUSE Software Solutions Germany GmbH, 90409 Nürnberg
> GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
