RE: [PATCH v3 06/24] mpi3mr: add support of event handling part-1

[Kashyap Desai <kashyap.desai@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 8579 bytes --]

> > @@ -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@suse.de			               +49 911 74053 688
> SUSE Software Solutions Germany GmbH, 90409 Nürnberg
> GF: F. Imendörffer, HRB 36809 (AG Nürnberg)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]